Skip to content

Move all the pixel computations to Sprite::compute_pixel_space_point.#2

Merged
vandie merged 9 commits intovandie:ignore_transparancy_on_sprite_pickingfrom
andriyDev:pixel_space2
Nov 26, 2024
Merged

Move all the pixel computations to Sprite::compute_pixel_space_point.#2
vandie merged 9 commits intovandie:ignore_transparancy_on_sprite_pickingfrom
andriyDev:pixel_space2

Conversation

@andriyDev
Copy link
Copy Markdown

@andriyDev andriyDev commented Nov 23, 2024

I merged, and then added 2 commits on top. The first is a function to compute where in the texture a sprite point is. The second is rewriting the picking backend to take advantage of this. I tested this with the sprite_picking example and it seems to work quite well! Note this isn't just a move: I rewrote the logic. I've taken some of the logic from crates\bevy_sprite\src\render\mod.rs:406 and mixed in some of my own ideas. Hopefully this is more clear and also handles more cases!

I haven't dug in to figuring out SpriteImageMode yet, so that might be the next step.

PS: I goofed and originally set it to merge to your main rather than your PR.

bas-ie and others added 5 commits November 22, 2024 22:16
Fix stray character in docs.
# Objective

- Refactor

## Solution

- Refactor

## Testing

- Ran 3d_scene

---

## Migration Guide

`RenderCreation::Manual` variant fields are now wrapped in a struct
called `RenderResources`
…o a sprite, to the pixel point to sample from.
Copy link
Copy Markdown
Owner

@vandie vandie left a comment

Choose a reason for hiding this comment

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

I think this is likely on the right track tbh

// Check the alpha is above the cutoff.
color.alpha() > cutoff
}
SpritePickingMode::BoundingBox => true,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

if you switch to bounding box mode, does this still work. We're not actually using the in rect check anywhere below

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We've already checked that compute_pixel_space_point returns Ok, which means we're in the bounds of the sprite. So we've already done the check.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I added a comment for this though, since it definitely was confusing.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can we try to limit this PR to the change that we're actually trying to accomplish. Including random other changes like the structure of this file will just make it harder to merge the final change

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That just comes from the merge commit. It's already on bevy main.

… in picking_backend.

Previously SpriteBackendSettings was private, making it impossible to change the SpritePickingMode. Now we can!
@andriyDev
Copy link
Copy Markdown
Author

Ok apparently I'm being gaslit. This doesn't seem to be working correctly anymore?? I had tested it visually, but doing it now produces the wrong results. I need to investigate.

This is already taken care of in `compute_pixel_space_point`.
@andriyDev
Copy link
Copy Markdown
Author

Fixed the above issue. I must have broken things as I was cleaning up my commits. I also tested using the SpritePickingMode::BoundingBox and it looks to be working correctly.

This matches the `SpritePickingPlugin`.
@vandie vandie merged commit 8eb09ff into vandie:ignore_transparancy_on_sprite_picking Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants