Unify picking backends by chompaa · Pull Request #17348 · bevyengine/bevy

chompaa · 2025-01-13T18:28:43Z

Objective

Currently, our picking backends are inconsistent:

Mesh picking and sprite picking both have configurable opt in/out behavior. UI picking does not.
- Sprite picking uses SpritePickingCamera and Pickable for control, but mesh picking uses RayCastPickable.
MeshPickingPlugin is not a part of DefaultPlugins. SpritePickingPlugin and UiPickingPlugin are.

Solution

Add configurable opt in/out behavior to UI picking (defaults to opt out).
Replace RayCastPickable with MeshPickingCamera and Pickable.
Remove SpritePickingPlugin and UiPickingPlugin from DefaultPlugins.

Testing

Ran some examples.

Migration Guide

UiPickingPlugin and SpritePickingPlugin are no longer included in DefaultPlugins. They must be explicitly added.

RayCastPickable has been replaced in favor of the MeshPickingCamera and Pickable components. You should add them to cameras and entities, respectively, if you have MeshPickingSettings::require_markers set to true.

alice-i-cecile · 2025-02-24T21:10:32Z

@notmd @fjkorf can I get your reviews over here? I'd like to unify all this for 0.16.

fjkorf

These changes make sense to me. The naming is more consistent. I appreciate the extra comments scattered throughout. I'm not sure if it makes sense to leave picking out of the default plugins, but I don't think either option is bad. I was able to run this code locally and try the examples. All said this looks good to me.

notmd · 2025-02-25T08:08:36Z

crates/bevy_ui/src/picking_backend.rs

+        if settings.require_markers && node.pickable.is_none() {
+            continue;


I think we should not make picking opt-out with require_markers. Maybe introduce a second flag or remove configurable opt-out. The require_markers should only use to config the camera, not the UI elements picking opt-in/out

Maybe introduce a second flag

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 UiPickingCamera when all your UI needs Pickable seems off. Happy to hear your thoughts though :)

[...] or remove configurable opt-out

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).

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 Pickable) and 1 camera. Then you start adding more cameras and now want to disable picking for them. You starting set make require_markers to true, but this will regret (even if you make your main camera to allow picking by inserting UiPickingCamera) all UI elements you have before because now it requires to explicit to insert Pickable. For UI, I only want to insert Pickable when I want to custom the behavior of Pickable. The only reason for UI picking opt-in is performance, but I believe you will soon realize that the performance gain is not worth the DX.

I think renaming the flag inside of UIPickingSettings to something more explicit like only_pickable is reasonable.

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.

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.

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 UiPickingPlugin should be added as a part of UiPlugin, but I'm not sure I agree with SpritePickingPlugin being added by default. What's the consensus there? If we decide that SpritePickingPlugin be added by default then I think MeshPickingPlugin should be too.

Edit: MeshPickingPlugin will need to be considered too.

I think no SpritePickingPlugin by default; it's definitely feasible to write games that never use it. The same cannot be said for building UIs that don't!

Not sure on the camera config TBH 🤔

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.

notmd · 2025-02-25T14:19:09Z

About the SpritePickingPlugin and UiPickingPlugin can we add it directly to the DefaultPlugins?. I agree removing them from SpritePlugin and UiPlugin, but many other plugins are doing the same.

NthTensor

Looks fine to me. Cart's suggestion to enable ui and sprite picking by default is still outstanding, but we have a release candidate to cut, and I'd like this to be in there from the start.

I propose we merge this, spin out the remaining few work items, and do some small refinements during the RC.

cart · 2025-03-18T19:13:28Z

crates/bevy_ui/src/lib.rs

                )
                    .chain(),
            )
+            .add_plugins(UiPickingPlugin)


This is not gated on the feature flag, which is inconsistent with the sprite picking backend and also seems necessary for configuration.

cart · 2025-03-18T19:15:21Z

examples/ui/tab_navigation.rs

+            DefaultPlugins,
+            InputDispatchPlugin,
+            TabNavigationPlugin,
+            UiPickingPlugin,


This is no longer necessary, as it is provided by default. The other examples touched by this PR should also be updated to remove the now-spurious registrations.

