Skip to content

Make sprite picking opt-in#17842

Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom
notmd:sprite_picking_opt_in
Feb 24, 2025
Merged

Make sprite picking opt-in#17842
alice-i-cecile merged 2 commits intobevyengine:mainfrom
notmd:sprite_picking_opt_in

Conversation

@notmd
Copy link
Copy Markdown
Contributor

@notmd notmd commented Feb 13, 2025 •
edited
Loading

Objective

Fix #17108
See #17108 (comment)

Solution

  • Make the query match &Pickable instead Option<&Pickable>

Testing

  • Run the sprite_picking example and everything still work

Migration Guide

  • Sprite picking are now opt-in, make sure you insert Pickable component when using sprite picking.
-commands.spawn(Sprite { .. } );
+commands.spawn((Sprite { .. }, Pickable::default());

@rparrett
Copy link
Copy Markdown
Contributor

rparrett commented Feb 13, 2025

Related PR: #17348

@alice-i-cecile alice-i-cecile added 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 A-Picking Pointing at and selecting objects of all sorts M-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 labels Feb 13, 2025
@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Feb 13, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Feb 13, 2025
@chompaa
Copy link
Copy Markdown
Member

chompaa commented Feb 14, 2025 •
edited
Loading

I think having the ability to opt-out is still valuable (e.g. debugging).

One solution that comes to mind is adding SpritePickingPlugin::opt_in (default true). We could have sprite_picking use generic QueryData, passing Option<&Pickable> or &Pickable based on opt_in.

Edit: woops, it's late and I didn't fully think the above solution through, perhaps there's still a way to add opt-out behavior?

@notmd
Copy link
Copy Markdown
Contributor Author

notmd commented Feb 14, 2025

I think there is still a way to make it opt-out but it hurts performance and potential footgun.

@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Feb 14, 2025

I think the best way for users to restore opt-out behavior (e.g. during debugging) is to just add an observer of their own, and add the required component automatically. There's no need for a dedicated setting for that.

@rparrett rparrett mentioned this pull request Feb 15, 2025
Closed
@fjkorf
Copy link
Copy Markdown
Contributor

fjkorf commented Feb 17, 2025

I looked over this update (and the preceding) and it all makes sense. I ran the sprite_picking example locally and it did, in fact, still work. Looks good to me.

@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 Feb 20, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 24, 2025
Merged via the queue into bevyengine:main with commit f7b2a02 Feb 24, 2025
1 check passed
@akimakinai akimakinai mentioned this pull request May 5, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request May 5, 2025
@akimakinai
Sprite picking docs fix (#19016)
0f6d532 
# Objective

- Docs in sprite picking plugin / example contain outdated information.

References:
- Sprite picking now always require `Picking` - #17842
- Transparency pass-through added - #16388

## Solution

- Fix the docs.
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this pull request May 17, 2025
@akimakinai @andrewzhurov
Sprite picking docs fix (bevyengine#19016)
f8dd8c3 
# Objective

- Docs in sprite picking plugin / example contain outdated information.

References:
- Sprite picking now always require `Picking` - bevyengine#17842
- Transparency pass-through added - bevyengine#16388

## Solution

- Fix the docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

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-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it 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.

Per Entity sprite picking (and maybe friends) opt-out

5 participants

@notmd @rparrett @chompaa @alice-i-cecile @fjkorf