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.

Sign up for GitHub

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

Jump to bottom

Fix text2d view-visibility #10100

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Fix text2d view-visibility #10100

merged 3 commits into from
Oct 13, 2023

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Oct 12, 2023 •
edited
Loading

Objective

Fixes #9676
Possible alternative to #9708

Text2dBundles are not currently drawn because the render-world-only entities for glyphs that are created in extract_text2d_sprite are not tracked by the per-view VisibleEntities.

Solution

Add an Option<Entity> to ExtractedSprite that keeps track of the original entity that caused a "glyph entity" to be created.

Use that in queue_sprites if it exists when checking view visibility.

Benchmarks

Quick benchmarks. Average FPS over 1500 frames.

bench before fps after fps diff
many_sprites 884.93 879.00 🟡 -0.7%
bevymark -- --benchmark --waves 100 --per-wave 1000 --mode sprite 75.99 75.93 🟡 -0.1%
bevymark -- --benchmark --waves 50 --per-wave 1000 --mode mesh2d 32.85 32.58 🟡 -0.8%

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Oct 13, 2023
@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Oct 13, 2023
@alice-i-cecile alice-i-cecile added P-High This is particularly urgent, and deserves immediate attention C-Regression Functionality that used to work but no longer does. Add a test for this! and removed C-Regression Functionality that used to work but no longer does. Add a test for this! labels Oct 13, 2023
superdump
superdump approved these changes Oct 13, 2023
View reviewed changes
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.

Assuming this works, I prefer this PR to the other one.

@robtfm
Copy link
Contributor

robtfm commented Oct 13, 2023 •
edited
Loading

Don’t want to muddy the waters but since the new sprites are only created for visible entities could we just have a force_visible bool that gets set to true on the new ones? Not sure if there’s other value to tracking the original entity

edit: why don’t we not extract invisible sprites and omit the check entirely?

@rparrett
Copy link
Contributor Author

Don’t want to muddy the waters but since the new sprites are only created for visible entities could we just have a force_visible bool that gets set to true on the new ones? Not sure if there’s other value to tracking the original entity

My understanding is that entities that are definitely not visible in any view are not extracted, but the visibility check in queue_sprites is per-view / based on RenderLayers.

@rparrett
Copy link
Contributor Author

I re-did the benchmarks on power / without a sneaky bevy ant simulator running in the background / actually averaging all 1500 framerate samples (oops).

@superdump superdump mentioned this pull request Oct 13, 2023
Closed
@superdump superdump added this pull request to the merge queue Oct 13, 2023
Merged via the queue into bevyengine:main with commit 05c87f3 Oct 13, 2023
25 checks passed
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
@rparrett @ameknite
Fix text2d view-visibility (bevyengine#10100)
ec14490 
# Objective

Fixes bevyengine#9676
Possible alternative to bevyengine#9708

`Text2dBundles` are not currently drawn because the render-world-only
entities for glyphs that are created in `extract_text2d_sprite` are not
tracked by the per-view `VisibleEntities`.

## Solution

Add an `Option<Entity>` to `ExtractedSprite` that keeps track of the
original entity that caused a "glyph entity" to be created.

Use that in `queue_sprites` if it exists when checking view visibility.

## Benchmarks

Quick benchmarks. Average FPS over 1500 frames.

| bench | before fps | after fps | diff |
|-|-|-|-|
|many_sprites|884.93|879.00|🟡 -0.7%|
|bevymark -- --benchmark --waves 100 --per-wave 1000 --mode
sprite|75.99|75.93|🟡 -0.1%|
|bevymark -- --benchmark --waves 50 --per-wave 1000 --mode
mesh2d|32.85|32.58|🟡 -0.8%|
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@rparrett
Fix text2d view-visibility (bevyengine#10100)
3f4846f 
# Objective

Fixes bevyengine#9676
Possible alternative to bevyengine#9708

`Text2dBundles` are not currently drawn because the render-world-only
entities for glyphs that are created in `extract_text2d_sprite` are not
tracked by the per-view `VisibleEntities`.

## Solution

Add an `Option<Entity>` to `ExtractedSprite` that keeps track of the
original entity that caused a "glyph entity" to be created.

Use that in `queue_sprites` if it exists when checking view visibility.

## Benchmarks

Quick benchmarks. Average FPS over 1500 frames.

| bench | before fps | after fps | diff |
|-|-|-|-|
|many_sprites|884.93|879.00|🟡 -0.7%|
|bevymark -- --benchmark --waves 100 --per-wave 1000 --mode
sprite|75.99|75.93|🟡 -0.1%|
|bevymark -- --benchmark --waves 50 --per-wave 1000 --mode
mesh2d|32.85|32.58|🟡 -0.8%|
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superdump superdump superdump approved these changes

@robtfm robtfm robtfm approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention
Projects
None yet
Milestone
0.12
Development

Successfully merging this pull request may close these issues.

Text2d is broken
4 participants
@rparrett @robtfm @superdump @alice-i-cecile