Rich text

[tigregalis]

Refer #1222

This takes advantage of glyph_brush_layout's SectionText API.

Currently the Text component has an assumption that there is exactly one section only, so all I've done is create a "sections" API to connect it to glyph_brush_layout's API. I've made as few changes as possible otherwise.

However, I've updated all examples with text in them, including enhancing them with multiple text sections where I thought it made sense.

This is a breaking change, so this will need a changelog entry - I wasn't sure where to put this, so please advise.

We can potentially flatten the API further; see the questions below.

New API

The new API is as follows:

// Component
pub struct Text {
    // pub value: String,             // moved to TextSection
    // pub font: Handle<Font>,        // moved to TextSection
    // pub style: TextStyle,          // moved to TextSection
    pub sections: Vec<TextSection>,   // +
    pub alignment: TextAlignment,     // +
}

pub struct TextSection {              // new
    pub value: String,                // +
    pub font: Handle<Font>,           // +
    pub style: TextStyle,             // +
}

pub struct TextStyle {
    pub font_size: f32,
    pub color: Color,
    // pub alignment: TextAlignment,  // moved to Text
}

Questions

It might make sense to either:

1. spread the members of TextStyle into TextSection, or
2. move font from TextSection into TextStyle

Example

Example below:

The "Score: " and the "10" have different fonts, font sizes, and colours

fn setup(
    commands: &mut Commands,
    mut materials: ResMut<Assets<ColorMaterial>>,
    asset_server: Res<AssetServer>,
) {
    // Add the game's entities to our world
    commands
        // ...
        // scoreboard
        .spawn(TextBundle {
            text: Text {
                sections: vec![
                    TextSection {
                        font: asset_server.load("fonts/FiraSans-Bold.ttf"),
                        value: "Score: ".to_string(),
                        style: TextStyle {
                            color: Color::rgb(0.5, 0.5, 1.0),
                            font_size: 40.0,
                        },
                    },
                    TextSection {
                        font: asset_server.load("fonts/FiraMono-Medium.ttf"),
                        value: "".to_string(),
                        style: TextStyle {
                            color: Color::rgb(0.9, 0.3, 1.0),
                            font_size: 60.0,
                        },
                    },
                ],
                ..Default::default()
            },
            style: Style {
                position_type: PositionType::Absolute,
                position: Rect {
                    top: Val::Px(5.0),
                    left: Val::Px(5.0),
                    ..Default::default()
                },
                ..Default::default()
            },
            ..Default::default()
        });
    // ...
}

fn scoreboard_system(scoreboard: Res<Scoreboard>, mut query: Query<&mut Text>) {
    for mut text in query.iter_mut() {
        text.sections[1].value = scoreboard.score.to_string();
    }
}

[mockersf]

Very nice and works well!

I'm a little unhappy by the fact that it makes the default case of displaying a simple text less simple, mostly that it now has to be a Vec of one element which is not intuitive... but I don't see a simple solution for that without either a lot of code duplication, or having two components in the bundle and pick the correct one based on context. Maybe with an enum

enum TextType {
    Text(Text),
    Sections(Vec<TextSection>)
}

[tigregalis]

Very nice and works well!

I'm a little unhappy by the fact that it makes the default case of displaying a simple text less simple, mostly that it now has to be a Vec of one element which is not intuitive... but I don't see a simple solution for that without either a lot of code duplication, or having two components in the bundle and pick the correct one based on context. Maybe with an enum

enum TextType {
    Text(Text),
    Sections(Vec<TextSection>)
}

Text alignment needs to go somewhere at the block level. Here's a couple of different variations I've come up with:

1. https://github.com/tigregalis/bevy/blob/rich-text-enum/crates/bevy_text/src/text.rs

2. https://github.com/tigregalis/bevy/blob/rich-text-enum-high/crates/bevy_text/src/text.rs

From the latter, it might be possible to spread the TextSection into the BasicText struct, and construct the TextSection on demand. I'll try that next.

So far I can't say that the enum approach has made things simpler, but it did lead me to the .sections() abstraction.

I'm gradually more convinced that the TextStyle struct doesn't really need to exist, and its members can be part of the TextSection struct.

I also tried creating a trait and implementing it for TextSection and Vec<TextSection> but abandoned that when it became clear it was making things way more complex.

[tigregalis]

I also did impl From<BasicText> for Text and impl From<RichText> for Text.

In retrospect, we can do this without enums at all, so we get the old API for text with one section, but just have to call Text::from(BasicText { / * ... * / }) or BasicText { / * ... * / }.into().

