Fix text alignment for unbounded text#17270
Merged
alice-i-cecile merged 3 commits intobevyengine:mainfrom Jan 11, 2025
Merged
Conversation
nicoburns
approved these changes
Jan 10, 2025
ickshonpe
approved these changes
Jan 10, 2025
Contributor
ickshonpe
left a comment
There was a problem hiding this comment.
Adding a check in update_buffer is exactly what I was thinking of. It's a very simple change and shouldn't affect UI text at all since UI text is always bounded by the size of its UI node.
I don't think we can be fussy about the performance regression, it's only during recomputation and users can manually add TextBounds themselves for anything performance critical.
My other suggestion to do a post-layout relayout and recalculate interaction coords would be a lot more involved and much more complicated and fragile. It isn't worth it, we'd be better off contributing the time to finding an upstream solution in cosmic-text.
Wouldn't expect to see any problems but I checked out all the text examples to be safe and didn't see anything wrong.
mrchantey
pushed a commit
to mrchantey/bevy
that referenced
this pull request
Feb 4, 2025
# Objective Fixes bevyengine#16783 ## Solution Works around a `cosmic-text` bug or limitation by triggering a re-layout with the calculated width from the first layout run. See linked issue. Credit to @ickshonpe for the clever solution. ## Performance This has a significant performance impact only on unbounded text that are not `JustifyText::Left`, which is still a bit of a bummer because text2d performance in 0.15.1 is already not great. But this seems better than alignment not working. ||many_text2d nfc re|many_text2d nfc re center| |-|-|-| |unbounded-layout-no-fix|3.06|3.10| |unbounded-layout-fix|3.05 ⬜ -0.2%|2.71 🟥 -12.5%| ## Testing I added a centered text to the `text2d` example. `cargo run --example text2d` We should look at other text examples and stress tests. I haven't tested as thoroughly as I would like, so help testing that this doesn't break something in UI would be appreciated.
mockersf
pushed a commit
that referenced
this pull request
Feb 6, 2025
Fixes #16783 Works around a `cosmic-text` bug or limitation by triggering a re-layout with the calculated width from the first layout run. See linked issue. Credit to @ickshonpe for the clever solution. This has a significant performance impact only on unbounded text that are not `JustifyText::Left`, which is still a bit of a bummer because text2d performance in 0.15.1 is already not great. But this seems better than alignment not working. ||many_text2d nfc re|many_text2d nfc re center| |-|-|-| |unbounded-layout-no-fix|3.06|3.10| |unbounded-layout-fix|3.05 ⬜ -0.2%|2.71 🟥 -12.5%| I added a centered text to the `text2d` example. `cargo run --example text2d` We should look at other text examples and stress tests. I haven't tested as thoroughly as I would like, so help testing that this doesn't break something in UI would be appreciated.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
Fixes #16783
Solution
Works around a
cosmic-textbug or limitation by triggering a re-layout with the calculated width from the first layout run. See linked issue.Credit to @ickshonpe for the clever solution.
Performance
This has a significant performance impact only on unbounded text that are not
JustifyText::Left, which is still a bit of a bummer because text2d performance in 0.15.1 is already not great. But this seems better than alignment not working.Testing
I added a centered text to the
text2dexample.cargo run --example text2dWe should look at other text examples and stress tests. I haven't tested as thoroughly as I would like, so help testing that this doesn't break something in UI would be appreciated.