Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom cursor #1183

Merged
merged 2 commits into from
Sep 30, 2020
Merged

Custom cursor #1183

merged 2 commits into from
Sep 30, 2020

Conversation

jneem
Copy link
Collaborator

@jneem jneem commented Sep 4, 2020

This isn't finished yet (at the very least, the windows implementation needs testing and error handling), but I'm posting it here because I have a question: the lowest-common-denominator across platforms seems to be that we can create cursors from bitmaps in memory, so it would be really handy to have support for that in druid-shell. I've put in ImageBuf as a placeholder, but there's a ton of overlap with druid::widgets::ImageData, but I didn't want to move ImageData into druid-shell because that would entail fiddling with the "image" feature. So my questions are:

  • would it make sense for piet to have some kind of bitmap type?
  • if so, where would image loading live? Also in piet?

@cmyr
Copy link
Member

cmyr commented Sep 6, 2020

cool, this is definitely something on my radar, so happy to have someone else interested too.

My feeling is that this needs to be a per-platform thing in druid-shell, and shouldn't really concern piet. I think cursors are going to have to be resource files that are provided per-platform; at launch time you would load these in druid, and then you could reference them by some sort of name or id when you wanted to set the cursor.

This per-platform resource thing is something (like application icons) that we haven't really tackled, yet, and so I'm just expressing my current best-guess for how this should work.

I've skimmed this patch and it doesn't seem unreasonable, but my gut is still that cursor data should be opaque; It may be that different platforms have different conventions around cursor size, or how you indicate a cursor's hit-position relative to the image resource, etc etc. I'll give this some more thought, though!

@raphlinus
Copy link
Contributor

I looked at winit to see if they have this functionality (I think we should always try to learn from them), but it seems like they pretty much only have an enum of standard variants for now, though there is an issue to add custom cursors. Might be worth taking a closer look at that to see how they're thinking about it (again, I didn't dig in).

Getting this really clean cross-platform might be challenging, especially because it's an area where the design of the cursor maybe should be platform-specific, but yes, it seems like useful functionality within the intended scope of druid-shell. I also have no problem with this moving forward.

@jneem
Copy link
Collaborator Author

jneem commented Sep 7, 2020

Regarding the cross-platform possibilities, I found the following information (mostly by googling and looking at API docs, which I didn't find particularly discoverable so I probably missed some things).

  • On windows, the main ways to load a cursor seem to be LoadImage, which loads a file from disk or a resource from the exe, and CreateIconIndirect, which can create a cursor from an in-memory bitmap. I couldn't find any authoritative documentation regarding the formats that LoadImage supports. There's also LoadCursor, but it's apparently superceded by LoadImage, and LoadCursorFromFile, which can load from a file but not from an exe resouce. At least that one documents the supported formats: .ani or .cur. Oh, and there's CreateCursor for creating cursors from in-memory data, but I think that one can't do color.
  • On mac, NSCursor can be constructed from an NSImage, which seems to be a flexible representation of in-memory images. It supports, for example, loading file from disk in a number of different formats. You can also convert an in-memory bitmap to an NSImage.
  • On linux, the underlying mechanism in X11 converts X pixmaps to cursors. GTK adds some convenience functions for making cursors from cairo surfaces or gdk pixbufs (which supports loading images in various formats).
  • Web seems to just support loading a cursor from a url? I have no idea how to package that up nicely in a wasm thing...

Leaving aside web for now, we basically seem to have a choice between 1) use platform-specific code to load image files and/or resources into a cursor, or 2) use platform-independent code to load image files into a buffer and then use platform-specific functionality to turn those buffers into a cursor. I went for 2 in this PR, partly because we already have some code for loading image files into a buffer and partly because 1 has the disadvantage of introducing extra platform-dependent packaging steps (i.e. turning say, a png cursor into a .cur on Windows).

@jneem
Copy link
Collaborator Author

jneem commented Sep 17, 2020

I had a quick look at the winit PR.

  • They have a "pixel buffer" type, but it only supports ARGB (whereas the one in this PR supports multiple formats)
  • They expose a cross-platform API for turning pixel buffers into cursors (and window icons, too)
  • On windows, they expose an API for loading cursors from resources or files.
  • Windows cursors automatically resize themselves sometimes. I don't know the best way to expose this (and it isn't fully implemented in the winit PR)

@cmyr
Copy link
Member

cmyr commented Sep 18, 2020

Sorry to let this sit. Your summary sounds accurate to me, and I think you describe the possible implementation approaches well. My gut prefers doing platform-specific stuff, but that would be blocked on resource bundles, so I'm okay with the other approach for the time being?

I'm not sure if we could use the piet::Image type to store our buffer, but that would be a nice advantage if we could. Otherwise I'm fine with adding some generic buffer type to druid to handle this, and we can consider moving it to piet in the future?

@jneem jneem marked this pull request as ready for review September 20, 2020 22:55
@jneem
Copy link
Collaborator Author

jneem commented Sep 20, 2020

Ok, I think this is ready for another look. I couldn't use RenderContext::Image because it doesn't have an easy way to extract the pixel data. Also, I'm still not sure what's the right place for ImageBuf and the image loading code, but I have them in their own modules now, so they should be easy to move later.

@cmyr cmyr self-requested a review September 22, 2020 15:34
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this looks good to me; a few little notes but no blockers. I think there are some larger questions about whether or not we would like a more general image format in piet, but this is a very reasonable solution for now. Feel free to open an issue about adding support on mac; I'll do that at some point if nobody beats me to it.

druid-shell/src/image.rs Show resolved Hide resolved
) -> impl Iterator<Item = impl Iterator<Item = Color> + 'a> + 'a {
// TODO: a version of this exists in piet-web and piet-coregraphics. Maybe put it somewhere
// common?
fn unpremul(x: u8, a: u8) -> u8 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see this in the piet util module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll make a piet PR

druid-shell/src/image.rs Outdated Show resolved Hide resolved
druid-shell/src/mouse.rs Outdated Show resolved Hide resolved
druid/src/image.rs Outdated Show resolved Hide resolved
@jneem
Copy link
Collaborator Author

jneem commented Sep 29, 2020

Ok, I think this addresses the druid comments. I'll make a separate piet pr.

I'll let this sit until at least tomorrow in case of further comments, and then if I haven't heard any objections I'll merge.

@jneem jneem merged commit 59f6750 into linebender:master Sep 30, 2020
@jneem jneem mentioned this pull request Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants