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.

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

Add bevy_picking sprite backend #14757

Merged
merged 15 commits into from
Aug 26, 2024
Merged

Conversation

jnhyatt
Copy link
Contributor

@jnhyatt jnhyatt commented Aug 15, 2024

Objective

Add bevy_picking sprite backend as part of the bevy_mod_picking upstreamening (#12365).

Solution

More or less a copy/paste from bevy_mod_picking, with the changes here. I'm putting that link here since those changes haven't yet made it through review, so should probably be reviewed on their own.

Testing

I couldn't find any sprite-backend-specific tests in bevy_mod_picking and unfortunately I'm not familiar enough with Bevy's testing patterns to write tests for code that relies on windowing and input. I'm willing to break the pointer hit system into testable blocks and add some more modular tests if that's deemed important enough to block, otherwise I can open an issue for adding tests as follow-up.

Follow-up work

  • More docs/tests
  • Ignore pick events on transparent sprite pixels with potential opt-out

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged A-Picking Pointing at and selecting objects of all sorts C-Feature A new feature, making something new possible labels Aug 15, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Aug 15, 2024
@aevyrie
Copy link
Member

aevyrie commented Aug 17, 2024

This doesn't test against alpha, which might be surprising in a first-party implementation. Would be nice to document on the plugin.

Additionally, it would be courteous to co-author the downstream contributors in a commit here.

crates/bevy_sprite/src/picking_backend.rs Outdated Show resolved Hide resolved
@jnhyatt
Copy link
Contributor Author

jnhyatt commented Aug 19, 2024

This doesn't test against alpha, which might be surprising in a first-party implementation. Would be nice to document on the plugin.

Not sure what you mean by "test against alpha". Could you elaborate?
EDIT - By that, do you mean whether or not the sprite is visible due to alpha/invisible texels? I'll add docs for that.

Additionally, it would be courteous to co-author the downstream contributors in a commit here.

Absolutely! I thought I'd opened this in draft and I hadn't planned on it getting merged without crediting you. Since you're the original author, is there anyone else you'd like listed as co-author? I also have to say I've never added a co-author, so I might mess it up a couple times, haha

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@jnhyatt jnhyatt removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Aug 19, 2024
@aevyrie
Copy link
Member

aevyrie commented Aug 19, 2024

Not sure what you mean by "test against alpha". Could you elaborate? EDIT - By that, do you mean whether or not the sprite is visible due to alpha/invisible texels? I'll add docs for that.

Exactly. I think most users would expect fully transparent pixels to not be hittable. Might be something where the backend/component specifies a threshold, so any alpha below that value will be ignored. Doesn't need to be added right now, but would be nice to mention in the plugin docs.

is there anyone else you'd like listed as co-author?

There are a bunch of people who have contributed to this backend. I'd take a look at the commit history here - I'm not sure if there is an easier way to do this: https://github.com/aevyrie/bevy_mod_picking/commits/main/backends/bevy_picking_sprite/src/lib.rs

I also have to say I've never added a co-author, so I might mess it up a couple times, haha

No worries! I think git makes it pretty simple, you just need to add the names/emails separated by newlines in a commit.

Copy link
Contributor

@kristoff3r kristoff3r left a comment

Choose a reason for hiding this comment

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

A few non-blocking code quality comments. I also tested the example on both linux and wasm.

crates/bevy_picking/src/lib.rs Outdated Show resolved Hide resolved
has_sprite_ambiguities Outdated Show resolved Hide resolved
crates/bevy_sprite/src/picking_backend.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/picking_backend.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/picking_backend.rs Outdated Show resolved Hide resolved
@jnhyatt jnhyatt added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 23, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 26, 2024
Merged via the queue into bevyengine:main with commit 3540b87 Aug 26, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Picking Pointing at and selecting objects of all sorts A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants