Skip to content

Add Hidden to CursorIcon and CursorSource enums #15386

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
38 changes: 36 additions & 2 deletions crates/bevy_render/src/view/window/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use bevy_ecs::{
};
use bevy_reflect::{std_traits::ReflectDefault, Reflect};
use bevy_utils::{tracing::warn, HashSet};
use bevy_window::{SystemCursorIcon, Window};
use bevy_window::{SystemCursorIcon, Window, WindowCreated};
use bevy_winit::{
convert_system_cursor_icon, CursorSource, CustomCursorCache, CustomCursorCacheKey,
PendingCursor,
Expand All @@ -27,7 +27,8 @@ impl Plugin for CursorPlugin {
fn build(&self, app: &mut App) {
app.register_type::<CursorIcon>()
.init_resource::<CustomCursorCache>()
.add_systems(Last, update_cursors);
.add_systems(Last, update_cursors)
.observe(add_cursor_to_windows);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


app.observe(on_remove_cursor_icon);
}
Expand All @@ -37,6 +38,28 @@ impl Plugin for CursorPlugin {
#[derive(Component, Debug, Clone, Reflect, PartialEq, Eq)]
#[reflect(Component, Debug, Default, PartialEq)]
pub enum CursorIcon {
/// Makes the cursor hidden
///
/// ## Platform-specific
///
/// - **`Windows`**, **`X11`**, and **`Wayland`**: The cursor is hidden only when inside the window.
/// To stop the cursor from leaving the window, change the [`bevy_window::Window`]
/// entity's
/// [`bevy_window::CursorOptions`] [`bevy_window::CursorGrabMode`] to
/// [`bevy_window::CursorGrabMode::Locked`] or
/// [`bevy_window::CursorGrabMode::Confined`]
/// - **`macOS`**: The cursor is hidden only when the window is focused.
/// - **`iOS`** and **`Android`** do not have cursors
///
/// # Note for developers
///
/// This was originally part of the [`bevy_window::CursorOptions`] component
/// as the `visible` field but it was moved out after adding Custom cursors. Before,
/// since we only had system cursors provided by the OS, this was heavily tied into
/// the implementation provided by [`bevy_winit`]. Now, you can think of this as part
/// of the `bevy_render` level, hence the drift from the original [`bevy_winit`]
/// coupled API.
Hidden,
/// Custom cursor image.
Custom(CustomCursor),
/// System provided cursor icon.
Expand Down Expand Up @@ -98,6 +121,7 @@ pub fn update_cursors(
}

let cursor_source = match cursor.as_ref() {
CursorIcon::Hidden => CursorSource::Hidden,
CursorIcon::Custom(CustomCursor::Image { handle, hotspot }) => {
let cache_key = match handle.id() {
AssetId::Index { index, .. } => {
Expand Down Expand Up @@ -196,3 +220,13 @@ fn image_to_rgba_pixels(image: &Image) -> Option<Vec<u8>> {
_ => None,
}
}

/// We need to add this here because we can't do it inside of [`bevy_winit`] due to issues
/// with circular dependencies. Before we had custom cursors, these cursor related options
/// would just be created automatically with the [`Window`] component, now however, this
/// has to be made with the creation of the window so that users can set the visibility
/// of it after the window is created.
fn add_cursor_to_windows(ev_window_created: Trigger<WindowCreated>, mut commands: Commands) {
let WindowCreated { window: entity } = ev_window_created.event();
commands.entity(*entity).insert(CursorIcon::default());
}
14 changes: 1 addition & 13 deletions crates/bevy_window/src/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +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
/// window entity.
/// The cursor options of this window. See `bevy_render` on how to change the cursor to a custom or system one.
pub cursor_options: CursorOptions,
/// What presentation mode to give the window.
pub present_mode: PresentMode,
Expand Down Expand Up @@ -549,16 +548,6 @@ impl WindowResizeConstraints {
)]
#[reflect(Debug, Default)]
pub struct CursorOptions {
/// Whether the cursor is visible or not.
///
/// ## Platform-specific
///
/// - **`Windows`**, **`X11`**, and **`Wayland`**: The cursor is hidden only when inside the window.
/// To stop the cursor from leaving the window, change [`CursorOptions::grab_mode`] to [`CursorGrabMode::Locked`] or [`CursorGrabMode::Confined`]
/// - **`macOS`**: The cursor is hidden only when the window is focused.
/// - **`iOS`** and **`Android`** do not have cursors
pub visible: bool,

/// Whether or not the cursor is locked by or confined within the window.
///
/// ## Platform-specific
Expand All @@ -581,7 +570,6 @@ pub struct CursorOptions {
impl Default for CursorOptions {
fn default() -> Self {
CursorOptions {
visible: true,
grab_mode: CursorGrabMode::None,
hit_test: true,
}
Expand Down
18 changes: 13 additions & 5 deletions crates/bevy_winit/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@fernanlukban fernanlukban Sep 24, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@eero-lehtinen eero-lehtinen Sep 25, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@eero-lehtinen eero-lehtinen Sep 25, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

This is still missing documentation about platform specific behavior.

/// A custom cursor was identified to be cached, no reason to recreate it.
CustomCached(CustomCursorCacheKey),
/// A custom cursor was not cached, so it needs to be created by the winit event loop.
Expand Down Expand Up @@ -800,22 +802,28 @@ impl<T: Event> WinitAppRunnerState<T> {
continue;
};

let final_cursor: winit::window::Cursor = match pending_cursor {
let final_cursor: Option<winit::window::Cursor> = match pending_cursor {
CursorSource::Hidden => None,
CursorSource::CustomCached(cache_key) => {
let Some(cached_cursor) = cursor_cache.0.get(&cache_key) else {
error!("Cursor should have been cached, but was not found");
continue;
};
cached_cursor.clone().into()
Some(cached_cursor.clone().into())
}
CursorSource::Custom((cache_key, cursor)) => {
let custom_cursor = event_loop.create_custom_cursor(cursor);
cursor_cache.0.insert(cache_key, custom_cursor.clone());
custom_cursor.into()
Some(custom_cursor.into())
}
CursorSource::System(system_cursor) => system_cursor.into(),
CursorSource::System(system_cursor) => Some(system_cursor.into()),
};
winit_window.set_cursor(final_cursor);
if let Some(final_cursor) = final_cursor {
winit_window.set_cursor_visible(true);
winit_window.set_cursor(final_cursor);
} else {
winit_window.set_cursor_visible(false);
}
}
}
}
Expand Down
5 changes: 1 addition & 4 deletions crates/bevy_winit/src/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ pub fn create_windows<F: QueryFilter + 'static>(
}

window_created_events.send(WindowCreated { window: entity });
commands.trigger(WindowCreated { window: entity });
}
}

Expand Down Expand Up @@ -367,10 +368,6 @@ pub(crate) fn changed_windows(
crate::winit_windows::attempt_grab(winit_window, window.cursor_options.grab_mode);
}

if window.cursor_options.visible != cache.window.cursor_options.visible {
winit_window.set_cursor_visible(window.cursor_options.visible);
}

if window.cursor_options.hit_test != cache.window.cursor_options.hit_test {
if let Err(err) = winit_window.set_cursor_hittest(window.cursor_options.hit_test) {
window.cursor_options.hit_test = cache.window.cursor_options.hit_test;
Expand Down
2 changes: 0 additions & 2 deletions crates/bevy_winit/src/winit_windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,6 @@ impl WinitWindows {
attempt_grab(&winit_window, window.cursor_options.grab_mode);
}

winit_window.set_cursor_visible(window.cursor_options.visible);

// Do not set the cursor hittest on window creation if it's false, as it will always fail on
// some platforms and log an unfixable warning.
if !window.cursor_options.hit_test {
Expand Down
19 changes: 9 additions & 10 deletions examples/helpers/camera_controller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
//! - Copy the code for the [`CameraControllerPlugin`] and add the plugin to your App.
//! - Attach the [`CameraController`] component to an entity with a [`Camera3dBundle`].

use bevy::{
input::mouse::{AccumulatedMouseMotion, AccumulatedMouseScroll, MouseScrollUnit},
prelude::*,
window::CursorGrabMode,
};
use bevy::input::mouse::{AccumulatedMouseMotion, AccumulatedMouseScroll, MouseScrollUnit};
use bevy::prelude::*;
use bevy::render::view::cursor::CursorIcon;
use bevy::window::CursorGrabMode;
use std::{f32::consts::*, fmt};

pub struct CameraControllerPlugin;
Expand Down Expand Up @@ -102,7 +101,7 @@ Freecam Controls:
#[allow(clippy::too_many_arguments)]
fn run_camera_controller(
time: Res<Time>,
mut windows: Query<&mut Window>,
mut windows_and_cursors: Query<(&mut Window, &mut CursorIcon)>,
accumulated_mouse_motion: Res<AccumulatedMouseMotion>,
accumulated_mouse_scroll: Res<AccumulatedMouseScroll>,
mouse_button_input: Res<ButtonInput<MouseButton>>,
Expand Down Expand Up @@ -195,18 +194,18 @@ fn run_camera_controller(
// Handle cursor grab
if cursor_grab_change {
if cursor_grab {
for mut window in &mut windows {
for (mut window, mut cursor_icon) in &mut windows_and_cursors {
if !window.focused {
continue;
}

window.cursor_options.grab_mode = CursorGrabMode::Locked;
window.cursor_options.visible = false;
*cursor_icon = CursorIcon::Hidden;
}
} else {
for mut window in &mut windows {
for (mut window, mut cursor_icon) in &mut windows_and_cursors {
window.cursor_options.grab_mode = CursorGrabMode::None;
window.cursor_options.visible = true;
*cursor_icon = CursorIcon::default();
}
}
}
Expand Down
13 changes: 8 additions & 5 deletions examples/input/mouse_grab.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Demonstrates how to grab and hide the mouse cursor.

use bevy::{prelude::*, window::CursorGrabMode};
use bevy::{prelude::*, render::view::cursor::CursorIcon, window::CursorGrabMode};

fn main() {
App::new()
Expand All @@ -12,19 +12,22 @@ fn main() {
// This system grabs the mouse when the left mouse button is pressed
// and releases it when the escape key is pressed
fn grab_mouse(
mut windows: Query<&mut Window>,
mut windows_and_cursors: Query<(&mut Window, &mut CursorIcon)>,
mouse: Res<ButtonInput<MouseButton>>,
key: Res<ButtonInput<KeyCode>>,
) {
let mut window = windows.single_mut();
// 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 (mut window, mut cursor) = windows_and_cursors.single_mut();

if mouse.just_pressed(MouseButton::Left) {
window.cursor_options.visible = false;
*cursor = CursorIcon::Hidden;
window.cursor_options.grab_mode = CursorGrabMode::Locked;
}

if key.just_pressed(KeyCode::Escape) {
window.cursor_options.visible = true;
*cursor = CursorIcon::default();
window.cursor_options.grab_mode = CursorGrabMode::None;
}
}
19 changes: 15 additions & 4 deletions examples/window/window_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,21 @@ fn change_title(mut windows: Query<&mut Window>, time: Res<Time>) {
);
}

fn toggle_cursor(mut windows: Query<&mut Window>, input: Res<ButtonInput<KeyCode>>) {
fn toggle_cursor(
mut windows: Query<(&mut Window, &mut CursorIcon)>,
input: Res<ButtonInput<KeyCode>>,
) {
if input.just_pressed(KeyCode::Space) {
let mut window = windows.single_mut();

window.cursor_options.visible = !window.cursor_options.visible;
let (mut window, mut cursor_icon) = windows.single_mut();

// Usually cursors will get read from some sort of resource or a map
// right now we just want to toggle the cursor, causing us to have to
// recreate the `CursorIcon` enum for the entity, see below in
// `cycle_cursor_icon` for an example on how to do this.
*cursor_icon = match *cursor_icon {
CursorIcon::Hidden => CursorIcon::default(),
_ => CursorIcon::Hidden,
};
window.cursor_options.grab_mode = match window.cursor_options.grab_mode {
CursorGrabMode::None => CursorGrabMode::Locked,
CursorGrabMode::Locked | CursorGrabMode::Confined => CursorGrabMode::None,
Expand All @@ -166,6 +176,7 @@ struct CursorIcons(Vec<CursorIcon>);

fn init_cursor_icons(mut commands: Commands, asset_server: Res<AssetServer>) {
commands.insert_resource(CursorIcons(vec![
CursorIcon::Hidden,
SystemCursorIcon::Default.into(),
SystemCursorIcon::Pointer.into(),
SystemCursorIcon::Wait.into(),
Expand Down