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

Improved Text2d glyph batching #17041

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Dec 30, 2024

Objective

Port the changes from #14848 to Text2d for improved glyph batching.

Solution

Store the glyph geometry in a seperate contiguous array, queue one Transparent2d item per text span, add indices into the glyph array to the ExtractedSprites for Text2d entities.


This still needs a bit more work, maybe ExtractedSprite should be an enum and I think the namings of some of the fields could be better. many_text2d doesn't show much improvement because it draws lots of four glyph text sections which means the number of extracted sprites is only reduced by three quarters. Whereas many_glyphs draws a single text span with 100,000 glyphs which results in a reduction from 100,000 sprites to just 1.

Testing

Ran some naive benchmarks:
```cargo run --example text_pipeline --release``
main: ~100fps
this: ~250fps

```cargo run --example many_text2d --release``
main: ~600fps
this: ~620fps

(modified many_glyphs example to only use Text2d)
```cargo run --example many_glyphs --release``
main: ~45fps
this: ~445fps


@ickshonpe ickshonpe requested a review from rparrett December 30, 2024 17:18
@ickshonpe ickshonpe added A-Text Rendering and layout for characters A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 30, 2024
@rparrett
Copy link
Contributor

rparrett commented Dec 30, 2024

I don't think I'm really qualified to review the rendering changes. But some notes:

The UI text rendering is really slow because it extracts each glyph as a separate ui node even though all the glyphs in a text section have the same texture, color and clipping rects.
-- 14848

I don't think this is true for the texture. Different glyphs of the same TextFont may reside in different atlases. I'm not sure if I'm engaging in pedantry with that bit of text or if it's an actual problem though.

edit: Okay, I see that's probably not an issue in the code here.

@ickshonpe ickshonpe changed the title Improved Text2d batching Improved Text2d glyph batching Dec 30, 2024
@ickshonpe
Copy link
Contributor Author

ickshonpe commented Dec 30, 2024

I don't think I'm really qualified to review the rendering changes. But some notes:

The UI text rendering is really slow because it extracts each glyph as a separate ui node even though all the glyphs in a text section have the same texture, color and clipping rects.
-- 14848

I don't think this is true for the texture. Different glyphs of the same TextFont may reside in different atlases. I'm not sure if I'm engaging in pedantry with that bit of text or if it's an actual problem though.

edit: Okay, I see that's probably not an issue in the code here.

Yep in the rareish case the font is split across multiple textures, with font_size: 1000. or something, it also starts a new group of glyphs. The check is here, you can see how it ends the current group if the next glyph is in a different span, its font texture atlas is different, or there no next glyph:

            if text_layout_info
                .glyphs
                .get(i + 1)
                .map(|info| {
                    info.span_index != current_span || info.atlas_info.texture != atlas_info.texture
                })
                .unwrap_or(true)
            {
                extracted_sprites.sprites.insert(
                    (
                        commands.spawn(TemporaryRenderEntity).id(),
                        original_entity.into(),
                    ),
                    ExtractedSprite {
                        transform,
                        color,
                        rect: None,
                        custom_size: None,
                        image_handle_id: atlas_info.texture.id(),
                        flip_x: false,
                        flip_y: false,
                        anchor: Anchor::Center.as_vec(),
                        original_entity: Some(original_entity),
                        group_indices: start..end,
                    },
                );
                start = end;
            }

@rparrett
Copy link
Contributor

Yep in the rareish case ... font_size: 1000. or something

This wouldn't be uncommon at all with something like CJK text, especially at larger (but still very reasonable) font sizes. But probably not worth doing something fancier here over. I'd rather add atlas-growing to mitigate that.

@ickshonpe
Copy link
Contributor Author

I don't like how get_glyph_atlas_info iterates through the fontatlas vec either, it should use a more direct lookup I think. Out of scope for this though.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Dec 30, 2024

I think, unlike the spritesource fix, this should be backportable to 0.15 without any external api changes.

@rparrett
Copy link
Contributor

rparrett commented Dec 30, 2024

This seems to have a measurable impact on sprite performance.

edit: this seems consistent on my m1 mac, but not quite as dramatic my earlier numbers indicated.

bevy_benchy bevymark 120 1000
main 47.28
5848beb 44.75 🔴 -5.4%
0dc0315 44.58 🔴 -5.7%

@alice-i-cecile
Copy link
Member

@akimakinai @SludgePhD @kristoff3r, could y'all review this one too? <3

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Dec 30, 2024

This seems to have a meaningful impact on sprite performance.

cargo run --example bevymark --release -- --waves 100 --per-wave 1000 --benchmark

main: ~57fps this pr: ~50fps

I didn't notice any significant difference on my computer but the new field holding the indices does add a couple of extra bytes to the size of an ExtractedSprite. I'll see if changing it to an enum does anything.

@BenjaminBrienen BenjaminBrienen added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Dec 31, 2024
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 A-Text Rendering and layout for characters C-Performance A change motivated by improving speed, memory usage or compile times D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants