Add optional transparency passthrough for sprite backend with bevy_picking #16388

vandie · 2024-11-14T09:43:42Z

Objective

Allow bevy_sprite_picking backend to pass through transparent sections of the sprite.
Fixes Add support for alpha-testing in sprite picking backend #14929

Solution

After sprite picking detects the cursor is within a sprites rect, check the pixel at that location on the texture and check that it meets an optional transparency cutoff. Change originally created for mod_picking on bevy 0.14 (Ignore transparent sections of sprite using sprite-picking aevyrie/bevy_mod_picking#373)

Testing

Ran Sprite Picking example to check it was working both with transparency enabled and disabled
ModPicking version is currently in use in my own isometric game where this has been an extremely noticeable issue

Showcase

Migration Guide

Sprite picking now ignores transparent regions (with an alpha value less than or equal to 0.1). To configure this, modify the SpriteBackendSettings resource.

github-actions · 2024-11-14T09:43:59Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

mgi388

I’m excited for this change. I did a quick visual code review but haven’t tested it yet.

mgi388 · 2024-11-14T10:05:28Z

crates/bevy_sprite/src/picking_backend.rs

+    /// Off by default for backwards compatibility. This setting is provided to give you fine-grained
+    /// control over if transparency on sprites is ignored.
+    pub passthrough_transparency: bool,
+    /// How Opaque does part of a sprite need to be in order count as none-transparent (defaults to 10)


Can you elaborate in the docs what the units are? 10 what? What is a valid range I could use, as a user?

Can these two options be combined into a single enum? It doesn't make sense to have an alpha cutoff setting when alpha passthrough is disabled. e.g.

SpritePickingAlphaTest { /// Don't test alpha, only consider the rect of sprites. Ignore, /// Test the pixel alpha when running hit tests. Alpha values below this will not be considered for picking. Threshold(u8) }

Yeah an enum is probably a better idea for those settings. Will rework to include.

crates/bevy_sprite/src/picking_backend.rs

mgi388 · 2024-11-14T10:13:39Z

@vandie also see #14929 I think your PR fixes it.

vandie · 2024-11-14T10:14:16Z

@vandie also see #14929 I think your PR fixes it.

I think it does yeah.

mgi388 · 2024-11-14T10:22:15Z

@vandie also see #14929 I think your PR fixes it.

I think it does yeah.

You can put “Fixes #{id}” in your PR description so merging this PR closes the issue as fixed. 😌

vandie · 2024-11-17T09:15:11Z

Seems everyone agrees that this should be enabled by default so I've updated that and introduced a new SpriteBackendAlphaPassthrough enum so this can be controlled by a single setting rather than multiple.

github-actions · 2024-11-17T13:57:24Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

alice-i-cecile · 2024-11-17T13:57:41Z

Can you add a small migration guide explaining that this is now on by default? Definitely the better default, but could break existing users.

vandie · 2024-11-17T14:50:15Z

Can you add a small migration guide explaining that this is now on by default? Definitely the better default, but could break existing users.

Added. Hope that's ok and clear?

alice-i-cecile · 2024-11-17T17:24:58Z

The migration guide was clear and effective, but the tone / target audience was a bit weird. The goal is to communicate "why is my stuff broken and how do I fix it", but your initial draft was closer to a change log :) I've quickly edited the PR description incorporating the information you've added. These get automatically scraped / compiled, so it's good to clean them up here.

crates/bevy_sprite/src/picking_backend.rs

andriyDev

Sorry for the previous two comments not being part of this review, I've been noticing while trying to adapt this picking backend into my project.

crates/bevy_sprite/src/picking_backend.rs

MonforteAdrian · 2024-11-22T07:46:55Z

Testing the changes got this error after a crash:

2024-11-22T07:39:32.137594Z  INFO bevy_winit::system: Creating new window "App" (0v1#4294967296)
2024-11-22T07:39:32.137772Z  INFO winit::platform_impl::platform::x11::window: Guessed window scale factor: 1
2024-11-22T07:39:32.326951Z  WARN bevy_render::view::window: Couldn't get swap chain texture after configuring. Cause: 'The underlying surface has changed, and therefore the swap chain must be updated'
thread 'Compute Task Pool (4)' panicked at /home/adrian/bevy_testing/bevy/crates/bevy_image/src/image.rs:974:36:
range end index 8 out of range for slice of length 4
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_sprite::picking_backend::sprite_picking`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

I'm using 0.15 release branch with the changes applied on top. Running the sprite_picking example
The sprite on the right works fine, but the squares and bevy logos works quite bad and make the app crash.
The test run on linux with x11 as you can see in the errors

andriyDev · 2024-11-22T08:02:06Z

Testing the changes got this error after a crash:

I verified this on Windows and on main as well. Looks like there's something wrong with Image::get_color_at. Dang! I'll file another issue.

vandie · 2024-11-22T10:08:14Z

Testing the changes got this error after a crash:

I verified this on Windows and on main as well. Looks like there's something wrong with Image::get_color_at. Dang! I'll file another issue.

edit: Oh wait, irritating, this looks like a different panic...

andriyDev

@vandie #16475 has been merged so that should fix one set of panics.

crates/bevy_sprite/src/picking_backend.rs

andriyDev · 2024-11-23T01:36:11Z

I tested the sprite_picking example, looks like the black squares are offset by half its width and height. Color sprites are actually a 1x1 image, so it looks like we're offset by half a pixel somewhere.

Edit: nevermind I completely misunderstood what was happening, looks like the color sprites are off by half width/height which in this case is 64x64

Edit 2: DOUBLE NEVERMIND I WAS CONFUSED, we pass a custom size to the color sprite, which makes it 64x64, but the image size is 1x1.

andriyDev

We're not considering the custom_size of sprites, or the rect of sprites inside the pixel checking code.

I also think we should consider moving all the pixel picking logic into the Sprite struct as a method. It seems reasonable to want to reuse "given this relative position, where am I in the texture for this sprite?" (but like with the float part remaining).

Edit: It looks like we're also not considering Sprite::flip_x, Sprite::flip_y, and Sprite::image_mode. I'm tinkering to see if I can get an implementation that works.

andriyDev · 2024-11-23T04:27:22Z

crates/bevy_sprite/src/picking_backend.rs

+                            })
+                            .or(Some(URect::new(0, 0, texture.width(), texture.height())))?;
+                        // get mouse position on texture
+                        let texture_position = (texture_rect.center().as_vec2()


We're getting the center and then casting to a float. Odd image sizes will mean the center won't be at 0.5, 0.5 like we'd expect. I'm pretty sure we need to cast the rect to f32 and then get the center, so texture_rect.as_rect().center().

Hmm, let me check my commits because I think I had some correction for this. Wonder if I accidentally removed it with all the reworking we've done.

andriyDev · 2024-11-23T18:53:05Z

@vandie When you have time, could you take a look at vandie#2 ? I tested it and it looks like it solves a lot of issues! (just wanna make sure it doesn't get lost)

vandie added 2 commits November 14, 2024 09:38

Add optional transparancy passthrough for sprite backend

c5dd17a

set correct default

bba576b

vandie added 4 commits November 14, 2024 09:44

fix typo

f9088c7

fix too many arguments

d1013cf

fix doc typo

1924e02

clean up final check into match

42c156b

mgi388 reviewed Nov 14, 2024

View reviewed changes

vandie added 2 commits November 14, 2024 10:18

fix docs and naming

3ca1039

change transparency to alpha in variable names

7038101

kristoff3r added S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Picking Pointing at and selecting objects of all sorts D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Nov 14, 2024

alice-i-cecile added this to the 0.16 milestone Nov 14, 2024

alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Nov 14, 2024

BenjaminBrienen approved these changes Nov 15, 2024

View reviewed changes

vandie added 3 commits November 17, 2024 09:10

Add AlphaPassthrough enum and change default to on

1f48ff2

improve comments

5ee1448

Merge branch 'main' into ignore_transparancy_on_sprite_picking

16f8654

vandie added 2 commits November 17, 2024 09:16

update comments

38eb823

update comments

26f4511

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 17, 2024

aevyrie reviewed Nov 18, 2024

View reviewed changes

crates/bevy_sprite/src/picking_backend.rs Outdated Show resolved Hide resolved

vandie added 4 commits November 18, 2024 09:50

rename Sprite Picking Mode

0737631

fix typo

0acb495

Merge branch 'main' into ignore_transparancy_on_sprite_picking

e35c25d

Merge branch 'main' into ignore_transparancy_on_sprite_picking

71f9383

aevyrie approved these changes Nov 19, 2024

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 Nov 19, 2024

andriyDev reviewed Nov 21, 2024

View reviewed changes

crates/bevy_sprite/src/picking_backend.rs Outdated Show resolved Hide resolved

andriyDev reviewed Nov 21, 2024

View reviewed changes

crates/bevy_sprite/src/picking_backend.rs Outdated Show resolved Hide resolved

andriyDev reviewed Nov 21, 2024

View reviewed changes

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 Show resolved Hide resolved

vandie added 3 commits November 21, 2024 14:54

switch to using bevy internal get_color_at

3afc5ca

add missing backticks

f0a9095

fix double backticks

c9fff89

andriyDev mentioned this pull request Nov 22, 2024

Disabling bevy_*_picking_backend does not disable backends. #16469

Closed

andriyDev mentioned this pull request Nov 22, 2024

Calling Image::get_color_at panics #16474

Closed

vandie added 2 commits November 22, 2024 10:08

improve accuracy and stop out of range error

c246fba

Merge branch 'main' into ignore_transparancy_on_sprite_picking

513a934

andriyDev reviewed Nov 22, 2024

View reviewed changes

crates/bevy_sprite/src/picking_backend.rs Outdated Show resolved Hide resolved

crates/bevy_sprite/src/picking_backend.rs Outdated Show resolved Hide resolved

vandie added 2 commits November 22, 2024 21:45

change floor location

c64dbdf

Merge branch 'main' into ignore_transparancy_on_sprite_picking

50e862d

andriyDev reviewed Nov 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional transparency passthrough for sprite backend with bevy_picking #16388

Add optional transparency passthrough for sprite backend with bevy_picking #16388

vandie commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024

mgi388 left a comment

mgi388 Nov 14, 2024 •

edited

Loading

aevyrie Nov 16, 2024

vandie Nov 17, 2024

mgi388 commented Nov 14, 2024

vandie commented Nov 14, 2024

mgi388 commented Nov 14, 2024

vandie commented Nov 17, 2024

github-actions bot commented Nov 17, 2024

alice-i-cecile commented Nov 17, 2024

vandie commented Nov 17, 2024 •

edited

Loading

alice-i-cecile commented Nov 17, 2024

andriyDev left a comment •

edited

Loading

MonforteAdrian commented Nov 22, 2024 •

edited

Loading

andriyDev commented Nov 22, 2024 •

edited

Loading

vandie commented Nov 22, 2024 •

edited

Loading

andriyDev left a comment

andriyDev commented Nov 23, 2024 •

edited

Loading

andriyDev left a comment •

edited

Loading

andriyDev Nov 23, 2024

vandie Nov 23, 2024

andriyDev commented Nov 23, 2024 •

edited

Loading

Add optional transparency passthrough for sprite backend with bevy_picking #16388

Are you sure you want to change the base?

Add optional transparency passthrough for sprite backend with bevy_picking #16388

Conversation

vandie commented Nov 14, 2024 • edited Loading

Objective

Solution

Testing

Showcase

Migration Guide

github-actions bot commented Nov 14, 2024

mgi388 left a comment

Choose a reason for hiding this comment

mgi388 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

aevyrie Nov 16, 2024

Choose a reason for hiding this comment

vandie Nov 17, 2024

Choose a reason for hiding this comment

mgi388 commented Nov 14, 2024

vandie commented Nov 14, 2024

mgi388 commented Nov 14, 2024

vandie commented Nov 17, 2024

github-actions bot commented Nov 17, 2024

alice-i-cecile commented Nov 17, 2024

vandie commented Nov 17, 2024 • edited Loading

alice-i-cecile commented Nov 17, 2024

andriyDev left a comment • edited Loading

Choose a reason for hiding this comment

MonforteAdrian commented Nov 22, 2024 • edited Loading

andriyDev commented Nov 22, 2024 • edited Loading

vandie commented Nov 22, 2024 • edited Loading

andriyDev left a comment

Choose a reason for hiding this comment

andriyDev commented Nov 23, 2024 • edited Loading

andriyDev left a comment • edited Loading

Choose a reason for hiding this comment

andriyDev Nov 23, 2024

Choose a reason for hiding this comment

vandie Nov 23, 2024

Choose a reason for hiding this comment

andriyDev commented Nov 23, 2024 • edited Loading

vandie commented Nov 14, 2024 •

edited

Loading

mgi388 Nov 14, 2024 •

edited

Loading

vandie commented Nov 17, 2024 •

edited

Loading

andriyDev left a comment •

edited

Loading

MonforteAdrian commented Nov 22, 2024 •

edited

Loading

andriyDev commented Nov 22, 2024 •

edited

Loading

vandie commented Nov 22, 2024 •

edited

Loading

andriyDev commented Nov 23, 2024 •

edited

Loading

andriyDev left a comment •

edited

Loading

andriyDev commented Nov 23, 2024 •

edited

Loading