split up `TextStyle` #15857

ickshonpe · 2024-10-11T16:42:36Z

Objective

Currently text is recomputed unnecessarily on any changes to its color, which is extremely expensive.

Solution

Split up TextStyle into two separate components TextFont and TextColor.

Testing

I added this system to many_buttons:

fn set_text_colors_changed(mut colors: Query<&mut TextColor>) {
    for mut text_color in colors.iter_mut() {
        text_color.set_changed();
    }
}

reports ~4fps on main, ~50fps with this PR.

Migration Guide

TextStyle has been renamed to TextFont and its color field has been moved to a separate component named TextColor which newtypes Color.

crates/bevy_text/src/text_access.rs

examples/animation/animation_events.rs

alice-i-cecile · 2024-10-11T16:50:56Z

I really like this change. It's more granular, allowing us to be lazier about recomputations, but IMO it's also much clearer to users.

rparrett · 2024-10-11T16:53:34Z

It looks like the current status of the examples is that they've been find/replaced, but many are likely broken in an obvious way. Feel free to ping me when this is ready for review.

viridia · 2024-10-11T16:59:55Z

This all sounds good to me. Adapting my framework to work with this will not be difficult.

ickshonpe · 2024-10-12T00:04:02Z

Yeah I thought about this, but not sure either. I can't think of any use since there aren't any TextColor fields or parameters?

Easier to use TextColor::from(Srgba::new(...)) than TextColor(Srgba::new(...).into()).

I guess and might be useful if someone is constructing a bunch of spans using an iterator or something.

crates/bevy_text/src/pipeline.rs

alice-i-cecile

I spotted a couple of missed internal renames, but those are just nits :) Nice stuff!

pablo-lua

Big changes. I have some questions

pablo-lua · 2024-10-12T13:31:42Z

examples/animation/animated_ui.rs

