Add Hidden to CursorIcon and CursorSource enums #15386

fernanlukban · 2024-09-23T10:38:20Z

Objective

Unifies the old winit cursor api (window.cursor.visible = false) with the new old winit cursor api (CursorIcon implemented in Add custom cursors #14284)
- Adds a Hidden enum variant for CursorIcon and related CursorSource in the consuming API
Fixes Add CursorIcon::Hidden #15379

Solution

I just decided to add Hidden variant to the enum, then decided to fix the errors one by one to find the places where the match guards weren't being exhausted properly. Once I saw that it gets consumed in the update_cursors function, I decided to see how it gets used to actually change the cursor. Looking at #14284, I saw that there was an intermediary CursorSource enum that would get used to actually create the cursor. From what I understand, this enum was created becase we can either have system cursors that exist on our OS or a custom one that we define. My thought process then, was to propagate the Hidden value to this struct and then instead of setting the actual cursor source, figure out how to turn off the visibility. I looked at where the window.cursor.visibility value that the old winit api was using and then saw that we could set winit_window.set_cursor_visible(false).

The only thing I'm worried about now is that I'm wondering about if there's a performance hit of doing this way. I haven't really gotten the time to look through the code properly, but previously we were not changing the visibility of this at all. Right now, I am changing the visibility whenever we are updating the cursors. I'm thinking that since this doesn't get changed too often, changing the visibility should just be minimal impact.

The only thing I wanted to do was something along the lines of

winit_window.set_cursor_visible(final_cursor.is_some())

but I am quite noob at rust and couldn't figure out how to fight back at the borrow checker.

Also, if there are any docs that I should update please let me know.

Testing

Built on top of the Add custom cursors #14284 example that was updated, added the enum to the variant and ran, working at least for macos, will be able to test on windows when I get back home in two days

trimmed.mov

Migration Guide

This might be a breaking change depending on if users ever matched on the CursorIcon enum, though I'm not really sure if they would ever

github-actions · 2024-09-23T10:38:33Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

alice-i-cecile · 2024-09-23T14:25:37Z

I'm thinking that since this doesn't get changed too often, changing the visibility should just be minimal impact.

100% agreed :)

alice-i-cecile

Very nice! No complaints here; this is how I would have done it (including chasing errors after adding the variant).

tychedelia

This conflicts with the existing CursorOptions field on Window that sets the visibility in changed_windows. I'm okay with considering the more ECS focused approached, but we should minimally remove visible from CursorOptions, as well as documenting the platform specific gotchas on the enum here, as well as explore adding a CursorGrabMode component and I guess HitTest too? Idk, now I'm also like that's too ECS-y lol, but minimally we need to resolve what to do with the existing window settings.

eero-lehtinen · 2024-09-23T18:28:17Z

With this design it becomes harder to toggle visibility. You would have to store the previous cursor somewhere to unhide it later. But it might not matter too much.

I think this option would fit better as a boolean field within the cursor component, but that unfortunately breaks change detection. Could we use the same diffing system that we use for the window settings, where we store a copy of the previous window component and that way get ergonomics of a single unified component and field level change detection?

fernanlukban · 2024-09-23T21:02:33Z

I kind of get the gist of it and agree with the ergonomics. Do you have a pointer of the window settings thing you're referencing?

tychedelia · 2024-09-23T21:04:26Z

I kind of get the gist of it and agree with the ergonomics. Do you have a pointer of the window settings thing you're referencing?

Look at the CursorOptions struct in window.rs.

