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

CustomCursor::Image could use a custom size option #17276

Open
marioferpa opened this issue Jan 10, 2025 · 28 comments
Open

CustomCursor::Image could use a custom size option #17276

marioferpa opened this issue Jan 10, 2025 · 28 comments
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@marioferpa
Copy link

What problem does this solve or what need does it fill?

The new CustomCursor::Image solution in Bevy 0.15 is a great addition, making it simple to replace the cursor with an image instead of hiding the original cursor and making a Sprite follow its position.

commands
  .entity(window_entity)
  .insert(
      CursorIcon::Custom(
          CustomCursor::Image {
              handle:     asset_server.load("data/vanilla/sprites/kenney_hand.png").into(),
              hotspot:    (0, 0)
          }
      )
  )
;

An advantage that the previous method had, however, was complete control about the image size, rotation, etc. In my current project, as an example, sprites are not represented with their original resolution, making the cursor look too small in comparison.

What solution would you like?

An extra field in CustomCursor::Image allowing for the sprite to be stretched or shrinked.

What alternative(s) have you considered?

A quick solution would be to modify the source file to make it bigger. This would mean however that using a different tileset with a different resolution would require making a new cursor as well, so their relative sizes keep being the same.

If one wanted to animate the cursor (making it grow when approaching something, or making it grow and shrink rhythmically) it would require making a cursor spritesheet, when changing the size value would be simpler.

@marioferpa marioferpa added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 10, 2025
@BenjaminBrienen BenjaminBrienen added A-UI Graphical user interfaces, styles, layouts, and widgets S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Needs-Triage This issue needs to be labelled labels Jan 11, 2025
@mgi388
Copy link
Contributor

mgi388 commented Jan 22, 2025

I started looking at this and it got me wondering about the best behavior for hotspots when users provide image manipulation settings like flip_x|y and now custom_size. Trying to work out if users would expect Bevy to adjust their provided hotspot based on the image transforms, or if users would expect to also provide their transformed hotspot. Example:

CursorIcon::Custom(CustomCursor::Image {
    handle: asset_server.load("rotate-cursor.png"), // source is 32x32
    texture_atlas: None,
    flip_x: true,
    flip_y: false,
    rect: None,
    hotspot: (15, 14), // should users pass the hotspot that matches the source or the transformed image?
    custom_size: Some((64, 64).into()), // x2
})

Also note that the cursor has flip_y, and so the hotspot.y field would also need to change.

Should users provided the final hotspot value, or should Bevy mutate that based on image transforms it did?

Another alternative is to not add custom_size but instead add scale: Option<Vec2>. The above would change to this:

CursorIcon::Custom(CustomCursor::Image {
    handle: asset_server.load("rotate-cursor.png"), // source is 32x32
    texture_atlas: None,
    flip_x: true,
    flip_y: false,
    rect: None,
    hotspot: (15, 14), // should users pass the hotspot that matches the source or the transformed image?
    scale: Some((2.0, 2.0).into()),
})

With regards to hotspots, I think it would feel more natural to just leave the source/original hotspot as it was (as it came from the cursor authoring software), and by providing scale you are asking Bevy to handle the scaling of your image as well as adjusting the hotspot for you.

Another benefit of scale is that it may feel more natural to use with texture atlases. When you are picking a sprite from a texture atlas, you don't really have that sprite image's size at hand (of course you can get it), so putting down an explicit custom size may seem annoying. Also, readers may think that it's changing the size of the entire texture atlas, but it's technically just changing the single sprite that Bevy picked from the atlas.

A downside of scale is that it means for users that do want an explicit custom size, they'd need to look up the image first and reverse engineer a scale from it.

At least for my own use case, I actually just want to scale my cursors up by 1.5, I don't care much for writing down an exact custom_size.

@UkoeHB
Copy link
Contributor

UkoeHB commented Jan 23, 2025

Scale makes sense to me and should cover most use-cases.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

Scale being Vec2 is annoying due to CustomCursor deriving Eq and Hash as previously discovered.

So this either makes scale a non-starter, or we need to remove Eq and Hash from the type.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

Proposed type with scale (if we didn't derive Eq and Hash), and importantly with updated field docs telling users that they don't need to adjust hotspot if flipping or scaling.

#[derive(Debug, Clone, Reflect, PartialEq)]
pub enum CustomCursor {
    /// Image to use as a cursor.
    Image {
        /// The image must be in 8 bit int or 32 bit float rgba. PNG images
        /// work well for this.
        handle: Handle<Image>,
        /// The (optional) texture atlas used to render the image.
        texture_atlas: Option<TextureAtlas>,
        /// Whether the image should be flipped along its x-axis.
        ///
        /// If true, the cursor's hotspot will be flipped along with the image.
        /// You don't need to adjust the `hotspot` that you provide to account
        /// for the flip.
        flip_x: bool,
        /// Whether the image should be flipped along its y-axis.
        ///
        /// If true, the cursor's hotspot will be flipped along with the image.
        /// You don't need to adjust the `hotspot` that you provide to account
        /// for the flip.
        flip_y: bool,
        /// An optional rectangle representing the region of the image to
        /// render, instead of rendering the full image. This is an easy one-off
        /// alternative to using a [`TextureAtlas`].
        ///
        /// When used with a [`TextureAtlas`], the rect is offset by the atlas's
        /// minimal (top-left) corner position.
        rect: Option<URect>,
        /// X and Y coordinates of the hotspot in pixels. The hotspot must be
        /// within the image bounds.
        ///
        /// If you are flipping or scaling the image using `flip_x`, `flip_y`,
        /// or `scale`, you don't need to adjust the `hotspot` that you provide
        /// to account for the flip or scale: it will be adjusted automatically.
        hotspot: (u16, u16),
        /// An optional scale factor to apply to the image.
        ///
        /// If provided, the image will be scaled by this factor. The cursor's
        /// hotspot will be scaled along with the image. You don't need to
        /// adjust the `hotspot` that you provide to account for the scale.
        scale: Option<Vec2>,
        #[reflect(ignore)]
        /// An optional filter to apply when scaling the image. If not provided,
        /// defaults to linear filtering.
        scale_filter: Option<imageops::FilterType>,
    },
    #[cfg(all(target_family = "wasm", target_os = "unknown"))]
    /// A URL to an image to use as the cursor.
    Url {
        /// Web URL to an image to use as the cursor. PNGs preferred. Cursor
        /// creation can fail if the image is invalid or not reachable.
        url: String,
        /// X and Y coordinates of the hotspot in pixels. The hotspot must be
        /// within the image bounds.
        hotspot: (u16, u16),
    },
}

Edit:

To support reflect, the scale_filter field may get its own copy/wrapper instead of using imageops::FilterType, if required.

We could consider merging the scale and scale_filter fields into one but it means introducing a new type like CustomCursorImageScaling which may or may not be better.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

POC.

  • I press +/= to increase/decrease the scale.
  • I press F to cycle through the image filter used when scaling.
Screen.Recording.2025-01-23.at.3.08.09.pm.mov

@UkoeHB
Copy link
Contributor

UkoeHB commented Jan 23, 2025

Can you explain a little about why you included scale_filter (and where is imageops::FilterType coming from?)?

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

Can you explain a little about why you included scale_filter (and where is imageops::FilterType coming from?)?

If the image is normally 32x32 and you set scale: Vec2::splat(5.0) i.e. increase by 5x, the image is resized/upscaled. I used:

let resized = dyn_image.resize(
    (width * scale.x) as u32,
    (height * scale.y) as u32,
    scale_filter.unwrap_or(image::imageops::FilterType::Nearest),
);

To do the resizing, and it needs users to tell it what filter to use when scaling.

As an aside: winit doesn't allow us to pass a custom size / scale or filter when setting the cursor, so we need to do all this on the CPU before we send it to winit.

To answer your question practically, this is one of my cursors scaled to 1.5x with the default / Nearest filtering:

nearest.mov

And with CatmullRom, it arguably looks much better:

calmullrom.mov

Therefore, I did this to have choice here.

@UkoeHB
Copy link
Contributor

UkoeHB commented Jan 23, 2025

Got it, that's a compelling example thanks!

@UkoeHB
Copy link
Contributor

UkoeHB commented Jan 23, 2025

Can you open a PR with the work you've done? It looks good to me, and further design changes can be done after review if necessary.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

Can you open a PR with the work you've done? It looks good to me, and further design changes can be done after review if necessary.

Absolutely (will push as soon as I can). I think the removal of Eq and Hash derives could be controversial, but if everything compiles, it seems fine to me, unless there's an external use case people are using it for that I've overlooked in removing it.

@marioferpa when you see the activity here, can you let us know if/how it addresses your original request?

@marioferpa
Copy link
Author

@marioferpa when you see the activity here, can you let us know if/how it addresses your original request?

Yes it does, actually it goes above and beyond, I didn't think of having a texture atlas in ther for example, and that seems super useful.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

@marioferpa cool. Note texture atlas support already exists in main, as of a few days before/after you opening this issue, and will be in Bevy 0.16.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

@eero-lehtinen if you get a chance, feel free to offer your input on the design aspects in this issue.

@eero-lehtinen
Copy link
Contributor

eero-lehtinen commented Jan 23, 2025

This is definitely convenient, but I'm not sure if it's smart because image scaling is a complicated issue. The results of arbitrary scalings are almost guaranteed to look bad. Also if people are expecting smooth results from lerping the cursor scale, that won't really happen because the OS APIs are restricted to a strict pixel grid. It can also waste a lot of memory because we cache all cursors.

So I guess it might be fine to add this for convenience, but instruct people to not rely on it. Instead preferring to author mindfully scaled source images, or just use sprites or other "software cursors" if they really want freely scalable cursors (also allows to do crazier things like rotations).

Also as a side note we should probably split a struct out of CustomCursor::Image contents because it's getting complicated and it would be nice to use ..default().

@eero-lehtinen
Copy link
Contributor

eero-lehtinen commented Jan 23, 2025

As an aside: winit doesn't allow us to pass a custom size / scale or filter when setting the cursor, so we need to do all this on the CPU before we send it to winit.

winit doesn't allow this because the underlying OS APIs don't allow this. They expect simple bitmaps and so we have to do all transformations ourselves. We are kind of abusing the APIs because they weren't designed for games. Wayland might be to only platform where the program is allowed to draw the cursor freely as a surface without clunky OS APIs.

@UkoeHB
Copy link
Contributor

UkoeHB commented Jan 23, 2025

It can also waste a lot of memory because we cache all cursors.

It should not be necessary to cache all scales of the same image. Only the most recent scale.

or just use sprites or other "software cursors" if they really want freely scalable cursors

Software cursors are a non-option for many games due to input lag.

@badgyro
Copy link

badgyro commented Jan 23, 2025

I wanted to let you know about a potential issue with custom cursors on Bevy. When using a scaled screen, like 200%, the cursor’s physical resolution doesn’t match the logical one. For instance, on a 4k screen with 200% scaling, the physical resolution is 3840x2160 px, but the logical one is 1920x1080 px. So, a cursor with a logical size of 16x16 px should have a physical resolution of 32x32 px. But when you set a custom cursor using winit, an image sized 32x32 px will still have a logical size of 32x32 px on the screen. It’ll be scaled to a physical 64x64 px, making the cursor too big and blurry.

I’ve attached a few screenshots to show what I mean. The first one is a software cursor that gives a pixel-perfect result, but it has a high input lag. On the right side is an image at 200% scale.

Image

The second screenshot is a custom winit cursor that shows incorrect scaling and blurriness.

Image

I don’t think this can be fixed on Bevy’s side until winit handles it on their end. But I’m sharing this information so that any future API is prepared for this change.

@eero-lehtinen
Copy link
Contributor

eero-lehtinen commented Jan 23, 2025

Some platforms just do their own upscaling and there is nothing winit can do. At least web does this, but I'm pretty sure other platforms don't usually do that.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

So I guess it might be fine to add this for convenience, but instruct people to not rely on it. Instead preferring to author mindfully scaled source images, or just use sprites or other "software cursors" if they really want freely scalable cursors (also allows to do crazier things like rotations).

Also as a side note we should probably split a struct out of CustomCursor::Image contents because it's getting complicated and it would be nice to use ..default().

Yep, makes sense. I have a builder here: https://github.com/mgi388/bevy-cursor-kit/blob/main/src/builder.rs but hopefully we can just use a struct inside Bevy and maybe my builder will become redundant.

This is definitely convenient, but I'm not sure if it's smart because image scaling is a complicated issue. The results of arbitrary scalings are almost guaranteed to look bad. Also if people are expecting smooth results from lerping the cursor scale, that won't really happen because the OS APIs are restricted to a strict pixel grid. It can also waste a lot of memory because we cache all cursors.

At least for my own use case, I don't want to animate the scale I just want to be able to upscale and pick a filtering option (this also means the cache isn't a problem because I'm not asking Bevy to cache N different scales for each frame). However, I also know that I can do this in a "preprocessing" step outside of Bevy similar to how I do color keying in my bevy_cursor_kit crate: https://github.com/mgi388/bevy-cursor-kit/blob/f89f509cfc2fa5ddc72862821b2dc4f08a8652c1/src/asset_image.rs#L78-L83. I had planned to add scaling to bevy_cursor_kit but then this issue was created and it made me think maybe it's worth having in Bevy proper.

I think I could go either way on whether this exists in Bevy or not. I suppose other texture things in Bevy get image scaling "out of the box" e.g. I think you can scale sprites and UI images, right? So having cursor image scaling may be uncontroversial from a parity perspective. Even if "image scaling is a complicated issue", would it be fair to say that it becomes less complicated if we consider that dyn_image.resize() and picking from 5 filter types seems to get us pretty far, or do you feel there's more to it than that?

@eero-lehtinen
Copy link
Contributor

@mgi388 If you make a PR I can approve it. I guess it's nice to have the option even if its slightly unoptimal to use automatic upscaling.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

Some platforms just do their own upscaling and there is nothing winit can do. At least web does this, but I'm pretty sure other platforms don't usually do that.

FYI I just checked and macOS does honor the system settings pointer size for custom cursors in Bevy/winit. In the video, the custom cursor shows up as large, and obviously it's blurry.

Image
Screen.Recording.2025-01-24.at.10.04.16.am.mov

Not sure there's any action here, just sharing the results of this test.

@eero-lehtinen
Copy link
Contributor

OS cursors are just janky, not much can be done.

@eero-lehtinen
Copy link
Contributor

eero-lehtinen commented Jan 23, 2025

Or there might be more complicated OS APIs available that allow specifying images for different scales, but winit doesn't support them. That likely requires the use of platform specific cursor files and this simple way of specifying pixels is not enough.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

@eero-lehtinen do you have any opinions on the hotspot question? I think it would be worth formally deciding on that question. My opinion is:

With regards to hotspots, I think it would feel more natural to just leave the source/original hotspot as it was (as it came from the cursor authoring software), and by providing scale you are asking Bevy to handle the scaling of your image as well as adjusting the hotspot for you.

flip_x and flip_y could go either way with respect to users transforming their own hotspot or not, because it's easy for them to do.

But once you add scale, I think that asking users to work out the new hotspot from looking up the sprite/frame image size and then scaling it themselves is a bit annoying.

@eero-lehtinen
Copy link
Contributor

I agree.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

Or there might be more complicated OS APIs available that allow specifying images for different scales, but winit doesn't support them. That likely requires the use of platform specific cursor files and this simple way of specifying pixels is not enough.

Yeah I started looking at the winit PRs and quickly saw the sentiment seemed to be "there's not really enough / consistent OS support here to do anything atm".

@eero-lehtinen
Copy link
Contributor

It could be pretty easy to implement at least for windows. Winit can already load icons from resources. https://docs.rs/winit/latest/x86_64-pc-windows-msvc/winit/platform/windows/trait.IconExtWindows.html#tymethod.from_resource

Though at that point the implementation would be pretty annoying because we would need to generate windows cursor resources at runtime or implement another cursor type.

@mgi388
Copy link
Contributor

mgi388 commented Jan 23, 2025

I'll push out some PRs as soon as I can:

  1. Move enum out to CustomCursorImage struct. Make CustomCursor variants CustomCursorImage/CustomCursorUrl structs #17518
  2. Add docs for flip_x and flip_y to tell users that Bevy handles the hotspot flipping for them (and I'll update the code to do that as well). Automatically transform cursor hotspot user asks to flip cursor image #17540
  3. Probably a few minor enhancements to the example.
  4. Scale PR.

That should help reviewers, and also keep the most controversial one not blocking the other improvements.

github-merge-queue bot pushed a commit that referenced this issue Jan 24, 2025
…17518)

# Objective

- Make `CustomCursor::Image` easier to work with by splitting the enum
variants off into `CustomCursorImage` and `CustomCursorUrl` structs and
deriving `Default` on those structs.
- Refs #17276.

## Testing

- Ran two examples: `cargo run --example custom_cursor_image
--features=custom_cursor` and `cargo run --example window_settings
--features=custom_cursor`
- CI.

---

## Migration Guide

The `CustomCursor` enum's variants now hold instances of
`CustomCursorImage` or `CustomCursorUrl`. Update your uses of
`CustomCursor` accordingly.
github-merge-queue bot pushed a commit that referenced this issue Jan 28, 2025
…#17540)

# Objective

- As discussed in
#17276 (comment),
we should transform the cursor's hotspot if the user is asking for the
image to be flipped.
- This becomes more important when a `scale` transform option exists.
It's harder for users to transform the hotspot themselves when using
`scale` because they'd need to look up the image to get its dimensions.
Instead, we let Bevy handle the hotspot transforms and make the
`hotspot` field the "original/source" hotspot.
- Refs #17276.

## Solution

- When the image needs to be transformed, also transform the hotspot. If
the image does not need to be transformed (i.e. fast path), no hotspot
transformation is applied.

## Testing

- Ran the example: `cargo run --example custom_cursor_image
--features=custom_cursor`.
- Add unit tests for the hotspot transform function.
- I also ran the example I have in my `bevy_cursor_kit` crate, which I
think is a good illustration of the reason for this PR.
- In the following videos, there is an arrow pointing up. The button
hover event fires as I move the mouse over it.
- When I press `Y`, the cursor flips. 
- In the first video, on `bevy@main` **before** this PR, notice how the
hotspot is wrong after flipping and no longer hovering the button. The
arrow head and hotspot are no longer synced.
- In the second video, on the branch of **this** PR, notice how the
hotspot gets flipped as soon as I press `Y` and the cursor arrow head is
in the correct position on the screen and still hovering the button.
Speaking back to the objective listed at the start: The user originally
defined the _source_ hotspot for the arrow. Later, they decide they want
to flip the cursor vertically: It's nice that Bevy can automatically
flip the _source_ hotspot for them at the same time it flips the
_source_ image.

First video (main):


https://github.com/user-attachments/assets/1955048c-2f85-4951-bfd6-f0e7cfef0cf8

Second video (this PR):


https://github.com/user-attachments/assets/73cb9095-ecb5-4bfd-af5b-9f772e92bd16
mrchantey pushed a commit to mrchantey/bevy that referenced this issue Feb 4, 2025
…evyengine#17518)

# Objective

- Make `CustomCursor::Image` easier to work with by splitting the enum
variants off into `CustomCursorImage` and `CustomCursorUrl` structs and
deriving `Default` on those structs.
- Refs bevyengine#17276.

## Testing

- Ran two examples: `cargo run --example custom_cursor_image
--features=custom_cursor` and `cargo run --example window_settings
--features=custom_cursor`
- CI.

---

## Migration Guide

The `CustomCursor` enum's variants now hold instances of
`CustomCursorImage` or `CustomCursorUrl`. Update your uses of
`CustomCursor` accordingly.
mrchantey pushed a commit to mrchantey/bevy that referenced this issue Feb 4, 2025
…bevyengine#17540)

# Objective

- As discussed in
bevyengine#17276 (comment),
we should transform the cursor's hotspot if the user is asking for the
image to be flipped.
- This becomes more important when a `scale` transform option exists.
It's harder for users to transform the hotspot themselves when using
`scale` because they'd need to look up the image to get its dimensions.
Instead, we let Bevy handle the hotspot transforms and make the
`hotspot` field the "original/source" hotspot.
- Refs bevyengine#17276.

## Solution

- When the image needs to be transformed, also transform the hotspot. If
the image does not need to be transformed (i.e. fast path), no hotspot
transformation is applied.

## Testing

- Ran the example: `cargo run --example custom_cursor_image
--features=custom_cursor`.
- Add unit tests for the hotspot transform function.
- I also ran the example I have in my `bevy_cursor_kit` crate, which I
think is a good illustration of the reason for this PR.
- In the following videos, there is an arrow pointing up. The button
hover event fires as I move the mouse over it.
- When I press `Y`, the cursor flips. 
- In the first video, on `bevy@main` **before** this PR, notice how the
hotspot is wrong after flipping and no longer hovering the button. The
arrow head and hotspot are no longer synced.
- In the second video, on the branch of **this** PR, notice how the
hotspot gets flipped as soon as I press `Y` and the cursor arrow head is
in the correct position on the screen and still hovering the button.
Speaking back to the objective listed at the start: The user originally
defined the _source_ hotspot for the arrow. Later, they decide they want
to flip the cursor vertically: It's nice that Bevy can automatically
flip the _source_ hotspot for them at the same time it flips the
_source_ image.

First video (main):


https://github.com/user-attachments/assets/1955048c-2f85-4951-bfd6-f0e7cfef0cf8

Second video (this PR):


https://github.com/user-attachments/assets/73cb9095-ecb5-4bfd-af5b-9f772e92bd16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Development

No branches or pull requests

6 participants