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

Changing the color of a TextSection triggers a redraw of the full Text #6967

Open
ickshonpe opened this issue Dec 16, 2022 · 1 comment
Open
Labels
A-Text Rendering and layout for characters A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times

Comments

@ickshonpe
Copy link
Contributor

ickshonpe commented Dec 16, 2022

Bevy version

0.9.1

What went wrong

Changing the Color of a TextSection trips change detection causing the entire Text to be needlessly recomputed.

Additional information

Workaround:

text_query.for_each_mut(|mut text| {
  text.bypass_change_detection().text.sections[0].style.color = new_color;
});

related: #6956, #5935

@ickshonpe ickshonpe added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Dec 16, 2022
@ickshonpe ickshonpe changed the title Changing a text section's causes the entire text to be redrawn. Changing a text section's color causes the entire text to be redrawn. Dec 16, 2022
@ickshonpe ickshonpe changed the title Changing a text section's color causes the entire text to be redrawn. Changing the color of a TextSection causes the entire Text to be redrawn. Dec 16, 2022
@ickshonpe ickshonpe changed the title Changing the color of a TextSection causes the entire Text to be redrawn. Changing the color of a TextSection triggers a redraw of the full Text Dec 16, 2022
@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets and removed S-Needs-Triage This issue needs to be labelled labels Dec 16, 2022
@tigregalis
Copy link
Contributor

tigregalis commented Dec 31, 2022

That is true. colors are the only parts of Text that don't affect layout.

Text {
    sections: vec![
        TextSection {
            style: TextStyle {
                color: // ...
                // ...
            }
            // ...
        }
    ],
    // ...
}
...
pub struct Text {
    pub sections: Vec<TextSection>,
    pub alignment: TextAlignment,
}

pub struct TextSection {
    pub value: String,
    pub style: TextStyle,
}

pub struct TextAlignment {
    pub vertical: VerticalAlign,
    pub horizontal: HorizontalAlign,
}

pub enum VerticalAlign {
    Top,
    Center,
    Bottom,
}

pub enum HorizontalAlign {
    Left,
    Center,
    Right,
}

pub struct TextStyle {
    pub font: Handle<Font>,
    pub font_size: f32,
    pub color: Color,
}

Just exploring the many possible solutions:

Detect changes to color only and don't re-layout

Adapt the text layout system to special-case for color-only changes, but does change detection even have this level of granularity? Can it?

color lifted out of Text into its own component

But then how do you link the individual text sections to the various colors? e.g. would you have

#[derive(Component)]
struct TextSectionColors(Vec<Color>);

but this would be painful to synchronise with the entity's text sections.

color is a handle to an asset

struct TextStyle {
    color: ColorInstance
}

struct ColorInstance(Color);

This makes creating the text and changing the color less nice.

fn add_color_and_spawn(
    mut colors: ResMut<Assets<ColorInstance>>,
) {
    let handle = colors.add(ColorInstance(Color::RED));
    // do something with the handle
}

fn change_color(text_query: Query<Text>, mut colors: ResMut<Assets<ColorInstance>>) {
    text_query.for_each_mut(|mut text| {
      if let Some(color) = colors.get(&text.sections[0].style.color) {
          color.0 = new_color;
      }
    });
}

On the other hand, you could potentially reuse that color for other sections (even in other entities).
Still, if someone actually swaps out the referenced color handle (for whatever reason), it can still result in a re-layout, so best practice would be to always only have one color handle to one section.
You'd also want to destroy these assets as sections are destroyed.
Perhaps it could be made more ergonomic to provide some type of special garbage collected asset and/or some type of "unique" handle.

TextColor could be an enum

struct TextStyle {
    color: TextColor
}

enum TextColor {
    Plain(Color)
    Handle(Handle<ColorInstance>)
}

It could be a plain color, or a handle to an asset, so you get to choose (and you document the tradeoff).

TextSections are their own entities, as children of Text entities

Then you could have

#[derive(Component)]
struct TextSectionColor(Color);

// Parent
struct Text {
    sections: Vec<Entity>,
    alignment: 
}

// Child

The challenge is ensuring you only use TextSection as children*. This is the kinded entities problem.

*On the other hand, at this point, the only thing from Text that's still relevant is TextAlignment, but this starts to look simply like a Block and InlineChildAlignment, and there's no reason you can't have other inline elements (other than implementation).

This is probably the "correct" solution (and some of the explorations above can be orthogonal), but the ergonomics are a bit lacking (compared to the current status quo) and it's the most work.

@viridia viridia added the A-Text Rendering and layout for characters label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Text Rendering and layout for characters A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

No branches or pull requests

4 participants