…indow::Window` component As per talks in https://discord.com/channels/691052431525675048/1287953140955746305, just going to remove visible so that we only have one API for making a cursor hidden.

…amples

Vrixyz · 2024-09-24T10:53:15Z

crates/bevy_window/src/window.rs

@@ -126,7 +126,7 @@ impl NormalizedWindowRef {
 )]
 #[reflect(Component, Default, Debug)]
 pub struct Window {
-    /// The cursor options of this window. Cursor icons are set with the `Cursor` component on the
+    /// The cursor options of this window. Cursor icons are set with the `CursorIcon` component on the


👍 ; I think that's important to note to ease with migration, that being said this property actually refers to bevy_render, and it's annoying that we can't link to it through a real link :( (also, should we at all ? 🤔)

Or should CursorIcon be brought in bevy_window ?

(Those questions are probably outside this PR scope, sorry for derailing :) )

no worries! we have had a whole conversation about it inside a discord thread

tl;dr ideally this would live inside of bevy_winit, to keep ergonomics the same since most of the use cases regarding icons right now involve Query<&Window> and reading the cursor_options field which is of the CursorOptions type, which originally had CursorIcon. Now that we have custom cursor support, this had to live inside bevy_render because the custom cursors require access to Image which can't be imported inside of bevy_winit nor bevy_window due to circular dependencies.

though I do agree that the comment should be linked properly - I tried adding the fully qualified name on another comment, but I wasn't sure about how to run the docs to check if it worked locally (i'm still a rust toolchain noob)

thanks for the insight !

I tried adding the fully qualified name on another comment

Nice try! the CI docs step fails due to that. Check it out (and I recommend looking at the .github workflow file to see more details on what commands are run 👍

I fear it's impossible to link because bevy_window doesn't rely on bevy_render :(

well I imagine we theoretically could by adding the dependency if building docs, but I personally wouldn't approve that :/

Continued rambling about CursorIcon

I'd prefer a CursorIcon::Custom empty enum variant, and let the other crates handle their custom thing however they want 🤔.

Or a generic Custom ? or a Box with a dyn trait something inside ?

Or straight up remove any mention to those classes and embrace this split 🤔

I wasn't sure about how to run the docs

cargo doc --open will bring you far enough 🤞

eero-lehtinen

I'm certainly in favor of moving cursor related options to the cursor component, but it feels weird losing the ability that winit provides to separately toggle visibility and choose the displayed icon. I don't have super strong feelings about though, this way is ok too.

The change to automatically add the cursor component to every window is good. I should have done it that way before. Though I think observers would work better in avoiding scheduling issues.

eero-lehtinen · 2024-09-24T12:04:18Z

crates/bevy_winit/src/state.rs

@@ -149,6 +149,8 @@ pub struct CustomCursorCache(pub HashMap<CustomCursorCacheKey, winit::window::Cu
 /// A source for a cursor. Is created in `bevy_render` and consumed by the winit event loop.
 #[derive(Debug)]
 pub enum CursorSource {
+    /// A hidden cursor, used to hide the cursor in `winit_window`
+    Hidden,


I don't think we should propagate cursor visibility to bevy_winit. Custom cursors are required to be changed in such a convoluted way because they require access to the winit event loop. Cursor visibility can be changed directly with NonSendMut<WinitWindows> in any system just like it was done before with cursor_options.

I'm starting to think what we really need to do is do something along the lines of not having a cursor from being instead Hidden to just not having a cursor source or None. This clearly denotes the feature set each cursor source has - the SystemCursor is heavily part of the winit package we use, hence the related feature set. Now that we have CursorSource::Custom, if we think that over time it ends up having a different feature set, maybe something like if we were to support gifs as cursors, or sprite sheets as cursors with animations, there might be options relating how fast to play the gif or whether or not to loop it's animation. This way, most of the examples will look almost the same in functionality, just changing a bool

pub enum CursorSource { None, Custom{ custom_cursor: CustomCursor, visibility: bool }, System{ system_cursor: SystemCursor, visibility: bool }, }

Overtime, I want to probably work on moving the system related CursorOptions found in bevy_window::Window and moving them with the CursorIcon.

I would ideally like to have these all live in the same crate so maybe looking at breaking the circular dependencies next is the thing to do.

As to the

Cursor visibility can be changed directly with NonSendMut<WinitWindows> in any system just like it was done before with cursor_options.

I don't get quite this means? Are you suggesting that if it isn't a CustomCursor then we should just use the system winit related API directly? I think we were initially against that because we would have two API's and it could get confusing, so I would like your take on it.

What I would be proposing is to just move cursor options from the window component to the cursor component and doing absolutely everything the exact same it was done before and make a new cursor component.

struct Cursor { pub visible: bool, ... other options ... pub icon: CursorIcon, }

Then move everything that was done in the window update system to the cursor update system.

But I that is a different proposal than what is included in the related issue, so it might not be relevant to this PR.

I don't get quite this means? Are you suggesting that if it isn't a CustomCursor then we should just use the system winit related API directly? I think we were initially against that because we would have two API's and it could get confusing, so I would like your take on it.

Yes. CursorSource is an implementation detail for smuggling cursor icon bytes to the bevy_winit crate. It would lead to less spread out code and less complexity to just set the WinitWindows cursor visibility boolean in the bevy_render cursor system. It doesn't create any additional APIs. Functionally your code does the same thing so consider what would be simpler.

Now that we have CursorSource::Custom, if we think that over time it ends up having a different feature set.

CursorSource is a direct equivalent to winits native cursors. Those are clearly defined and not flexible, so CursorSource doesn't need to be flexible.

E.g. if we want to use sprite sheets as animated cursors, those would need to be converted to be winit compatible outside of bevy_winit anyway because in can't use assets.

fernanlukban · 2024-09-24T23:01:20Z

crates/bevy_render/src/view/window/cursor.rs

@@ -29,7 +28,7 @@ impl Plugin for CursorPlugin {
        app.register_type::<CursorIcon>()
            .init_resource::<CustomCursorCache>()
            .add_systems(Last, update_cursors)
-            .add_systems(Update, add_cursor_to_windows);
+            .observe(add_cursor_to_windows);


@eero-lehtinen this was quite pretty to do, I remember reading the example for this last week and now this seems so nice

fernanlukban · 2024-09-24T23:01:46Z

examples/input/mouse_grab.rs

@@ -19,9 +19,7 @@ fn grab_mouse(
    // There are cases where we can have multiple windows and cursors,
    // but not in this example. Keeping the naming convention since it
    // exists in other examples
-    let Ok((mut window, mut cursor)) = windows_and_cursors.get_single_mut() else {


it ended up fixing the non-determinism thing here so that we will always have the cursor when windows are created which is perfect

tychedelia · 2024-09-25T01:56:20Z

crates/bevy_winit/src/state.rs

@@ -154,6 +154,8 @@ pub struct CustomCursorCache(pub HashMap<CustomCursorCacheKey, winit::window::Cu
 /// A source for a cursor. Is created in `bevy_render` and consumed by the winit event loop.
 #[derive(Debug)]
 pub enum CursorSource {
+    /// A hidden cursor, used to hide the cursor in `winit_window`
+    Hidden,


This is still missing documentation about platform specific behavior.

Add Hidden to CursorIcon and CursorSource enums

13d73fb

Fixed failing doctest and updated some comments

7cd2a57

alice-i-cecile requested review from tychedelia and janhohenheim September 23, 2024 14:24

alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 23, 2024

alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Sep 23, 2024

alice-i-cecile approved these changes Sep 23, 2024

View reviewed changes

alice-i-cecile added this to the 0.15 milestone Sep 23, 2024

alice-i-cecile requested a review from viridia September 23, 2024 14:26

tychedelia suggested changes Sep 23, 2024

View reviewed changes

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 Sep 23, 2024

viridia approved these changes Sep 23, 2024

View reviewed changes

alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Sep 23, 2024

Fernan Lukban added 3 commits September 24, 2024 00:27

Removed visible from CursorOptions struct used in `bevy_window::w…

