-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Unify picking backends #17348
Unify picking backends #17348
Changes from all commits
ad10ea2
4cc698a
6d6fde8
0feea8f
1d03834
fe21cc4
ec411bd
5f7a11a
5670736
51fcfca
b13025e
46b0a14
8ed3727
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,18 +29,59 @@ use bevy_app::prelude::*; | |
use bevy_ecs::{prelude::*, query::QueryData}; | ||
use bevy_math::{Rect, Vec2}; | ||
use bevy_platform_support::collections::HashMap; | ||
use bevy_reflect::{std_traits::ReflectDefault, Reflect}; | ||
use bevy_render::prelude::*; | ||
use bevy_transform::prelude::*; | ||
use bevy_window::PrimaryWindow; | ||
|
||
use bevy_picking::backend::prelude::*; | ||
|
||
/// An optional component that marks cameras that should be used in the [`UiPickingPlugin`]. | ||
/// | ||
/// Only needed if [`UiPickingSettings::require_markers`] is set to `true`, and ignored | ||
/// otherwise. | ||
#[derive(Debug, Clone, Default, Component, Reflect)] | ||
#[reflect(Debug, Default, Component)] | ||
pub struct UiPickingCamera; | ||
|
||
/// Runtime settings for the [`UiPickingPlugin`]. | ||
#[derive(Resource, Reflect)] | ||
#[reflect(Resource, Default)] | ||
pub struct UiPickingSettings { | ||
/// When set to `true` UI picking will only consider cameras marked with | ||
/// [`UiPickingCamera`] and entities marked with [`Pickable`]. `false` by default. | ||
/// | ||
/// This setting is provided to give you fine-grained control over which cameras and entities | ||
/// should be used by the UI picking backend at runtime. | ||
pub require_markers: bool, | ||
} | ||
|
||
#[expect( | ||
clippy::allow_attributes, | ||
reason = "clippy::derivable_impls is not always linted" | ||
)] | ||
#[allow( | ||
clippy::derivable_impls, | ||
reason = "Known false positive with clippy: <https://github.com/rust-lang/rust-clippy/issues/13160>" | ||
)] | ||
impl Default for UiPickingSettings { | ||
fn default() -> Self { | ||
Self { | ||
require_markers: false, | ||
} | ||
} | ||
} | ||
|
||
/// A plugin that adds picking support for UI nodes. | ||
/// | ||
/// This is included by default in [`UiPlugin`](crate::UiPlugin). | ||
#[derive(Clone)] | ||
pub struct UiPickingPlugin; | ||
impl Plugin for UiPickingPlugin { | ||
fn build(&self, app: &mut App) { | ||
app.add_systems(PreUpdate, ui_picking.in_set(PickSet::Backend)); | ||
app.init_resource::<UiPickingSettings>() | ||
.register_type::<(UiPickingCamera, UiPickingSettings)>() | ||
.add_systems(PreUpdate, ui_picking.in_set(PickSet::Backend)); | ||
} | ||
} | ||
|
||
|
@@ -63,8 +104,14 @@ pub struct NodeQuery { | |
/// we need for determining picking. | ||
pub fn ui_picking( | ||
pointers: Query<(&PointerId, &PointerLocation)>, | ||
camera_query: Query<(Entity, &Camera, Has<IsDefaultUiCamera>)>, | ||
camera_query: Query<( | ||
Entity, | ||
&Camera, | ||
Has<IsDefaultUiCamera>, | ||
Has<UiPickingCamera>, | ||
)>, | ||
primary_window: Query<Entity, With<PrimaryWindow>>, | ||
settings: Res<UiPickingSettings>, | ||
ui_stack: Res<UiStack>, | ||
node_query: Query<NodeQuery>, | ||
mut output: EventWriter<PointerHits>, | ||
|
@@ -81,7 +128,8 @@ pub fn ui_picking( | |
// cameras. We want to ensure we return all cameras with a matching target. | ||
for camera in camera_query | ||
.iter() | ||
.map(|(entity, camera, _)| { | ||
.filter(|(_, _, _, cam_can_pick)| !settings.require_markers || *cam_can_pick) | ||
.map(|(entity, camera, _, _)| { | ||
( | ||
entity, | ||
camera.target.normalize(primary_window.single().ok()), | ||
|
@@ -91,7 +139,7 @@ pub fn ui_picking( | |
.filter(|(_entity, target)| target == &pointer_location.target) | ||
.map(|(cam_entity, _target)| cam_entity) | ||
{ | ||
let Ok((_, camera_data, _)) = camera_query.get(camera) else { | ||
let Ok((_, camera_data, _, _)) = camera_query.get(camera) else { | ||
continue; | ||
}; | ||
let mut pointer_pos = | ||
|
@@ -122,6 +170,10 @@ pub fn ui_picking( | |
continue; | ||
}; | ||
|
||
if settings.require_markers && node.pickable.is_none() { | ||
continue; | ||
Comment on lines
+173
to
+174
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should not make picking opt-out with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
To me, this would decrease readability for opt-in behavior. In my opinion, if you opt-in, you are opting to be explicit, so not having to add
To the best of my knowledge UI picking is going to drive a lot of the interactions going forward, so I think it makes sense to be opt-out by default (and consequently have the option to opt-in since there's no (?) perf loss). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, sorry for not making it clear. I mean a common scenario is you start with picking opt-out (all elements are pickable without inserting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think renaming the flag inside of That said the reason for making picking opt-in is performance, and that performance gain is worth a learning moment in the developer experience. The DX of updating your UI to reflect your picking preferences does not seem onerous. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should remove the ability to toggle opt-out behavior at the plugin level, and then use required components as needed to get "automatic picking". We don't need more mechanisms for this! I also think that the UiPickingPlugin should be added as part of UiPlugin. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, that makes sense, I agree required components do solve the DX issue. How do we want to handle it for cameras though? I feel like having the plugin level toggle is nice in this case. I can see why Edit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think no Not sure on the camera config TBH 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think sprite picking and ui picking should be enabled by default. Sprite picking has almost no cost associated with it (per-entity opt-in, filtered out cheaply via archetype queries) and provides high utility, and UI picking is a core piece of the UI puzzle. |
||
} | ||
|
||
// Nodes that are not rendered should not be interactable | ||
if node | ||
.inherited_visibility | ||
|
@@ -208,7 +260,7 @@ pub fn ui_picking( | |
|
||
let order = camera_query | ||
.get(*camera) | ||
.map(|(_, cam, _)| cam.order) | ||
.map(|(_, cam, _, _)| cam.order) | ||
.unwrap_or_default() as f32 | ||
+ 0.5; // bevy ui can run on any camera, it's a special case | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not gated on the feature flag, which is inconsistent with the sprite picking backend and also seems necessary for configuration.