@@ -40,7 +40,7 @@ fn main() {
 }

 impl AnimatableProperty for FontSizeProperty {
-    type Component = TextStyle;
+    type Component = TextFont;


If I understand the bevy error 0005 correctly, we shouldn't generally be changing the size of a font this way, and instead use transform or ui_scale. Should this example continue to teach this possible footgun to the users?

Yeah I agree it's not a good example, it should probably be removed. Scaling the foot using transform isn't normally a good solution either as the text will end up blurred (or blocky if you turn off anti-aliasing). It's outside the scope of this PR to fix it though I think.

examples/animation/easing_functions.rs

pablo-lua · 2024-10-12T13:37:42Z

examples/games/game_menu.rs

@@ -433,11 +432,11 @@ mod menu {
                        // Display the game name
                        parent.spawn((
                            Text::new("Bevy Game Menu UI"),
-                            TextStyle {
+                            TextFont {
                                font_size: 67.0,


Question: generally we only set the size of the font when creating it. Is it possible that we maybe add a shorthand method for this? like TextFont::from_size(size: f32)

Yeah I agree it's annoying. FontSmoothing is quite niche as well, I'd rather it was a separate component.
I want to keep this PR to just dealing with color though as it's the biggest problem.

rparrett

I have some slight reservations about ux, particularly the amount of

let text_style = (
    TextFont {
        // ...
    },
    TextColor(/* ... */)
);

now happening, which I think will be very common in user code.

But if we're happy with the design then the code looks good to me. The benefits are certainly obvious.

pablo-lua

I'm fine with the changes and has some good improvements. I have one small nit, but otherwise LGTM

examples/async_tasks/external_source_external_thread.rs

ickshonpe · 2024-10-12T15:35:31Z

I have some slight reservations about ux, particularly the amount of
let text_style = (
    TextFont {
        // ...
    },
    TextColor(/* ... */)
);
now happening, which I think will be very common in user code.

But if we're happy with the design then the code looks good to me. The benefits are certainly obvious.

Yeah I think what this needs is just text font + color inheritance flowing down the tree, It's trivial to implement a basic version, but maybe depends what's being planned with bsn etc.

UkoeHB

Looking good, just one API request.

UkoeHB · 2024-10-12T21:47:22Z

crates/bevy_dev_tools/src/fps_overlay.rs

-        writer.for_each_style(entity, |mut style| {
-            *style = overlay_config.text_config.clone();
+        writer.for_each_font(entity, |mut font| {
+            *font = overlay_config.text_config.clone();
        });
+        writer.for_each_color(entity, |mut color| color.0 = overlay_config.text_color);


Can we keep for_each_style and have it pass both the font and color as params?

We could but I'm wondering if all these helper functions are even needed though. Users can just use for_each and ignore the fields that they don't need? We could return a struct with named fields instread of an optional tuple to make it more ergonomic.

Yeah, I believe that we should leave this helpers to a follow-up PR so we can discuss better what to do with them

crates/bevy_text/src/text.rs

ickshonpe added 8 commits October 11, 2024 16:26

split up textstyle

a6d1ac9

Merge branch 'main' into text-style-split

f225ee8

update text_access

300d378

updated some example

222efd3

Added TextColor to bevy_text prelude

86cd1cb

Updated text example

b89d4fc

updated examples

3a97382

updated more examples

4c4bf5c

alice-i-cecile requested review from cart and UkoeHB October 11, 2024 16:45

alice-i-cecile requested a review from viridia October 11, 2024 16:46

alice-i-cecile added this to the 0.15 milestone Oct 11, 2024

alice-i-cecile reviewed Oct 11, 2024

View reviewed changes

crates/bevy_text/src/text_access.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Oct 11, 2024

View reviewed changes

crates/bevy_text/src/text_access.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Oct 11, 2024

View reviewed changes

crates/bevy_text/src/text_access.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Oct 11, 2024

View reviewed changes

examples/animation/animation_events.rs Outdated Show resolved Hide resolved

alice-i-cecile added the C-Performance A change motivated by improving speed, memory usage or compile times label Oct 11, 2024

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 11, 2024

ickshonpe added 4 commits October 11, 2024 18:01

updated more examples

8858bac

updated virtual_time example

662071f

updated text_access, added color accessors

ebfe991

more examples

2356f9a

alice-i-cecile reviewed Oct 12, 2024

View reviewed changes

crates/bevy_text/src/pipeline.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Oct 12, 2024

View reviewed changes

crates/bevy_text/src/pipeline.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Oct 12, 2024

View reviewed changes

crates/bevy_text/src/pipeline.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Oct 12, 2024

View reviewed changes

crates/bevy_text/src/pipeline.rs Outdated Show resolved Hide resolved

alice-i-cecile approved these changes Oct 12, 2024

View reviewed changes

ickshonpe added 4 commits October 12, 2024 01:23

Fixed namings in pipeline

5c0d007

another renaming

da84131

More renamings

4796821

More renamings

bda4be2

alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 12, 2024

alice-i-cecile requested a review from rparrett October 12, 2024 01:22

pablo-lua reviewed Oct 12, 2024

View reviewed changes

ickshonpe added 2 commits October 12, 2024 15:26

Merge branch 'main' into text-style-split

270b640

renaming to fit new api

868b270

rparrett approved these changes Oct 12, 2024

View reviewed changes

pablo-lua approved these changes Oct 12, 2024

View reviewed changes

examples/async_tasks/external_source_external_thread.rs Outdated Show resolved Hide resolved

rparrett added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 12, 2024

ickshonpe added 2 commits October 12, 2024 16:28

More renamings

bc9ae9a

Removed text_style variable from example.

0833e1e

UkoeHB suggested changes Oct 12, 2024

View reviewed changes

update example variable names

07da055

UkoeHB approved these changes Oct 13, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Oct 13, 2024

Merged via the queue into bevyengine:main with commit 6f7d0e5 Oct 13, 2024
27 checks passed

rparrett mentioned this pull request Oct 13, 2024

Migrate from Query::single and friends to Single #15872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split up `TextStyle` #15857

split up `TextStyle` #15857

ickshonpe commented Oct 11, 2024 •

edited

Loading

alice-i-cecile commented Oct 11, 2024

rparrett commented Oct 11, 2024

viridia commented Oct 11, 2024

ickshonpe commented Oct 12, 2024

alice-i-cecile left a comment

pablo-lua left a comment

pablo-lua Oct 12, 2024

ickshonpe Oct 12, 2024 •

edited

Loading

pablo-lua Oct 12, 2024

ickshonpe Oct 12, 2024 •

edited

Loading

rparrett left a comment •

edited

Loading

pablo-lua left a comment

ickshonpe commented Oct 12, 2024 •

edited

Loading

UkoeHB left a comment •

edited

Loading

UkoeHB Oct 12, 2024

ickshonpe Oct 12, 2024 •

edited

Loading

pablo-lua Oct 12, 2024

split up TextStyle #15857

split up TextStyle #15857

Conversation

ickshonpe commented Oct 11, 2024 • edited Loading

Objective

Solution

Testing

Migration Guide

alice-i-cecile commented Oct 11, 2024

rparrett commented Oct 11, 2024

viridia commented Oct 11, 2024

ickshonpe commented Oct 12, 2024

alice-i-cecile left a comment

Choose a reason for hiding this comment

pablo-lua left a comment

Choose a reason for hiding this comment

pablo-lua Oct 12, 2024

Choose a reason for hiding this comment

ickshonpe Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

pablo-lua Oct 12, 2024

Choose a reason for hiding this comment

ickshonpe Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

rparrett left a comment • edited Loading

Choose a reason for hiding this comment

pablo-lua left a comment

Choose a reason for hiding this comment

ickshonpe commented Oct 12, 2024 • edited Loading

UkoeHB left a comment • edited Loading

Choose a reason for hiding this comment

UkoeHB Oct 12, 2024

Choose a reason for hiding this comment

ickshonpe Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

pablo-lua Oct 12, 2024

Choose a reason for hiding this comment

split up `TextStyle` #15857

split up `TextStyle` #15857

ickshonpe commented Oct 11, 2024 •

edited

Loading

ickshonpe Oct 12, 2024 •

edited

Loading

ickshonpe Oct 12, 2024 •

edited

Loading

rparrett left a comment •

edited

Loading

ickshonpe commented Oct 12, 2024 •

edited

Loading

UkoeHB left a comment •

edited

Loading

ickshonpe Oct 12, 2024 •

edited

Loading