Skip to content
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

Jump to bottom

Make sprite picking opt-in #17225

Merged
merged 7 commits into from
Jan 9, 2025
Merged

Make sprite picking opt-in #17225

merged 7 commits into from
Jan 9, 2025

Conversation

chompaa
Copy link
Member

@chompaa chompaa commented Jan 7, 2025 •
edited
Loading

Objective

Fixes #16903.

Solution

  • Make sprite picking opt-in by requiring a new SpritePickingCamera component for cameras and usage of a new Pickable component for entities.
  • Update the sprite_picking example to reflect these changes.
  • Some reflection cleanup (I hope that's ok).

Testing

Ran the sprite_picking example

Open Questions

  • Is the name `SpritePickable` appropriate?
  • Should `SpritePickable` be in `bevy_sprite::prelude?

Migration Guide

The sprite picking backend is now strictly opt-in using the SpritePickingCamera and Pickable components. You should add the Pickable component any entities that you want sprite picking to be enabled for, and mark their respective cameras with SpritePickingCamera.

@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Contentious There are nontrivial implications that should be thought through A-Picking Pointing at and selecting objects of all sorts C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 7, 2025
@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jan 7, 2025
@chompaa chompaa force-pushed the sprite-picking-opt-in branch from c80bad8 to a0008cc Compare January 8, 2025 00:51
@chompaa chompaa added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 8, 2025
alice-i-cecile
alice-i-cecile requested changes Jan 8, 2025
View reviewed changes
Copy link
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.

Looking better :) Final changes:

  1. Move marker component to bevy_picking, and call it Pickable.
  2. Split apart the components and call the camera one SpritePickingCamera.

Follow-up:

  • use this design for mesh-picking as well

@chompaa
Copy link
Member Author

chompaa commented Jan 8, 2025 

2. Split apart the components and call the camera one `SpritePickingCamera`.

What are your thoughts on a PickingCamera component in bevy_picking? I can see it being used in the mesh picking backend and the UI picking backend if we were to support diegetic UI.

I'd be happy to tackle the follow-up :)

@chompaa
Copy link
Member Author

chompaa commented Jan 8, 2025

What are your thoughts on a PickingCamera component in bevy_picking? I can see it being used in the mesh picking backend and the UI picking backend if we were to support diegetic UI.

On second thought, I don't think this makes sense. Cameras could be used for distinct backends, so generalizing seems wrong.

@chompaa chompaa requested a review from alice-i-cecile January 8, 2025 07:51
@alice-i-cecile
Copy link
Member

On second thought, I don't think this makes sense. Cameras could be used for distinct backends, so generalizing seems wrong.

Exactly my thinking.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 8, 2025
@alice-i-cecile 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 Jan 8, 2025
aevyrie
aevyrie suggested changes Jan 8, 2025
View reviewed changes
crates/bevy_picking/src/backend.rs Outdated Show resolved Hide resolved
aevyrie
aevyrie reviewed Jan 8, 2025
View reviewed changes
crates/bevy_sprite/src/picking_backend.rs Outdated
//! # Usage
//!
//! This backend is strictly opt-in. For entities to be considered for sprite picking, you should
//! mark them with [`Pickable`] and their respective cameras with [`SpritePickingCamera`].
Copy link
Member

@aevyrie aevyrie Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sometimes you want things to be opt-out, especially if everything is interactive, or if you are just debugging.

More importantly, we should make sure our backends are consistent, at least the first party ones. The mesh picking backend has a resource that controls whether it is opt in or opt out. Instead of changing this behavior for everyone, the need for this PR suggests we need both, and we should match the mesh picking backend.

https://docs.rs/bevy/latest/bevy/picking/mesh_picking/struct.MeshPickingSettings.html

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, the debugging use case is a good argument for making this configurable 🤔 I would like to unify this across all of our backends though.

Copy link
Member Author

Choose a reason for hiding this comment

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

My only qualm is that we lose some performance benefit, but I'm in agreement.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is the question of whether it should be opt in by default or not. Backend consistency says it should be opt out, but perhaps we want to push users to opt in?

Copy link
Member

@aevyrie aevyrie Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

From my experience working on mod_picking, it's a better experience for users if it starts as opt-out, because it removes friction and steps to just get things working. Once it is working, it's then another small step to switch to opt-in if they decide to.

@chompaa
Copy link
Member Author

chompaa commented Jan 8, 2025

I've pushed some changes:

  • Removed Pickable in favor of using PickingBehavior (I'd like to follow up on a rename).
  • Added require_markers to SpritePickingSettings (defaults to false to align with the mesh picking backend).

I also think that the SpritePickingPlugin should be explicitly added by the user, and not a part of DefaultPlugins (and in general, all backends should be), but that seems out of scope for this PR.

@alice-i-cecile 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 Jan 8, 2025
@aevyrie
Copy link
Member

aevyrie commented Jan 9, 2025 •
edited
Loading

I've pushed some changes: [...]

Agree with everything here. Doing a pass to standardize all backends and make them all opt-in would be great.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 9, 2025
Merged via the queue into bevyengine:main with commit 0a9740c Jan 9, 2025
29 checks passed
@chompaa chompaa mentioned this pull request Jan 9, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 12, 2025
@chompaa
Rename PickingBehavior to Pickable (#17266)
02bb151 
# Objective

PR #17225 allowed for sprite picking to be opt-in. After some
discussion, it was agreed that `PickingBehavior` should be used to
opt-in to sprite picking behavior for entities. This leads to
`PickingBehavior` having two purposes: mark an entity for use in a
backend, and describe how it should be picked. Discussion led to the
name `Pickable`making more sense (also: this is what the component was
named before upstreaming).

A follow-up pass will be made after this PR to unify backends.

## Solution

Replace all instances of `PickingBehavior` and `picking_behavior` with
`Pickable` and `pickable`, respectively.

## Testing

CI

## Migration Guide

Change all instances of `PickingBehavior` to `Pickable`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

@rparrett rparrett Awaiting requested review from rparrett

@NthTensor NthTensor Awaiting requested review from NthTensor

@aevyrie aevyrie Awaiting requested review from aevyrie

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

Assignees
No one assigned
Labels
A-Picking Pointing at and selecting objects of all sorts C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
0.16
Development

Successfully merging this pull request may close these issues.

Consider changing the default picking behavior of sprites to be non-pickable by default
4 participants
@chompaa @alice-i-cecile @aevyrie @BenjaminBrienen