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

Perform text scaling calculations per text, not per glyph #7819

Merged
merged 25 commits into from
Mar 14, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Feb 25, 2023

Objective

The scaling calculations for text rendering are being performed per glyph but they only need to be performed per text.

Solution

Move these calculations outside of the inner loop.


Changelog

  • Moved text scaling calculations outside of the inner loop in extract_text_uinodes and extract_text2d_sprite.
  • Added a new example to stress tests, "many_glyphs".

@ickshonpe ickshonpe changed the title Move text scaling calculations out of the inner extract text loop Perform text scaling calculations per text, not per glyph Feb 25, 2023
@james7132 james7132 added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Feb 25, 2023
@james7132
Copy link
Member

This looks generally pretty sound in terms of approach, but I'd like to see some backing numbers for this to show the impact.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Feb 25, 2023

It seems like a solid improvement:

cargo run --example many_glyphs --profile stress-test --features trace_tracy

text_perf_capture

many_buttons the difference is smaller as the text sections are only five or so glyphs each, so it spends much less time inside the inner loop:

tp_many_buttons_cap

@james7132 james7132 removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Feb 26, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Good changes, and a fresh benchmark! Nice work.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Really like these gains, but there are a few bits about readability I want addressed before this is merged.

crates/bevy_text/src/text2d.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved
crates/bevy_text/src/text2d.rs Outdated Show resolved Hide resolved
crates/bevy_text/src/text2d.rs Outdated Show resolved Hide resolved
crates/bevy_text/src/text2d.rs Outdated Show resolved Hide resolved
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

LGTM, though I want to see some final performance numbers. I'm sure the changes made have some positive benefit, but it's ideal if we can quantify them.

@james7132 james7132 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 Mar 13, 2023
@ickshonpe
Copy link
Contributor Author

LGTM, though I want to see some final performance numbers. I'm sure the changes made have some positive benefit, but it's ideal if we can quantify them.

cargo run --profile stress-test --features trace_tracy --example many_buttons

many_buttons_text_perf

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 14, 2023
Merged via the queue into bevyengine:main with commit e77eb00 Mar 14, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
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-UI Graphical user interfaces, styles, layouts, and widgets C-Performance A change motivated by improving speed, memory usage or compile times 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.

4 participants