Skip to content

Split PointerId into distinct id and PointerKind fields #17808

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.

Sign up for GitHub

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 2 commits into
base: main
Choose a base branch
from
+59 −29
Open

Split PointerId into distinct id and PointerKind fields #17808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8841236
Refactor PointerId to store the id and Kind in distinct fields
alice-i-cecile Feb 11, 2025
921fd54
Use the Virtual variant for mocked directional navigation
alice-i-cecile Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/bevy_picking/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ bevy_platform_support = { path = "../bevy_platform_support", version = "0.16.0-d

# other
crossbeam-channel = { version = "0.5", optional = true }
uuid = { version = "1.13.1", features = ["v4"] }
tracing = { version = "0.1", default-features = false, features = ["std"] }

[target.'cfg(target_arch = "wasm32")'.dependencies]
Expand Down
15 changes: 9 additions & 6 deletions crates/bevy_picking/src/input.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use bevy_window::{PrimaryWindow, WindowEvent, WindowRef};
use tracing::debug;

use crate::pointer::{
Location, PointerAction, PointerButton, PointerId, PointerInput, PointerLocation,
Location, PointerAction, PointerButton, PointerId, PointerInput, PointerKind, PointerLocation,
};

use crate::PickSet;
Expand Down Expand Up @@ -99,7 +99,7 @@ impl Plugin for PointerInputPlugin {

/// Spawns the default mouse pointer.
pub fn spawn_mouse_pointer(mut commands: Commands) {
commands.spawn(PointerId::Mouse);
commands.spawn(PointerId::MOUSE);
}

/// Sends mouse pointer events to be processed by the core plugin
Expand All @@ -126,7 +126,7 @@ pub fn mouse_pick_events(
position: event.position,
};
pointer_events.send(PointerInput::new(
PointerId::Mouse,
PointerId::MOUSE,
location,
PointerAction::Move {
delta: event.position - *cursor_last,
Expand Down Expand Up @@ -155,7 +155,7 @@ pub fn mouse_pick_events(
ButtonState::Pressed => PointerAction::Press(button),
ButtonState::Released => PointerAction::Release(button),
};
pointer_events.send(PointerInput::new(PointerId::Mouse, location, action));
pointer_events.send(PointerInput::new(PointerId::MOUSE, location, action));
}
WindowEvent::MouseWheel(event) => {
let MouseWheel { unit, x, y, window } = *event;
Expand All @@ -172,7 +172,7 @@ pub fn mouse_pick_events(

let action = PointerAction::Scroll { x, y, unit };

pointer_events.send(PointerInput::new(PointerId::Mouse, location, action));
pointer_events.send(PointerInput::new(PointerId::MOUSE, location, action));
}
_ => {}
}
Expand All @@ -192,7 +192,10 @@ pub fn touch_pick_events(
) {
for window_event in window_events.read() {
if let WindowEvent::TouchInput(touch) = window_event {
let pointer = PointerId::Touch(touch.id);
let pointer = PointerId {
id: touch.id,
kind: PointerKind::Touch,
};
let location = Location {
target: match RenderTarget::Window(WindowRef::Entity(touch.window))
.normalize(primary_window.get_single().ok())
Expand Down
64 changes: 45 additions & 19 deletions crates/bevy_picking/src/pointer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ use bevy_reflect::prelude::*;
use bevy_render::camera::{Camera, NormalizedRenderTarget};
use bevy_window::PrimaryWindow;

use uuid::Uuid;

use core::{fmt::Debug, ops::Deref};

use crate::backend::HitData;
Expand All @@ -29,41 +27,69 @@ use crate::backend::HitData;
#[derive(Debug, Default, Clone, Copy, Eq, PartialEq, Hash, Component, Reflect)]
#[require(PointerLocation, PointerPress, PointerInteraction)]
#[reflect(Component, Default, Debug, Hash, PartialEq)]
pub enum PointerId {
/// The mouse pointer.
#[default]
Mouse,
/// A touch input, usually numbered by window touch events from `winit`.
Touch(u64),
/// A custom, uniquely identified pointer. Useful for mocking inputs or implementing a software
/// controlled cursor.
#[reflect(ignore)]
Custom(Uuid),
pub struct PointerId {
Copy link
Member

Choose a reason for hiding this comment

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

Given that PointerId is now a common "thing" maybe it should just be PointerId(u64) and PointerKind would just be stored adjacent to it in places where that is relevant (ex: as a separate Component)? To distinguish between two pointers, don't we only need to compare / hash the ID, not the kind?

Copy link
Member

@cart cart Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If it is not a common thing, then I think we should move back to a typed-wrapper to protect against comparing "unlike" ids across types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now (and before this PR), the IDs are only meaningfully used by touches. In theory you could encode information for Virtual / Custom types in the u64 / UUID, but I'm not sure what you would use that for.

In either case, in order for two PointerId to match, both the kind and the number associated with it must match. As a result, I don't think we should split these up, but I'm fine to go back to an enum design here. Which do you prefer?

  1. The current design, with split Id + kind.
  2. enum PointerId, only Touch gets a u64.
  3. enum PointerId, each kind has a u64.

I think I prefer solution 2, but went with solution 1 to ease migration.

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer solution (2). I think thats the move.

/// A stable identifier for a specific pointer.
///
/// This is typically provided by winit,
/// and is unique for each touch input or mouse.
Comment on lines +31 to +34
Copy link
Contributor

@ickshonpe ickshonpe Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This feels misleading. The id here corresponds to a specific touch on the device and it's only guaranteed to be unique for the duration of that touch I think.

pub id: u64,
/// The type of hardware device that created this pointer.
pub kind: PointerKind,
}

impl PointerId {
/// The [`PointerId`] of the mouse.
///
/// Currently, only one mouse pointer is supported.
pub const MOUSE: Self = Self {
id: 0,
kind: PointerKind::Mouse,
};

/// Returns true if the pointer is a touch input.
pub fn is_touch(&self) -> bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

I've left these utility methods alone to ease migration, but I'm not convinced they're particularly valuable now.

matches!(self, PointerId::Touch(_))
matches!(self.kind, PointerKind::Touch)
}
/// Returns true if the pointer is the mouse.
pub fn is_mouse(&self) -> bool {
matches!(self, PointerId::Mouse)
matches!(self.kind, PointerKind::Mouse)
}
/// Returns true if the pointer is a custom input.
pub fn is_custom(&self) -> bool {
matches!(self, PointerId::Custom(_))
/// Returns true if the pointer is a virtual input.
pub fn is_virtual(&self) -> bool {
matches!(self.kind, PointerKind::Virtual)
}
/// Returns the touch id if the pointer is a touch input.
pub fn get_touch_id(&self) -> Option<u64> {
if let PointerId::Touch(id) = self {
Some(*id)
if let PointerKind::Touch = self.kind {
Some(self.id)
} else {
None
}
}
}

/// The input device that created a pointer.
#[derive(Default, Debug, Clone, Copy, PartialEq, Eq, Hash, Reflect)]
#[reflect(Default, Debug, Hash, PartialEq)]
pub enum PointerKind {
/// A pointer that is controlled by a mouse.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// A pointer that is controlled by a mouse.
/// A pointer that behaves like a traditional mouse cursor.

///
/// Not a rodent; the device that you move around on a desk.
Mouse,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that PointerKind is meant to describe the behaviour of the pointer, not the actual input device, but it seems a bit unclear. It would be nice to be able to support things like a "mouse" pointer that's controlled by a joystick or a mouse controlled first person pov with diegetic touch screen UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the intent is that things like UI libraries will match on this variant, and behave accordingly.

/// A pointer that is controlled by a touch input.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// A pointer that is controlled by a touch input.
/// A pointer that behaves like it is controlled by touch.

Like an obvious thing I think we'd want to support here is touch input emulation with the mouse?

///
/// Typically with your fingers on a touch screen,
/// but can also be a stylus or other touch input device.
Touch,
/// A pointer that is driven by code.
///
/// No underlying hardware is associated with this pointer,
/// and it might represent a gamepad moving a cursor,
/// or an input sent to a focused element in a UI.
#[default]
Virtual,
}

/// Holds a list of entities this pointer is currently interacting with, sorted from nearest to
/// farthest.
#[derive(Debug, Default, Clone, Component, Reflect)]
Expand Down
8 changes: 5 additions & 3 deletions examples/ui/directional_navigation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use bevy::{
math::{CompassOctant, FloatOrd},
picking::{
backend::HitData,
pointer::{Location, PointerId},
pointer::{Location, PointerId, PointerKind},
},
platform_support::collections::{HashMap, HashSet},
prelude::*,
Expand Down Expand Up @@ -383,8 +383,10 @@ fn interact_with_focused_button(
commands.trigger_targets(
Pointer::<Click> {
target: focused_entity,
// We're pretending that we're a mouse
pointer_id: PointerId::Mouse,
pointer_id: PointerId {
id: 0,
kind: PointerKind::Virtual,
Copy link
Member Author

Choose a reason for hiding this comment

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

No more pretending to be a mouse!

},
// This field isn't used, so we're just setting it to a placeholder value
pointer_location: Location {
target: NormalizedRenderTarget::Image(
Expand Down
Loading