# Objective Currently, our picking backends are inconsistent: - Mesh picking and sprite picking both have configurable opt in/out behavior. UI picking does not. - Sprite picking uses `SpritePickingCamera` and `Pickable` for control, but mesh picking uses `RayCastPickable`. - `MeshPickingPlugin` is not a part of `DefaultPlugins`. `SpritePickingPlugin` and `UiPickingPlugin` are. ## Solution - Add configurable opt in/out behavior to UI picking (defaults to opt out). - Replace `RayCastPickable` with `MeshPickingCamera` and `Pickable`. - Remove `SpritePickingPlugin` and `UiPickingPlugin` from `DefaultPlugins`. ## Testing Ran some examples. ## Migration Guide `UiPickingPlugin` and `SpritePickingPlugin` are no longer included in `DefaultPlugins`. They must be explicitly added. `RayCastPickable` has been replaced in favor of the `MeshPickingCamera` and `Pickable` components. You should add them to cameras and entities, respectively, if you have `MeshPickingSettings::require_markers` set to `true`. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

@cart

# Objective @cart noticed some issues with my work in #17348 (comment), which I somehow missed before merging the PR. ## Solution - feature gate the UiPickingPlugin correctly - don't manually add the picking plugins ## Testing Ran the debug_picking and sprite_picking examples (for UI and sprites respectively): both seem to work fine.

@cart

# Objective @cart noticed some issues with my work in #17348 (comment), which I somehow missed before merging the PR. ## Solution - feature gate the UiPickingPlugin correctly - don't manually add the picking plugins ## Testing Ran the debug_picking and sprite_picking examples (for UI and sprites respectively): both seem to work fine.

alice-i-cecile added M-Release-Note Work that should be called out in the blog due to impact M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 13, 2025

alice-i-cecile requested review from NthTensor and aevyrie January 13, 2025 18:38

alice-i-cecile added this to the 0.16 milestone Jan 13, 2025

This was referenced Feb 13, 2025

Make sprite picking opt-in #17842

Merged

Disable sprite picking in bevymark #17868

Closed

alice-i-cecile removed the M-Release-Note Work that should be called out in the blog due to impact label Feb 24, 2025

chompaa added 7 commits February 24, 2025 19:08

Update mesh picking

ad10ea2

Update sprite picking

4cc698a

Rebase

6d6fde8

Fixes

0feea8f

Fix UI opt in

1d03834

CI

fe21cc4

Fix docs

ec411bd

chompaa force-pushed the unify-picking-backends branch from eb6d2a7 to ec411bd Compare February 25, 2025 00:12

Merge branch 'main' into unify-picking-backends

5f7a11a

fjkorf approved these changes Feb 25, 2025

View reviewed changes

notmd reviewed Feb 25, 2025

View reviewed changes

NthTensor approved these changes Mar 18, 2025

View reviewed changes

alice-i-cecile 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 Mar 18, 2025

Merge branch 'main' into unify-picking-backends

5670736

alice-i-cecile mentioned this pull request Mar 18, 2025

Enable UI and sprite picking by default #18400

Closed

alice-i-cecile enabled auto-merge March 18, 2025 18:47

alice-i-cecile added 4 commits March 18, 2025 12:01

Don't register deleted type

51fcfca

Tiny docs for SpritePickingPlugin

b13025e

Add SpritePickingPlugin to SpritePlugin

46b0a14

Enable UiPickingPlugin by default

8ed3727

alice-i-cecile requested a review from NthTensor March 18, 2025 19:05

alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Mar 18, 2025

NthTensor approved these changes Mar 18, 2025

View reviewed changes

cart reviewed Mar 18, 2025

View reviewed changes

alice-i-cecile added this pull request to the merge queue Mar 18, 2025

Merged via the queue into bevyengine:main with commit 65e289f Mar 18, 2025
33 checks passed

alice-i-cecile mentioned this pull request Mar 18, 2025

Unified picking cleanup #18401

Merged

		if settings.require_markers && node.pickable.is_none() {
		continue;

Uh oh!

Conversation

chompaa commented Jan 13, 2025

Objective

Solution

Testing

Migration Guide

Uh oh!

alice-i-cecile commented Feb 24, 2025

Uh oh!

fjkorf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmd Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chompaa Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmd commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NthTensor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

notmd Feb 25, 2025 •

edited

Loading

chompaa Feb 27, 2025 •

edited

Loading

notmd commented Feb 25, 2025 •

edited

Loading