bc9e1a3

…indow::Window` component As per talks in https://discord.com/channels/691052431525675048/1287953140955746305, just going to remove visible so that we only have one API for making a cursor hidden.

Fix examples

b0e7ce4

Add system to create CursorIcon on Windowcreated event and fix ex…

6d7b8c8

…amples

fernanlukban force-pushed the cursor_icon_hidden branch from 1d73022 to 7cd2a57 Compare September 24, 2024 10:13

fernanlukban and others added 2 commits September 24, 2024 03:15

Merge branch 'main' into cursor_icon_hidden

9217457

format

905d75e

Vrixyz reviewed Sep 24, 2024

View reviewed changes

eero-lehtinen reviewed Sep 24, 2024

View reviewed changes

Fixed more typos and docs

9b9e1cd

Use observers to trigger cursor icon insertion on window creation

b9ed614

fernanlukban commented Sep 24, 2024

View reviewed changes

Merge branch 'main' into cursor_icon_hidden

b48136e

viridia approved these changes Sep 24, 2024

View reviewed changes

tychedelia reviewed Sep 25, 2024

View reviewed changes

alice-i-cecile removed this from the 0.15 milestone Oct 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Hidden to CursorIcon and CursorSource enums #15386

Add Hidden to CursorIcon and CursorSource enums #15386

fernanlukban commented Sep 23, 2024 •

edited

Loading

github-actions bot commented Sep 23, 2024

alice-i-cecile commented Sep 23, 2024

alice-i-cecile left a comment

tychedelia left a comment

eero-lehtinen commented Sep 23, 2024

fernanlukban commented Sep 23, 2024

tychedelia commented Sep 23, 2024

Vrixyz Sep 24, 2024 •

edited

Loading

fernanlukban Sep 24, 2024

Vrixyz Sep 24, 2024 •

edited

Loading

eero-lehtinen left a comment •

edited

Loading

eero-lehtinen Sep 24, 2024

fernanlukban Sep 24, 2024 •

edited

Loading

fernanlukban Sep 24, 2024

eero-lehtinen Sep 25, 2024 •

edited

Loading

eero-lehtinen Sep 25, 2024 •

edited

Loading

fernanlukban Sep 24, 2024

fernanlukban Sep 24, 2024

tychedelia Sep 25, 2024

Add Hidden to CursorIcon and CursorSource enums #15386

Are you sure you want to change the base?

Add Hidden to CursorIcon and CursorSource enums #15386

Conversation

fernanlukban commented Sep 23, 2024 • edited Loading

Objective

Solution

Testing

Migration Guide

github-actions bot commented Sep 23, 2024

alice-i-cecile commented Sep 23, 2024

alice-i-cecile left a comment

Choose a reason for hiding this comment

tychedelia left a comment

Choose a reason for hiding this comment

eero-lehtinen commented Sep 23, 2024

fernanlukban commented Sep 23, 2024

tychedelia commented Sep 23, 2024

Vrixyz Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

fernanlukban Sep 24, 2024

Choose a reason for hiding this comment

Vrixyz Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

eero-lehtinen left a comment • edited Loading

Choose a reason for hiding this comment

eero-lehtinen Sep 24, 2024

Choose a reason for hiding this comment

fernanlukban Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

fernanlukban Sep 24, 2024

Choose a reason for hiding this comment

eero-lehtinen Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

eero-lehtinen Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

fernanlukban Sep 24, 2024

Choose a reason for hiding this comment

fernanlukban Sep 24, 2024

Choose a reason for hiding this comment

tychedelia Sep 25, 2024

Choose a reason for hiding this comment

fernanlukban commented Sep 23, 2024 •

edited

Loading

Vrixyz Sep 24, 2024 •

edited

Loading

Vrixyz Sep 24, 2024 •

edited

Loading

eero-lehtinen left a comment •

edited

Loading

fernanlukban Sep 24, 2024 •

edited

Loading

eero-lehtinen Sep 25, 2024 •

edited

Loading

eero-lehtinen Sep 25, 2024 •

edited

Loading