https://github.com/tigregalis/bevy/tree/rich-text-from/crates/bevy_text/src/text.rs

#[derive(Debug, Default, Clone)]
pub struct BasicText {
    pub value: String,
    pub font: Handle<Font>,
    pub style: TextStyle,
    pub alignment: TextAlignment,
}

impl From<BasicText> for Text {
    fn from(source: BasicText) -> Self {
        let BasicText {
            value,
            font,
            style,
            alignment,
        } = source;
        Self {
            sections: vec![TextSection { value, font, style }],
            alignment,
        }
    }
}

commands.spawn(TextBundle {
    text: BasicText {
        value: "hello world!",
        ..Default::default()
    }.into(),
    ..Default::default()
});

We can use a similar approach with a HTML-like API as in #1222.

[mockersf]

that looks nicer with the From implementation 👍
and we could add later implementations for a MdText, UnityStyledText, ...

[tigregalis]

So at this stage we've got a working rich text API along with a slightly updated API for basic text with a single style, and which is also extensible in the future.

What are our thoughts on either:

1. spread the members of TextStyle into TextSection, or

pub struct TextSection {
    pub value: String,
    pub font: Handle<Font>,
    // pub style: TextStyle,   // -
    pub font_size: f32,        // +
    pub color: Color,          // +
}

2. move font from TextSection into TextStyle

pub struct TextSection {
    pub value: String,
    // pub font: Handle<Font>, // -
    pub style: TextStyle,
}

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

I think now's the time to decide this as we're already changing the API as it is.

In general, I favour option 1 because it's flatter/simpler (easier to write).

One reason I can foresee for keeping a separate TextStyle struct is potentially sharing a TextStyle definition among multiple entities and/or multiple sections, in which case, TextStyle should be an asset and we should store a handle to it. Thus a TextStyle would be roughly equivalent to a CSS class. This would take a bit of work I think though.

pub struct TextSection {
    pub value: String,
    pub style: Handle<TextStyle>,
}

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

[mockersf]

I prefer option 2, as it allows easier reuse of a style.

I don't think moving the style to a Handle is a good idea, as I find handles are somewhat costlier (cognitive and runtime) to use:

• additional parameter to the system
• need a ResMut<Assets<TextStyle>> to add one, and I try to avoid ResMut
• handles make more sense for heavy objects that you can't clone around easily, or that you want to handle differently to send to the gpu. this isn't the case here, it's size is like sizeof(Handle) + f32 + f32*4, so we would be avoiding copying 20 bytes, I don't think that will impact anything much

[tigregalis]

I prefer option 2, as it allows easier reuse of a style.

I don't think moving the style to a Handle is a good idea, as I find handles are somewhat costlier (cognitive and runtime) to use:

• additional parameter to the system

• need a ResMut<TextStyle> to add one, and I try to avoid ResMut

• handles make more sense for heavy objects that you can't clone around easily, or that you want to handle differently to send to the gpu. this isn't the case here, it's size is like sizeof(Handle) + f32 + f32*4, so we would be avoiding copying 20 bytes, I don't think that will impact anything much

Ok, fair. I'm convinced. I've made the change to option 2.

The new API is as follows:

// Component
pub struct Text {
    // pub value: String,             // - (moved to `TextSection`)
    // pub font: Handle<Font>,        // - (moved to `TextStyle`)
    // pub style: TextStyle,          // - (moved to `TextSection`)
    pub sections: Vec<TextSection>,   // +
    pub alignment: TextAlignment,     // +
}

pub struct TextSection {              // NEW
    pub value: String,                // +
    pub style: TextStyle,             // +
}

pub struct TextStyle {
    pub font: Handle<Font>,           // +
    pub font_size: f32,
    pub color: Color,
    // pub alignment: TextAlignment,  // - (moved to `Text`)
}

// Transient helper type. Construct it then call `.into()` to convert to a `Text` component.
pub struct BasicText {                // NEW
    pub value: String,
    pub style: TextStyle,
    pub alignment: TextAlignment,
}

// Text with a single section:
commands.spawn(TextBundle {
    // construct a `BasicText`
    text: BasicText {
        value: "hello world!".to_string(),
        style: TextStyle {
            font: asset_server.load("fonts/FiraSans-Bold.ttf"),
            font_size: 40.0,
            color: Color::WHITE,
        },
        ..Default::default()
    }
    .into(), // convert it to `Text`
    ..Default::default()
});

// Updating properties
for mut text in text_query.iter_mut() {
    // this no longer works
    // text.value = "hey hey".to_string();

    // instead, update the only section
    text.sections[0].value = "hey hey".to_string();
}

