Text2d visibility fix #9708
Closed
Text2d visibility fix #9708
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…that all share the same entity for visibility checks. Then `extracted_text2d_sprite` extracts into `ExtractedSpriteBatches` instead of `ExtractedSprites` and text drawn with `Text2d` is visible again.
alice-i-cecile
added
C-Bug
| @@ -590,6 +597,38 @@ pub fn queue_sprites( | |||
| }); | |||
| } | |||
| } | |||
|
|
|||
| for (batch_entity, sprite_entities) in extracted_batches.batches.iter() { | |||
| if !view_entities.contains(batch_entity.index() as usize) { | |||
There was a problem hiding this comment.
This could use a bit of a comment: it's really quite unusual to care about the index of an Entity.
There was a problem hiding this comment.
The original code wasn't commented either, I'm confident these changes work and don't mess anything up but I don't know enough about the rendering internals to be explaining anything to anyone I think.
…e vec for each batch.
mockersf
approved these changes
Sep 8, 2023
mnmaita
reviewed
Sep 13, 2023
| @@ -333,6 +333,27 @@ pub struct ExtractedSprites { | |||
| pub sprites: SparseSet<Entity, ExtractedSprite>, | |||
| } | |||
|
|
|||
| /// Allows extraction of multiply sprites for a single entity that all share that entity's id for visibility resolution. | |||
There was a problem hiding this comment.
Suggested change
| /// Allows extraction of multiply sprites for a single entity that all share that entity's id for visibility resolution. | |
| /// Allows the extraction of multiple sprites sharing the same entity id for visibility resolution. |
I think there's a typo here (multiply -> multiple). I'm also leaving a suggestion to rephrase the comment, hopefully it still conveys the same meaning.
|
I prefer #10100 if it fixes the same problem. |
|
Closing due to merging #10100 If that PR doesn’t address the problem, you’re welcome to come shout at me. 💜 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 13, 2023
# 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%|
ameknite
pushed a commit
to ameknite/bevy
that referenced
this pull request
Nov 6, 2023
# 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
# 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Text drawn with
Text2disn't visible in main.fixes #9676
Solution
Add an
ExtractedSpriteBatchesresource that allows the extraction of sprites in batches that all share the same entity for visibility checks.In
extract_text2d_spriteextract toExtractedSpriteBatchesrather thanExtractedSprites.This is a bit of a hack but it's harmless and minimal.
My first attempt had three buffers:
Text2dentities with theirEntity,Transform, and a range into buffer 2.VecofExtractedGlyphSections. EachExtractedGlyphSectionhas a handle for the font's atlas texture, color, and a range of indices into buffer 3.VecofExtractedGlyphs. EachExtractedGlyphhas a position (relative to the transform) and aRectcontaining coords of the glyph in the font's atlas texture.I struggled to get it to work though. It was almost there but there were some weird texturing issues I couldn't work out even after spending half a day on it:
I still don't understand Bevy's rendering internals very well, so it could be anything.
Anyway, I decided to switch approach. This PR is deliberately as simple as I could make it. It fixes
Text2drendering and shouldn't introduce any new problems or have any significant performance costs. The implementations ofExtractedSpriteBatchesandextract_text2d_spritesaren't super efficient, but naive benchmarks suggest thisText2dis faster than 0.11.2'sText2dby about 5-10% (probably entirely due to the improvements in bevy_sprites).(yellow is this PR, red is main)
The main drawback is that
ExtractedSpriteBatchesis exposed as public, I'd rather have kept it private as it's not really suitable for much else except text rendering.Changelog
ExtractedSpriteBatches.queue_spritesandprepare_spritesso they also add the sprites fromExtractedSpriteBatches.extract_text2d_spriteextracts toExtractedSpriteBatchesinstead ofExtractedSprites.