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

Add custom cursors #14284

Merged
merged 24 commits into from
Aug 12, 2024
Merged

Conversation

eero-lehtinen
Copy link
Contributor

@eero-lehtinen eero-lehtinen commented Jul 11, 2024

Objective

Solution

  • Change cursor type to accommodate both native and image cursors
  • I don't really like this solution because I couldn't use Handle<Image> directly. I would need to import bevy_assets and that causes a circular dependency. Alternatively we could use winit's CustomCursor smart pointers, but that seems hard because the event loop is needed to create those and is not easily accessable for users. So now I need to copy around rgba buffers which is sad.
  • I use a cache because especially on the web creating cursor images is really slow
  • Sorry to WIP custom cursors #14196 for yoinking, I just wanted to make a quick solution for myself and thought that I should probably share it too.

Update:

  • Now uses Handle<Image>, reads rgba data in bevy_render and uses resources to send the data to bevy_winit, where the final cursors are created.

Testing

  • Added example which works fine at least on Linux Wayland (winit side has been tested with all platforms).
  • I haven't tested if the url cursor works.

Migration Guide

  • CursorIcon is no longer a field in Window, but a separate component can be inserted to a window entity. It has been changed to an enum that can hold custom images in addition to system icons.
  • Cursor is renamed to CursorOptions and cursor field of Window is renamed to cursor_options
  • CursorIcon is renamed to SystemCursorIcon

@BD103 BD103 added C-Feature A new feature, making something new possible A-Windowing Platform-agnostic interface layer to run your app in 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 labels Jul 13, 2024
@Hexorg
Copy link
Contributor

Hexorg commented Jul 14, 2024

Would it be more clear to have SystemCursor and SpriteCursor?

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 14, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I really want this feature, and I'm excited to see that there's a path forward.

That said, I really dislike the custom image buffer :( That's really unpleasant to use, and hard to maintain. Can you say more about the circular dependency? What exactly is going on here: can we refactor things to get a nice API?

@eero-lehtinen
Copy link
Contributor Author

That said, I really dislike the custom image buffer :( That's really unpleasant to use, and hard to maintain. Can you say more about the circular dependency? What exactly is going on here: can we refactor things to get a nice API?

bevy_render contains Image and depends on bevy_window, so bevy_window can't depend on bevy_render and so can't use Image in the Window component.

We also can't use Handle in bevy_winit because bevy_asset depends on bevy_winit. It's only used with a little bit with android specific stuff, so it could maybe be removed. Then we could use a separate component from bevy_winit (not included in Window) to store a cursor Handle<Image>.

@eero-lehtinen
Copy link
Contributor Author

eero-lehtinen commented Jul 14, 2024

Here is a graph of bevy workspace dependencies if someone wants to think about it. It doesn't show bevy_asset depending on bevy_winit because that is only on android.

Basically it would be best if both bevy_window and bevy_winit could depend on bevy_render, but at least bevy_window is deeply used in bevy_render, so is probably impossible. Maybe just inverting the (not shown in the graph) dependency between bevy_asset and bevy_winit is enough, because it would allow bevy_winit to depend on bevy_render and bevy_asset and then could use both Handle and Image. Then we need to just live with the fact that cursors will not be included in the Window component.
asd

@Hexorg
Copy link
Contributor

Hexorg commented Jul 14, 2024

Is moving Image into bevy_asset an option?

@alice-i-cecile
Copy link
Member

I'd really prefer not to move that into bevy_asset. Could we move Image into its own crate?

For reference, the same problem is encountered with #1031: we really want to feed normal image data (loaded via the asset system) back into windowing.

@alice-i-cecile alice-i-cecile added A-Cross-Cutting Impacts the entire engine D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR and removed D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jul 14, 2024
@eero-lehtinen
Copy link
Contributor Author

eero-lehtinen commented Jul 14, 2024

Having Image in a separate crate could fix this as long as it's made to not depend on bevy_render and then we would also need to reverse/fix the android dependency.

@alice-i-cecile
Copy link
Member

Great. Can you link to the Android stuff? We should pull this out into its own issue; I think this is a pretty reasonable plan of attack.

@eero-lehtinen
Copy link
Contributor Author

eero-lehtinen commented Jul 15, 2024

let asset_manager = bevy_winit::ANDROID_APP

On android, we get the asset path from winit. It should be possible to just reverse the dependency and smuggle the paths with resources or call functions the other way. (Or I guess its a whole asset manager, so might be more difficult)

@eero-lehtinen eero-lehtinen force-pushed the custom-cursors branch 2 times, most recently from 46da546 to cbafc20 Compare July 18, 2024 17:39
@alice-i-cecile alice-i-cecile self-requested a review July 18, 2024 18:03
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Jul 18, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Jul 18, 2024
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Really great change, thank you for doing this! It would be awesome to have an example demonstrating the new feature, but this looks great as is.

crates/bevy_window/src/system_cursor.rs Show resolved Hide resolved
crates/bevy_winit/src/state.rs Outdated Show resolved Hide resolved
@tychedelia tychedelia added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 9, 2024
@eero-lehtinen
Copy link
Contributor Author

Really great change, thank you for doing this! It would be awesome to have an example demonstrating the new feature, but this looks great as is.

The window_settings example is modified to include an image cursor.

@tychedelia
Copy link
Contributor

Really great change, thank you for doing this! It would be awesome to have an example demonstrating the new feature, but this looks great as is.

The window_settings example is modified to include an image cursor.

Ah great :) I just saw the system cursors and missed the custom. Perfect!

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Aug 9, 2024
Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Great API, good work!

crates/bevy_render/src/view/window/cursor.rs Outdated Show resolved Hide resolved
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
@alice-i-cecile
Copy link
Member

For folks looking in the future, #14351 talks about the web of dependencies and why this implementation was chosen.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 12, 2024
Merged via the queue into bevyengine:main with commit 47c4e30 Aug 12, 2024
29 of 30 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Aug 15, 2024
# Objective

- CI broke after #14284 because of the `cursor_options` rename

## Solution

- Update the broken patch

## Testing

- Patch tested with `git apply`.
@alice-i-cecile alice-i-cecile added the A-UI Graphical user interfaces, styles, layouts, and widgets label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine A-UI Graphical user interfaces, styles, layouts, and widgets A-Windowing Platform-agnostic interface layer to run your app in C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add first-party asset-based cursor icons to Bevy
6 participants