// Rich text with multiple sections:
commands.spawn(TextBundle {
    // use `Text` directly
    text: Text {
        // construct a `Vec` of `TextSection`s
        text: vec![
            TextSection {
                value: "hello ".to_string(),
                style: TextStyle {
                    font: asset_server.load("fonts/FiraSans-Bold.ttf"),
                    font_size: 40.0,
                    color: Color::WHITE,
                }
            }
            TextSection {
                value: "world!".to_string(),
                style: TextStyle {
                    font: asset_server.load("fonts/FiraMono-Medium.ttf"),
                    font_size: 60.0,
                    color: Color::GREEN,
                }
            }
        ],
        ..Default::default()
    },
    ..Default::default()
});

// Updating properties
for mut text in text_query.iter_mut() {
    text.sections[0].value = "hi ".to_string();
    text.sections[1].value = "bevy".to_string();
    text.sections[1].style.color = Color::BLUE;
}

[rparrett]

FWIW, my project is working great with this PR as-is. Was able to remove ~100 LOC and my text is no longer wiggly!

[cart]

Thanks for putting in the work here! This is definitely a huge step forward in usability. In general this all looks great. The only thing I want to discuss is the "single section text construction api". Its worth doing a side by side comparison of some of our options:

1. Raw (376 characters)
   • Smallest api surface. Adding new sections is straightforward and requires no refactors / type changes. A bit more typing than other options. "Idiomatic bevy struct initialization"
2. From<TextSection> (387 characters)
   • Works decently well for simple cases: text: TextSection {...}.into(), but gets nasty when specifying Text fields like alignment
3. Builder (320 characters)
   • Slightly larger api surface. In Bevy we try to avoid builders as much as possible. I personally prefer (1) because it is more idiomatic (from a Bevy perspective) and requires a smaller api surface
4. New Type (330 characters)
   • I am personally against this because it increases api surface for what I consider to be dubious benefit. I've seen the BasicX / SimpleX pattern in other engines (like Amethyst) and it has always sort of rubbed me the wrong way. Apis should be basic/simple by default. Adding a new type to make them "simple" is:
     • Not simple. you now need to learn about a new type and how it converts to the type you actually care about
     • Potentially an indicator of underlying problems in the api (ex: its too complicated for normal use cases). I don't think this is the case for the Text api, I'm just calling this out as a general thing.
5. Constructor (302 characters)
   • This is the smallest amount of typing and requires no new types. The biggest tradeoff is that we aren't using the preferred "struct initializer" pattern. I personally prefer this to (4) as it keeps the api surface low, feels more "discoverable", is more ergonomic, and doesn't use terms like "basic" or "simple".

I think my personal preference is (1) + (5). People that want shorthand can use the constructor and people that want to be "idiomatic" or want to avoid unnecessary refactors when new sections are added can use the initializer syntax.

[rparrett]

I'm not personally bothered with either way, but is (4) viable if the current Text is renamed so that BasicText can just be Text?

Being able to support other from impls like MdText, UnityStyledText seemed pretty cool/elegant to me.

edit: Oh, maybe that's not elegant if plugin authors can't write those impls? Bit of a rust newbie over here.

[cart]

I'm not personally bothered with either way, but is (4) viable if the current Text is renamed so that BasicText can just be Text?

It would make that more palatable, but the pattern is still the same. My preference is still for (1) + (5) here. Text is basically a "collection" type. let collection: Collection<T> = Collection::from(SingleItemCollection<T>(T { ... })); just feels way too complicated to me.

Being able to support other from impls like MdText, UnityStyledText seemed pretty cool/elegant to me.

I'm very open to the From<X> pattern in those cases. MdText and UnityStyledText are "new" formats that need to be converted to our Text format. BasicText isn't a new format. Its an alternative (slightly) lower-boilerplate syntax for constructing a single item version of Text.

Oh, maybe that's not elegant if plugin authors can't write those impls? Bit of a rust newbie over here.

That shouldn't be a problem, provided the plugin author has defined the MdText type in their crate. You just can't impl crate-external traits on crate-external types.

[tigregalis]

In general, I don't have any strong preferences with respect to the "single section text construction api", so I'll happily defer to those with stronger opinions on it.

However, some comments on each:

(1) This approach will always be possible anyway, with this implementation, so I think the question is only whether we consider it idiomatic and use it in examples and documentation.

(2) This feels strange to me. But I don't mind the idea of converting from a TextSection to a Text in general.

(2.5) However, for convenience you might provide a method like with_alignment.

This would look like:

Text::from(TextSection {
  value: "hello world!".to_string(),
  style: TextStyle {
    font: asset_server.load("fira.ttf"),
    font_size: 60.0, 
    color: Color::WHITE,
  }
})
.with_alignment(TextAlignment {
  vertical: VerticalAlign::Center,
  horizontal: Horizontal::Center,
})

If you had this though, you'd likely also want to do the reverse (start with the alignment):

Text::from(TextAlignment {
  vertical: VerticalAlign::Center,
  horizontal: Horizontal::Center,
})
.with_section(TextSection {
  value: "hello world!".to_string(),
  style: TextStyle {
    font: asset_server.load("fira.ttf"),
    font_size: 60.0, 
    color: Color::WHITE,
  }
})
// and now this is exactly the same as (3) but with `new` renamed to `from`

Do we want with_alignment and with_section methods regardless? Then another approach would look like:

Text::default()
.with_alignment(TextAlignment {
  vertical: VerticalAlign::Center,
  horizontal: Horizontal::Center,
})
.with_section(TextSection {
  value: "hello world!".to_string(),
  style: TextStyle {
    font: asset_server.load("fira.ttf"),
    font_size: 60.0, 
    color: Color::WHITE,
  }
})

The main benefit of this approach is that it extends to multiple section text very nicely:

Text::default()
.with_alignment(TextAlignment {
  vertical: VerticalAlign::Center,
  horizontal: Horizontal::Center,
})
.with_section(TextSection {
  value: "hello world!".to_string(),
  style: TextStyle {
    font: asset_server.load("fira.ttf"),
    font_size: 60.0, 
    color: Color::WHITE,
  }
})
.with_section(TextSection {
  value: " and goodbye...".to_string(),
  style: TextStyle {
    font: asset_server.load("fira.ttf"),
    font_size: 30.0, 
    color: Color::RED,
  }
})

(3) I'm comfortable with builders (see above) but understand that it largely isn't idiomatic in Bevy land.

(4) To be honest, I agree with cart's comments here. The part I like the least actually is that there is a difference between how you populate the text initially Text { value } and how you later retrieve the text text.sections[0].value. The fact that you are explicitly converting the type using into or from does mitigate this to some extent, but I can see it being potentially confusing initially (i.e. a user may initially try text.value).

(5) I do think that this is the nicest approach, but hadn't considered it at the time as constructors didn't feel Bevy-like to me at the time: Bevy encourages and makes use of struct literal syntax everywhere; in retrospect this is not strictly the case, when you consider another complex component Transform which has a number of constructors. I think that if we chose this API, I'd wish to be more explicit in terms of the function name, e.g. Text::single Text::one Text::single_section Text::one_section or something along those lines. Otherwise, you might initially conclude that Text::section() returns a TextSection. And it still doesn't use terms like "basic" or "simple" and so doesn't treat it as "special" (or more accurately, "distinct" from Text).

[cart]

2.5) I don't personally see the builder pattern as a win over the ..Default pattern here.

If we have decided that this is acceptable for the "single section api":

Text::default()
    .with_alignment(TextAlignment {
        vertical: VerticalAlign::Center,
        horizontal: Horizontal::Center,
    })
    .with_section(TextSection {
        value: "hello world!".to_string(),
        style: TextStyle {
            font: asset_server.load("fira.ttf"),
            font_size: 60.0, 
            color: Color::WHITE,
        }
    })

Then imo this is also acceptable (and preferable):

Text {
  alignment: TextAlignment {
    vertical: VerticalAlign::Center,
    horizontal: Horizontal::Center,
  }),
  sections: vec![TextSection {
    value: "hello world!".to_string(),
    style: TextStyle {
      font: asset_server.load("fira.ttf"),
      font_size: 60.0, 
      color: Color::WHITE,
    }
  }]
}

The main allure of (5) is that you don't need to think about the TextSection type at all. I think the desire for the simplicity of the old api is whats motivating this ergonomics discussion, and so far only (4) and (5) have handled that.

I think we should either embrace the extra boilerplate and use (1) everywhere, with the benefit being that the api is consistent across all use cases. Or use (5) (or something like it) for single-text sections to re-capture the simplicity of the old api.

For (5) naming, maybe we should roll with the rust with_more_details constructor naming convention. So in this case Text::with_section(text, style, alignment).

[cart]

Sounds like we're in agreement then. Lets replace BasicText with Text::with_section() and get this merged!

[tigregalis]

Done!

Edit: I need to rebase and update the new example. I'll get to it shortly.

[cart]

Looks good to me. Thanks again 😄