Refactor: use glyph_brush_layout and add text alignment support

[AlisCode]

Context

The current state of bevy_text is :

• ab_glyph for font loading
• A custom, hand-made implementation of text layout with some of the code coming out of the ab_glyph examples
• No standard way to measure text

Content

This PR introduces a dependency to glyph_brush_layout in bevy_text, and replaces the current implementation of text layout (which is done in the DrawableText's implementation of Drawable) with a more robust solution (e.g. using a crate that's meant to do that).

Since we're using glyph_brush_layout, we get text alignment for free in bevy_ui, so that's implemented also :) more on that later.

I'm exposing a TextPipeline. It is a public abstraction that allows to launch all text-related operations on a GlyphBrush, where GlyphBrush has been reimplemented for bevy's specific needs. Operation on a TextPipeline could potentially be launched from different threads, and include :

• Queing text
• Pre-rendering text in the queue (so, computing its layout)
• Measuring text

The idea is that TextPipeline should be the public interface and encapsulates operations on the GlyphBrush, making sure we dont require a breaking change when we refactor it in the future for some reason (namely to add caching).

Goal

In short, this PR allows for standardized text layout and text measurement implemented in bevy_text. It would then be usable in the whole bevy ecosystem. I see two main use cases here :

• Alternative UI system (imgui, iced are two examples that comes to mind)
• In-game text

Caveats

No caching

We'd have to see if caching is something worth doing or not : right now, whenever the text or its style changes, its layout is recomputed. This is awful for things such as animating the color of a text. I originally thought TextAlignment would be worth storing in the text style, but a change of text style might not need a whole text layout recompute, while a change of text alignment definitely would.

The fact that we're adding a field to the TextStyle component effectively makes this PR a breaking change; It might be worth refactoring TextAlignment to use its own component. But the problem remains for the text size which is contained in TextStyle. Changing a text size will of course require to recompute all its glyphs.

bevy_ui's Val::Percent variant

Right now, the usage a Val::Percent to apply a constraint on the size of a textblock in min_size, size or max_size will be ... Ignored. We might need to use the parent's constraint to compute the percentage, I'm not exactly sure how to handle that.

Rationale

It's also possible to use another text engine like Fontdue or something. But since bevy currently uses ab_glyph it only makes sense to use glyph_brush_layout since it's based off ab_glyph, and actually written and maintained by the same author.

Thanks ❤️

@tigregalis for doing most of the work on glyph positioning
@verzuz for the help in debugging this

[cart]

Lazy question that I could answer myself when I eventually review this pr: does this approach resolve #283?

[mockersf]

yes, yes it does

[enfipy]

Hey, I tried to rebase this PR branch on bevy master and now it just doesn't show any text. Maybe you have some ideas?

[AlisCode]

@enfipy I rebased on master, fixed the problem and force-pushed.

CI is broken but that seems to come from bevy_ecs ? Not sure there's anything I can do for that.

[cart]

Hmm yeah that's a problem. I'll fix that now.

[cart]

Reviewing now btw!

[cart]

(master build is fixed)

[cart]

can we move this alongside the rest of the ui examples? i like to keep these sorted

[cart]

It looks like text now has a small amount of extra "left-padding" now (master on top, this pr on bottom)

Any clue why that is?

[cart]

Here is how it renders in firefox, which makes me think the first one is correct (red bounds are a div container):

[tigregalis]

It's the side bearing that has been subtracted from min_x making the total text rectangle larger as well as increasing the distance from the left, because (if ab_glyph uses the same methodology), the origin already "includes" the side bearing so this has doubled up:

taken from: http://mirror.informatimago.com/next/developer.apple.com/documentation/mac/Text/Text-186.html

i.e. for any glyph:

// left edge
min_x = origin_x;
// right edge
max_x = origin_x + advance;
// top edge
min_y = origin_y + ascent; // because down is positive in ab_glyph, this becomes `origin_y - ascent`
// bottom edge
max_y = origin_y - descent; // down is positive in ab_glyph, so this would be `origin_y + descent`, however, descent is already provided as a negative value in ab_glyph, so it would be `origin_y + -descent`

---

Simply removing - scaled_font.h_side_bearing(glyph.id) from glyph_brush.rs and pipeline.rs appears to fix it.

---

Before:

After:

48px font (bevy/ab_glyph) equivalent to 40px font / 30pt font (CSS/Word)
400% zoom
background image is bevy with white text
foreground image is Word with red text (confirmed behaves the same in Firefox)

[cart]

lets just remove new() here in favor of default()

[AlisCode]

I removed new since you're in charge of the style of this codebase so I'm assuming that we should keep it symetrical with other bevy APIs. I would like to mention this though :

https://rust-lang.github.io/api-guidelines/interoperability.html?highlight=expected#types-eagerly-implement-common-traits-c-common-traits

Note that it is common and expected for types to implement both Default and an empty new constructor. new is the constructor convention in Rust, and users expect it to exist, so if it is reasonable for the basic constructor to take no arguments, then it should, even if it is functionally identical to default.

[cart]

This is one of the few places where my opinions diverge from the api guidelines. I prefer "default only" because:

• it means theres "one way" to instantiate the type (a good thing ... lowers cognitive load)
• less code (easier to read, easier to maintain, marginally faster to compile)

[cart]

This doesn't work how it used to. It stops adding new glyphs to the atlas after a certain (random) point in time. Probably because it hits a random character that breaks part of the the glyph generation process?

[AlisCode]

This example was breaking because the size field of the text component's style was set to be [250px, 60px].

The size is being used to limit the bounds that glyph_brush_layout generates the glyphs in, and its behavior in this case is to ... Not generate glyphs that are out of bounds (probably by early returning btw, I haven't checked the sources).

I fixed the example by removing the size of the component, I do not think that this is a bug per se.

[cart]

yeah that that seems like a desirable property. carry on!

[cart]

bevy uses parking_lot mutexes elsewhere. can we switch to that?

[cart]

we can remove redundant code here by replacing this section with f32::MAX/MIN values:

let mut min_x = std::f32::MAX;
let mut min_y = std::f32::MAX;
let mut max_x = std::f32::MIN;
let mut max_y = std::f32::MIN;

[cart]

do these fields need to be public? otherwise its probably better to make them private to prevent accidental misuse.

[cart]

It seems like "vertex" isn't the right name for this. Vertices are points that make up a mesh, but this seems more like a PositionedGlyph.

[cart]

Not necessarily something we need to do in this pr, but it would be interesting to try to remove allocations here entirely and just walk through each glyph + render it directly

[AlisCode]

I do not think that this is feasible with the new design: we essentially store the TextGlyphs inside an entity now, and iterate over them when rendering. Collection needs to happen at some point, I believe.

[cart]

i think we can remove this function (and get_char_index() and glyph_to_index)

[razaamir]

www.medium.com

On Sat, Nov 7, 2020 at 4:43 PM tigregalis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In examples/ui/text.rs <#765 (comment)>: > @@ -42,6 +42,7 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) { style: TextStyle { font_size: 60.0, color: Color::WHITE, + ..Default::default() It's the side bearing that has been subtracted from min_x making the total text rectangle larger as well as increasing the distance from the left, because (if ab_glyph uses the same methodology), the origin already "includes" the side bearing so this has doubled up: <https://camo.githubusercontent.com/8324e13b1d96a41f92eddfaed92911c6dc1db8c3f39bbf5580a244508e6f8f48/687474703a2f2f6d6972726f722e696e666f726d6174696d61676f2e636f6d2f6e6578742f646576656c6f7065722e6170706c652e636f6d2f646f63756d656e746174696f6e2f6d61632f546578742f67726170686963732f464d2d30312e6a7067> taken from: http://mirror.informatimago.com/next/developer.apple.com/documentation/mac/Text/Text-186.html i.e. for any glyph: // left edge min_x = origin_x; // right edge max_x = origin_x + advance; // top edge min_y = origin_y + ascent; // because down is positive in ab_glyph, this becomes `origin_y - ascent` // bottom edge max_y = origin_y - descent; // down is positive in ab_glyph, so this would be `origin_y + descent`, however, descent is already provided as a negative value in ab_glyph, so it would be `origin_y + -descent` ------------------------------ Simply removing - scaled_font.h_side_bearing(glyph.id) from glyph_brush.rs and pipeline.rs appears to fix it. ------------------------------ Before: [image: image] <https://user-images.githubusercontent.com/38416468/98439463-f35a6f80-212c-11eb-8474-3a9afb3aa1d0.png> After: [image: image] <https://user-images.githubusercontent.com/38416468/98439840-944a2a00-212f-11eb-91dd-38f6e37f8b32.png> *48px font (bevy/ab_glyph) equivalent to 40px font / 30pt font (CSS/Word) 400% zoom background image is bevy with white text foreground image is Word with red text (confirmed behaves the same in Firefox)* — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#765 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHGK5TH7UBM4H2I23LC4BS3SOUXFFANCNFSM4TGZBCVQ> .

-- Raza Amir

[AlisCode]

@cart Thanks for taking the time to review this :)

All comments are adressed.

I have one concern regarding the Mutex on section_queue in glyph_brush : it is essentially useless for now. We need to determine whether queing text from different threads is a goal or not (but I dont think it is, at least for now).

In my opinion :

• Measuring text shouldn't require mutable access to the TextPipeline since it is done "in one go" and cannot possibly collide with an operation coming from an other thread.
• Queuing text and processing it using the TextPipeline should require mutable access, because we must not have two threads queuing sections in parellel, that would likely be no bueno.

If we agree on that, I'll make it so.

[enfipy]

@AlisCode There's sometimes the error NoSuchFont when the font not loaded before render. This error appeared only on this PR's fork. When loading font on the resource stage - everything works.

[tigregalis]

@AlisCode There's sometimes the error NoSuchFont when the font not loaded before render. This error appeared only on this PR's fork. When loading font on the resource stage - everything works.

I ran into this as well on the first run for the text_debug example, but wasn't able to reproduce it in subsequent runs.

[AlisCode]

@enfipy @tigregalis

Yep, thanks for noticing ; that would be because I removed the QueuedText local resource from text_system, thinking we did not need it anymore. Turns out I was wrong, and I now understand why. I'll re-add it. Re-scheduling the measurement/layout computing sounds like better error handling than the println! that's currently in the PR.

Any comment on the mutability concern around TextPipeline I mentioned ?

[tigregalis]

because we must not have two threads queuing sections in parellel, that would likely be no bueno

@AlisCode Why is this? I'm not sure whether queuing text, and processing it, can't be done from multiple threads. At the entity/component level they shouldn't conflict, at least.

It's just that when you're queuing, you're touching the same resources, writing to the same atlases or potentially inserting to maps under the same keys. I don't think the order things get added matters, so that shouldn't be a concern.

Having said this, right now, it looks like, for any given entity, we're adding its text to the queue, then immediately removing it from the queue to process it, because we need to return the PositionedGlyphs specific to this entity. The queue isn't really doing anything. I think this is because the original intent was that the GlyphBrush would do the drawing, as that's what the original glyph_brush crate was for.

On that particular topic, we could combine the measure, queue_text and process_queued functions on TextPipeline into a single function (let's just call this queue_text), as we're effectively doing all of these operations sequentially without ever "yielding" so to speak. Additionally, we could let the TextPipeline/GlyphBrush end up handling the drawing at the end. Here's what I imagine:

• in text_system in bevy_ui::stage::UI:
  • for each entity, TextPipeline::queue_text:
    • calculates the SectionGlyphs,
    • calculates the size,
    • builds the OutlineGlyphs,
    • constructs the *Atlases and Textures (ideally support this from multiple threads)
    • calculates the PositionedGlyphs
    • it inserts (overwriting) these onto a queue: HashMap<Entity, Vec<PositionedGlyph>> (ideally support this from multiple threads)
  • The only output from queue_text back into the ECS (text_system) is size, i.e.

    if let Ok(size) = text_pipeline.queue_text(entity, /* ... */) {
        calculated_size.size = size;
    }

  • Otherwise, all the state is held within the TextPipeline/GlyphBrush.
  • Within queue_text you could make extensive use of caching and memoisation.
• in draw_text_system in bevy_render::stage::DRAW:
  • for each entity, run some new function e.g. TextPipeline::draw_text, and pass in all of:

    mut context: DrawContext,
    msaa: Res<Msaa>,
    mut render_resource_bindings: ResMut<RenderResourceBindings>,
    mut asset_render_resource_bindings: ResMut<AssetRenderResourceBindings>,

    as well as:

    entity: Entity,
    position: Vec3,
    draw: Draw,
    color: Color,

    i.e.

    let text_pipeline = text_pipeline.draw_text(
        &mut context,
        &msaa,
        &mut render_resource_bindings,
        &mut asset_render_resource_bindings,
        entity,
        position,
        &mut draw,
        color,
    );

    • This function would then
      • pull the PositionedGlyphs from its queue for that particular entity,
      • construct the DrawableText for the entity, and call the DrawableText::draw() method
  • You could probably change the order of this a bit and process it in a batch.

[AlisCode]

@tigregalis

I do agree with the first part, including this :

Additionally, we could let the TextPipeline/GlyphBrush end up handling the drawing at the end

While I agree with the concept of letting bevy_text's TextPipeline store the glyphs, the solution described afterwards would - in my opinion - work well for bevy_ui only. My intent with this PR was also to allow non-bevy_ui approaches to text rendering, such as iced and imgui, where using multiple Entities doesn't make sense (I have some ideas on how to integrate with an ECS for those framework, but that's beside the point).

If we want to go this way and make the TextPipeline modular, in my opinion, we should have bevy_text store in a HashMap (as you mentioned) any identifier for the text, as a generic, and the value would be the PositionedGlyph. That way bevy_ui can set this to be an Entity, and alternative UI backends can set this to be any kind of relevant information/identifier they want.

[tigregalis]

@AlisCode

If we want to go this way and make the TextPipeline modular, in my opinion, we should have bevy_text store in a HashMap (as you mentioned) any identifier for the text, as a generic, and the value would be the PositionedGlyph. That way bevy_ui can set this to be an Entity, and alternative UI backends can set this to be any kind of relevant information/identifier they want.

Yeah, I like it.

How straightforward do you think it would be to have "in-game" sprite text for 2d and 3d games?

[AlisCode]

I dont think it will exactly be straightforward : 2D text will indeed be very simple since it does not differ from rendering on the UI, we can essentially reuse DrawableText, while 3D text will require rendering to a texture.

I'm not sure if bevy supports that sort of thing out of the box (but I would assume not), I also dont know if there's a correct way of supporting this (maybe it's just as stupid as copying pixels to the render texture).

In any case, I'm not sure where this should go. Using a generic on TextPipeline means that any UI plugin (bevy_ui or other) will have to define their TextPipeline resource. Should the definition of in-game sprite lie in bevy_ui then ? In which case that means any user using bevy_whatever as their UI system would not benefit from in-game sprite ...

[tigregalis]

For rendering to a texture, I don't understand it 100% but I think that's what's already being done. I might be wildly off the mark, but if I understand correctly it's drawing to a texture which is applied to a quad mesh, and that's actually what gets rendered. On that same note, even in 2d, perhaps it would be useful to provide functionality to bake in certain text onto a newly spawned mesh*, and that mesh would act as any other sprite.
*Though I think this is technically what gets sent to DrawableText.

pub fn draw_text_system(
    mut context: DrawContext,
    msaa: Res<Msaa>,
    meshes: Res<Assets<Mesh>>, <----
    mut render_resource_bindings: ResMut<RenderResourceBindings>,
    mut asset_render_resource_bindings: ResMut<AssetRenderResourceBindings>,
    mut query: Query<(&mut Draw, &Text, &TextGlyphs, &Node, &GlobalTransform)>,
) {
    let font_quad = meshes.get(&QUAD_HANDLE).unwrap(); // <----
    let vertex_buffer_descriptor = font_quad.get_vertex_buffer_descriptor(); // <----

    for (mut draw, text, text_glyphs, node, global_transform) in query.iter_mut() {
        let position = global_transform.translation - (node.size / 2.0).extend(0.0);

        let mut drawable_text = DrawableText {
            render_resource_bindings: &mut render_resource_bindings,
            asset_render_resource_bindings: &mut asset_render_resource_bindings,
            position,
            msaa: &msaa,
            text_glyphs,
            font_quad_vertex_descriptor: &vertex_buffer_descriptor, // <----
            style: &text.style,
        };

        drawable_text.draw(&mut draw, &mut context).unwrap();
    }
}

I think it's fair that TextPipeline<Entity> is inserted in bevy_text::TextPlugin, since the Entity component is a core part of bevy_ecs which underpins the engine. Other plugins that want to make use of TextPipeline<SomeOtherKey> will have to insert their own resource.

As for in-game sprite text, that could be provided via an extra two systems that largely mimic what's in bevy_ui. Those systems could potentially be provided by bevy_text::TextPlugin, as bevy_text already depends on bevy_sprite, but conversely, in-game sprite text should have no requirement on bevy_ui.

[AlisCode]

I dont think this is rendering to a target texture. I think the QUAD_HANDLE is just a handle to a Mesh that's inserted by default in the Mesh assets, and allows to draw .. well, a quad. Without re-allocating every time.

I'm done with the refactor, and found the API to be overall much simpler. Most notably :

• The queue_text function does not return anything. It just computes the glyphs using the GlyphBrush (see below), and stores them in a HashMap owned by the TextPipeline.
• Refactored GlyphBrush so that we now have :
  • compute_glyphs, which gives a Vec<SectionGlyph> (ab_glyph's specific types)
  • process_glyphs, which computes the Vec<PositionedGlyph> given a Vec<SectionGlyph>.
  • The TextPipeline then measures the size of the output, and stores it inside its HashMap, together with the glyphs, forming a TextLayoutInfo.
• GlyphBrush logic is somewhat separated from the TextPipeline. The goal of having ab_glyph's implementation details being abstracted away is reached (the user of a TextPipeline does not need ab_glyph details) which sounds reasonable. I'm not sure where the caching - if we implement caching, that is - needs to be implemented. I suggest we keep that for a different PR. Caching will need benchmarks to show that it is indeed more performant. There's also a lot of possible work GlyphBrush-wise to avoid recomputing the whole layout of a text when we just change e.g. one letter.
• Ho btw, when we fail to add a glyph to the text atlas, we switched from a panic in the font_atlas code to an error that flows to the user of the TextPipeline, who can now handle the error properly (though in most cases, such an error seems to indicate that something is going very wrong)

I still have two issues to tackle (but that should be fairly trivial now) :

• The NoSuchFont error by re-implementing the QueuedText local resource (this is my next commit).
• Right now, the TextPipeline needs to be mutably-borrowed to add text to it. As I previously mentioned, in all theory, we should be able to add text to the queue from multiple threads. This is as simple a refactor as using Mutexes, I believe.

[cart]

These changes all sound good. For now lets not add mutexes. I add rather add them later when they have been shown to be measurably better for common use cases.

[AlisCode]

@cart ack. I'll rebase on master now. Once this is done and since the NoSuchFont error has been fixed, I think this is good for another round of review if necessary :)

I tested that the error was gone by adding (0..10000000).for_each(|_| {}); in the FontLoader so that it takes time to load the font, and the behaviour is correct (No text is shown until the font is effectively loaded, and no crash).

[AlisCode]

All good ! :)

[cart]

Alrighty I think this is good to go! I pushed two small changes:

• removed the unused glyph_to_index hashmap
• removed declarations of text alignment from examples where the size isn't explicitly defined, which means size==text_bounds, which means alignment doesnt matter

[cart]

Thanks for the awesome work! You've resolved a pretty major papercut here 😄

[AlisCode]

Oops I forgot about those things... The minor tweaks are looking good to me :)

Glad I could contribute to something of use for the engine !

Feel free to merge :)