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

Fix visibility setting to ViewVisibility #49

Conversation

luca-della-vedova
Copy link
Contributor

Hello!

In the migration to 0.12 in #43, there was a mistake here where a ComputedVisibility.set_visible_in_view() was migrated to a setting to InheritedVisibility.
This resulted in the plugin breaking all hierarchical visibility propagations as soon as a grid is spawned, making everything always visible at all times.
In this PR I changed it to what it should have been migrated to, which is a setting to ViewVisibility.

CC @pcwalton just for a sanity check

@ThomasAlban
Copy link

Thank god, tracking down that it was this crate rather than an error in my code took me ages to find!

Copy link
Collaborator

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Thank you! We are in the process of updating to 0.12 so we haven't used the crate yet in practice.

Can you bump the patch version in the cargo.toml? This will trigger CI to release the updated version

@pcwalton
Copy link
Contributor

LGTM

Signed-off-by: Luca Della Vedova <[email protected]>
@luca-della-vedova
Copy link
Contributor Author

Bumped the patch version!

Thank god, tracking down that it was this crate rather than an error in my code took me ages to find!

I hear you! It took me several days, a plea for help in Discord and digging all the way into bevy_hierarchy to finally figure it out, it really was a journey

@IceSentry IceSentry enabled auto-merge February 23, 2024 04:19
@IceSentry
Copy link
Collaborator

Welp, looks like bumping from a branch of someone that isn't an owner of the crate doesn't work with our CI. I'll revert the commit and bump it myself. Sorry about that.

@IceSentry IceSentry disabled auto-merge February 23, 2024 07:27
@loispostula loispostula merged commit 86018dd into ForesightMiningSoftwareCorporation:main Feb 23, 2024
13 of 14 checks passed
@luca-della-vedova luca-della-vedova deleted the luca/fix_visibility branch February 23, 2024 08:01
@luca-della-vedova luca-della-vedova restored the luca/fix_visibility branch February 23, 2024 08:02
@luca-della-vedova
Copy link
Contributor Author

Thanks for the quick review! Can you folks push a patch release to crates.io? I would almost consider yanking the previous version since the bug is pretty severe but I'll leave the choice up to you!

@IceSentry
Copy link
Collaborator

Hey, very sorry about that, we had CI issues and I never realized that a version with this fix was never published. I just published it so now 0.10.1 has the fix and is compatible with bevy 0.12.

In the bevy 0.13 version of this crate we just removed the shadow feature because it was causing too many issues like this.

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.

5 participants