Skip to content

Split CursorOptions off of Window#19668

Merged
alice-i-cecile merged 16 commits into
bevyengine:mainfrom
janhohenheim:split-window
Jun 17, 2025
Merged

Split CursorOptions off of Window#19668
alice-i-cecile merged 16 commits into
bevyengine:mainfrom
janhohenheim:split-window

Conversation

@janhohenheim
Copy link
Copy Markdown
Member

@janhohenheim janhohenheim commented Jun 16, 2025 •
edited
Loading

Objective

Solution

  • Split Window into multiple components, each with working change detection
  • Disable caching of the cursor grab mode
    • This will only attempt to force the grab mode when the CursorOptions were touched by the user, which is much rarer than simply moving the mouse.
  • If this PR is merged, I'll do the exact same for the other constituents of Window as a follow-up

Testing

  • Ran all the changed examples

@janhohenheim janhohenheim added A-Input Player input via keyboard, mouse, gamepad, and more M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 16, 2025
@janhohenheim janhohenheim added A-Windowing Platform-agnostic interface layer to run your app in and removed A-Input Player input via keyboard, mouse, gamepad, and more labels Jun 16, 2025
@alice-i-cecile alice-i-cecile added the A-Input Player input via keyboard, mouse, gamepad, and more label Jun 16, 2025
alice-i-cecile
alice-i-cecile reviewed Jun 16, 2025
View reviewed changes
Copy link
Copy Markdown
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 don't have the bandwidth to immediately review this in detail, but I like this direction. Required components are great here, and working change detection is very desirable!

janhohenheim
janhohenheim commented Jun 16, 2025
View reviewed changes
Comment thread crates/bevy_window/src/lib.rs
/// Defaults to `Some(CursorOptions::default())`.
///
/// Has no effect if [`WindowPlugin::primary_window`] is `None`.
pub primary_cursor_options: Option<CursorOptions>,
Copy link
Copy Markdown
Member Author

@janhohenheim janhohenheim Jun 16, 2025

Choose a reason for hiding this comment

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

I left this in for parity with the existing API, but since it's a required component, we could also omit it and let the user override it in an observer for OnAdd, PrimaryWindow

janhohenheim
janhohenheim commented Jun 16, 2025
View reviewed changes
Comment thread crates/bevy_winit/src/system.rs
pub window: Window,
}
#[derive(Debug, Clone, Component, Deref, DerefMut)]
pub(crate) struct CachedWindow(Window);
Copy link
Copy Markdown
Member Author

@janhohenheim janhohenheim Jun 16, 2025

Choose a reason for hiding this comment

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

Making this pub(crate) is not breaking, it was already de facto on that visibility.

@ickshonpe ickshonpe self-requested a review June 16, 2025 11:12
ickshonpe
ickshonpe approved these changes Jun 16, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

Looks good. The changes make sense and I didn't see any problems with any of the examples I tried.

Comment thread crates/bevy_window/src/lib.rs
/// Defaults to `Some(CursorOptions::default())`.
///
/// Has no effect if [`WindowPlugin::primary_window`] is `None`.
pub primary_cursor_options: Option<CursorOptions>,
Copy link
Copy Markdown
Contributor

@ickshonpe ickshonpe Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's a bit tangential but I don't really like how in the window module we use "cursor" to refer to the mouse pointer and in the UI and picking modules we use "pointer". I'd prefer using "pointer" consistently.

Copy link
Copy Markdown
Member Author

@janhohenheim janhohenheim Jun 16, 2025

Choose a reason for hiding this comment

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

That's a very good point. I'll open an issue for unifying terminology later :)

Copy link
Copy Markdown
Member Author

@janhohenheim janhohenheim Jun 17, 2025

Choose a reason for hiding this comment

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

Done and already resolved: #19688 (comment)

@janhohenheim janhohenheim mentioned this pull request Jun 17, 2025
Closed
IceSentry
IceSentry approved these changes Jun 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I don't know how I feel about breaking up the window component, but if we are moving in this direction this PR is good.

@IceSentry IceSentry 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 Jun 17, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 17, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 17, 2025
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Jun 17, 2025

@janhohenheim ping me when you're ready to try re-merging this.

@janhohenheim
Copy link
Copy Markdown
Member Author

janhohenheim commented Jun 17, 2025

@alice-i-cecile now :)

@alice-i-cecile alice-i-cecile enabled auto-merge June 17, 2025 20:03
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 17, 2025
Merged via the queue into bevyengine:main with commit a750cfe Jun 17, 2025
32 checks passed
@alice-i-cecile alice-i-cecile mentioned this pull request Oct 14, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile Awaiting requested review from alice-i-cecile

2 more reviewers

@IceSentry IceSentry IceSentry approved these changes

@ickshonpe ickshonpe ickshonpe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Input Player input via keyboard, mouse, gamepad, and more A-Windowing Platform-agnostic interface layer to run your app in D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bevy caches invalid cursor settings

4 participants

@janhohenheim @alice-i-cecile @IceSentry @ickshonpe