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

Support texture atlases in CustomCursor::Image #17121

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

mgi388
Copy link
Contributor

@mgi388 mgi388 commented Jan 3, 2025

Objective

  • Bevy 0.15 added support for custom cursor images.
  • However, to do animated cursors using the initial support shipped in 0.15 means you'd have to animate the Handle<Image>: You can't use a TextureAtlas like you can with sprites and UI images.
  • For my use case, my cursors are spritesheets. To animate them, I'd have to break them down into multiple Image assets, but that seems less than ideal.

Solution

  • Allow users to specify a TextureAtlas field when creating a custom cursor image.
  • To create parity with Bevy's TextureAtlas support on Sprites and ImageNodes, this also allows users to specify rect, flip_x and flip_y. In fact, for my own use case, I need to flip_y.

Testing

  • I added unit tests for calculate_effective_rect and extract_and_transform_rgba_pixels.
  • I added a brand new example for custom cursor images. It has controls to toggle fields on and off. I opted to add a new example because the existing cursor example (window_settings) would be far too messy for showcasing these custom cursor features (I did start down that path but decided to stop and make a brand new example).
  • The new example uses a Kenny cursor icon sprite sheet. I included the licence even though it's not required (and it's CC0).
  • I decided to make the example just loop through all cursor icons for its animation even though it's not a realistic in-game animation sequence.
  • I ran the PNG through https://tinypng.com. Looks like it's about 35KB.
  • I'm open to adjusting the example spritesheet if required, but if it's fine as is, great.

Showcase

custom-cursor-001.mov

Migration Guide

The CustomCursor::Image enum variant has some new fields. Update your code to set them.

Before:

CustomCursor::Image {
    handle: asset_server.load("branding/icon.png"),
    hotspot: (128, 128),
}

After:

CustomCursor::Image {
    handle: asset_server.load("branding/icon.png"),
    texture_atlas: None,
    flip_x: false,
    flip_y: false,
    rect: None,
    hotspot: (128, 128),
}

References

@mgi388
Copy link
Contributor Author

mgi388 commented Jan 3, 2025

Another showcase video. This is a more realistic in-game animated cursor. The pointer animates every 5s (blink and you'll miss it):

Screen.Recording.2025-01-03.at.11.15.03.PM.mov

@LikeLakers2
Copy link
Contributor

LikeLakers2 commented Jan 3, 2025

The new example uses a Kenny cursor icon sprite sheet. I included the licence even though it's not required (and it's CC0).

First off: Thank you for attributing to Kenney. He's a cool person. :D

Second off: I don't see any assets added by this PR? All the files touched are code, except for examples/README.md and a Cargo.toml.

@IQuick143 IQuick143 added A-Windowing Platform-agnostic interface layer to run your app in X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 4, 2025
@mgi388 mgi388 force-pushed the custom-cursor-image-texture-atlas branch from c6ba500 to 2f77a85 Compare January 6, 2025 00:31
@mgi388
Copy link
Contributor Author

mgi388 commented Jan 6, 2025

@eero-lehtinen 👋 would you be interested in reviewing this PR for texture atlas support on custom cursor images?

Copy link
Contributor

@eero-lehtinen eero-lehtinen left a comment

Choose a reason for hiding this comment

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

Seems fine to me except for a couple of things.

_ => None,
};

let image_data = image_data?;
Copy link
Contributor

@eero-lehtinen eero-lehtinen Jan 6, 2025

Choose a reason for hiding this comment

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

I would like to keep a fast path when there is no transformations and just return the data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I think I can adjust this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. There are technically some cases where texture atlas might be the full image rect, but I decided to not let those follow the fast path since it's not obvious to me how to check that properly.

crates/bevy_winit/Cargo.toml Outdated Show resolved Hide resolved
@BenjaminBrienen BenjaminBrienen added S-Blocked This cannot move forward until something else changes and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 7, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jan 7, 2025
# Objective

- Allow other crates to use `TextureAtlas` and friends without needing
to depend on `bevy_sprite`.
- Specifically, this allows adding `TextureAtlas` support to custom
cursors in #17121 by allowing
`bevy_winit` to depend on `bevy_image` instead of `bevy_sprite` which is
a [non-starter].

[non-starter]:
#17121 (comment)

## Solution

- Move `TextureAtlas`, `TextureAtlasBuilder`, `TextureAtlasSources`,
`TextureAtlasLayout` and `DynamicTextureAtlasBuilder` into `bevy_image`.
- Add a new plugin to `bevy_image` named `TextureAtlasPlugin` which
allows us to register `TextureAtlas` and `TextureAtlasLayout` which was
previously done in `SpritePlugin`. Since `SpritePlugin` did the
registration previously, we just need to make it add
`TextureAtlasPlugin`.

## Testing

- CI builds it.
- I also ran multiple examples which hopefully covered any issues:

```
$ cargo run --example sprite
$ cargo run --example text
$ cargo run --example ui_texture_atlas
$ cargo run --example sprite_animation
$ cargo run --example sprite_sheet
$ cargo run --example sprite_picking
```

---

## Migration Guide

The following types have been moved from `bevy_sprite` to `bevy_image`:
`TextureAtlas`, `TextureAtlasBuilder`, `TextureAtlasSources`,
`TextureAtlasLayout` and `DynamicTextureAtlasBuilder`.

If you are using the `bevy` crate, and were importing these types
directly (e.g. before `use bevy::sprite::TextureAtlas`), be sure to
update your import paths (e.g. after `use bevy::image::TextureAtlas`)

If you are using the `bevy` prelude to import these types (e.g. `use
bevy::prelude::*`), you don't need to change anything.

If you are using the `bevy_sprite` subcrate, be sure to add `bevy_image`
as a dependency if you do not already have it, and be sure to update
your import paths.
@mgi388 mgi388 force-pushed the custom-cursor-image-texture-atlas branch from 00a36d4 to 569260c Compare January 8, 2025 02:33
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Jan 8, 2025
@mgi388
Copy link
Contributor Author

mgi388 commented Jan 8, 2025

Changes (also see latest commits):

  • Don't depend on bevy_sprite.
  • Follow fast path in the case there is no image transform to do.
  • Add custom_cursor.rs, CustomCursorPlugin and move most of the custom cursor code to it. This has the nice benefits of a) allowing us to encapsulate the registration of TextureAtlasPlugin and b) remove some #[cfg(feature = "custom_cursor")]. There's still room to move the CustomCursor type, but I wanted to reduce the diff in this PR so it's still in cursor.rs.

@BenjaminBrienen
Copy link
Contributor

I don't have time to do a full code review, but I'll run the examples when I get to my laptop

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

custom_cursor_image works. window_settings actually seems broken here and on main.

@mgi388
Copy link
Contributor Author

mgi388 commented Jan 8, 2025

custom_cursor_image works. window_settings actually seems broken here and on main.

@BenjaminBrienen how so? Seems to work for me. You need to left/right click to cycle the cursor icon if you didn't already do that.

@BenjaminBrienen
Copy link
Contributor

custom_cursor_image works. window_settings actually seems broken here and on main.

@BenjaminBrienen how so? Seems to work for me. You need to left/right click to cycle the cursor icon if you didn't already do that.

See #17227

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants