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 rendering of sprites, text, and meshlets after #12582. #12945

Merged

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Apr 12, 2024

Sprite, Text, and Handle<MeshletMesh> were types of renderable entities that the new segregated visible entity system didn't handle, so they didn't appear.

Because bevy_text depends on bevy_sprite, and the visibility computation of text happens in the latter crate, I had to introduce a new marker component, SpriteSource. SpriteSource marks entities that aren't themselves sprites but become sprites during rendering. I added this component to Text2dBundle. Unfortunately, this is technically a breaking change, although I suspect it won't break anybody in practice except perhaps editors.

Fixes #12935.

Changelog

Changed

  • Text2dBundle now includes a new marker component, SpriteSource. Bevy uses this internally to optimize visibility calculation.

Migration Guide

  • Text now requires a SpriteSource marker component in order to appear. This component has been added to Text2dBundle.

`Sprite`, `Text`, and `Handle<MeshletMesh>` were types of renderable
entities that the new segregated visible entity system didn't handle, so
they didn't appear.

Because `bevy_text` depends on `bevy_sprite`, and the visibility
computation of text happens in the latter crate, I had to introduce a
new marker component, `SpriteSource`. `SpriteSource` marks entities that
aren't themselves sprites but become sprites during rendering. I added
this component to `Text2dBundle`. Unfortunately, this is technically a
breaking change, although I suspect it won't break anybody in practice
except perhaps editors.
@pcwalton pcwalton requested a review from superdump April 12, 2024 20:03
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Not a thorough review but looks ok. Nothing controversial in my opinion. Noting this in case others get to it more thoroughly before me.

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Apr 12, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen P-High This is particularly urgent, and deserves immediate attention M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Apr 12, 2024
Copy link
Contributor

@Elabajaba Elabajaba left a comment

Choose a reason for hiding this comment

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

It fixes the bug for me.

It'd be nice if there was a way to collapse this into a 1 or 2 liner somehow, but not essential.

                check_visibility::<WithMeshletMesh>
                    .in_set(VisibilitySystems::CheckVisibility)
                    .after(VisibilitySystems::CalculateBounds)
                    .after(VisibilitySystems::UpdateOrthographicFrusta)
                    .after(VisibilitySystems::UpdatePerspectiveFrusta)
                    .after(VisibilitySystems::UpdateProjectionFrusta)
                    .after(VisibilitySystems::VisibilityPropagate)
                    .after(TransformSystem::TransformPropagate),

/// Right now, this is used for `Text`.
#[derive(Component, Reflect, Clone, Copy, Debug, Default)]
#[reflect(Component, Default)]
pub struct SpriteSource;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand why we need this, but I also couldn't get this working with just Text so I assume it's necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, actually I think I came up with a way to make that no longer necessary. I'll try it tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, never mind, it'd be uglier than the current solution.

The problem is that Text uses the sprite visibility checking system. But that system can't name Text as it's an upstream crate. So we need some marker component that the sprite visibility checker can name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, the idea I had was to introduce a "label" type that's different from the component type. That way Sprite and Text would have the same label. But then you'd need two check_visibility systems, one for each component, that together populate the same VisibleEntities list. I think that's uglier than just using SpriteSource.

@alice-i-cecile alice-i-cecile 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 Apr 13, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 13, 2024
Merged via the queue into bevyengine:main with commit 8577a44 Apr 13, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide P-High This is particularly urgent, and deserves immediate attention S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sprite example broken after #12582: Divide the single VisibleEntities list
4 participants