Cosmic text

[TotalKrill]

Fixes #7616.

Rebase of https://github.com/bevyengine/bevy/pull/8808

This needs all the information from that PR, as well as an update to cosmic-text 0.10.0

but this compiles and can run the ui/text.rs example, and thats about it.

Left image is 15c54b55427ef340d4d1616b165a98c9e5658c32 Right image is 8e3de7b73f6580e11b7206172b637aa0cbc0f93e

LeftOver TODOs:

// TODO: TextPipeline: cache buffers / store buffers on the entity // TODO: TextPipeline: solve spans with different font sizes, see https://github.com/pop-os/cosmic-text/issues/64 // TODO: TextPipeline: reconstruct byte indices for keeping track of eventual cursors in text input // TODO: TextPipeline: (future work) split text entities into section entities // TODO: TextPipeline: (future work) text editing // TODO: [crates::bevy_text::Font::from_bytes] font validation // TODO: Remove mutext from FontSystem the only reason we need a mutex is due to TextMeasure // TODO: TextPipeline::create_buffer: Support multiple section font sizes, pending upstream implementation in cosmic_text For now, just use the first section's size or a default // TODO: Support line height as an option. Unitless 1.2 is the default used in browsers (1.2x font size). // TODO: TextPipeline cache textbuffers see (glyphon/iced)

[TotalKrill]

@tigregalis I must have messed up somethign in the rebasing and everything, and I absolutely could not figure out how to fix it.

So basically, made an entirely new commit, containing all the changes in this branch, mashed into one. To make it possible for me to just keep the branch rebased onto main.

If you want some credit on the commit, do you have any suggestions on how to fix this? you can find the branch here https://github.com/TotalKrill/bevy/tree/cosmic-text-fix-githistory

The only way someone, including me can continue work on this is if the mess i made of the git-history is fixed, and this is the only way I could figure out how to do it

[nicoburns]

@TotalKrill https://stackoverflow.com/questions/7442112/how-to-attribute-a-single-commit-to-multiple-developers

[TotalKrill]

Hello!

I am having less time than anticipated ( sorry ) which means that adopting this might have been premature from my side.

However, changing to the cosmic-text crate from the current one would be beneficial, since cosmic text seems to become the rust text standard.

Seeing this, I am seeing that the started port tried to implement a few new features, and thus changed quite a bit of the original code. Since I did not write the first implementation, is there a way we could reduce the scope of this pull request, to get cosmic-text into bevy, for the reason to enable further improvements that cosmic-text CAN provide.

If that is an option, could someone help me with defining a clear goal of what a minumum acceptable pull request for this would be? @alice-i-cecile @tigregalis @nicoburns ?

[alice-i-cecile]

If that is an option, could someone help me with defining a clear goal of what a minumum acceptable pull request for this would be? @alice-i-cecile @tigregalis @nicoburns ?

Personally, I think we should go with a very minimal or incremental approach. As far as I can tell, we have consensus that "we should move to cosmic-text". Rewrites shouldn't block that, and even minor regressions in functionality (like multiple fonts of different sizes in the same text section) shouldn't either. Improvements don't have to be monotonic.

There are two ways to tackle that IMO:

1. Rip out the existing backend and swap to cosmic-text in a single PR.
2. Add an alternate backend controlled by a feature flag, which we iterate on across a series of PRs.

If we can do it in less than say 1k lines of code, I feel the former is the way. Regardless, my preference is to merge the migration early in the cycle and iterate to catch bugs and rearchitect as needed.

[nicoburns]

Copying from discord (there is also some more chat about text around that point in the channel):

I believe it needs:

• https://github.com/pop-os/cosmic-text/issues/42#issuecomment-1607731931 / https://github.com/pop-os/cosmic-text/issues/70 for correct alignment
• https://github.com/pop-os/cosmic-text/pull/150 for mixed text sizes (which I believe existing bevy text supports).

I can see a case that regressing on multiple font sizes could be acceptable (although I'm always quite loathe to break things that users might quite reasonably be depending on), but I feel like breaking text alignment wouldn't be?

And it could also do with a new Taffy release, which will allow the PR to remove a Mutex, but that's probably not blocking.

[tigregalis]

My general view is that the rebase was the hard part so this is 90% done for the MVP.

Seeing this, I am seeing that the started port tried to implement a few new features, and thus changed quite a bit of the original code. Since I did not write the first implementation, is there a way we could reduce the scope of this pull request, to get cosmic-text into bevy, for the reason to enable further improvements that cosmic-text CAN provide.

A lot of the original code was tied to the glyph_brush implementation, so swapping that out for cosmic-text necessitated rewriting most of pipeline.rs in particular, and everything else it touched. Feature-wise I think the only changes I made were being able to load system fonts and font references (so that those systems fonts could be supported), but I think those features are basically complete.

• Request: Allow buffer dimensions to be undefined pop-os/cosmic-text#42 (comment) / Let a Buffer figure out its height during set_size pop-os/cosmic-text#70 for correct alignment

The current code in this PR fakes correct alignment by using cosmic-text's left alignment (which is not broken), then just "visually" adjusts the glyphs in the line based on the calculated width.

Code

                // TODO: use cosmic text's implementation (per-BufferLine alignment) as it will be editor aware
                // see https://github.com/pop-os/cosmic-text/issues/130 (currently bugged)
                let x = x + match text_alignment {
                    TextAlignment::Left => 0.0,
                    TextAlignment::Center => (box_size.x - line_w) / 2.0,
                    TextAlignment::Right => box_size.x - line_w,
                };

Could use some testing but I think it works. This wouldn't work if we wanted to support the Editor abstraction (which I think we eventually do, but we'd have to use cosmic-text's alignment properly) but if we're reducing the scope we can probably merge it as is.

• WIP Multiple font size / line height spans pop-os/cosmic-text#150 for mixed text sizes (which I believe existing bevy text supports).

To be honest, the current state of the mixed font sizes PR for cosmic-text would work for Bevy in its current state: it correctly sizes and positions the glyphs. Cosmic-text does a little more than that though, and primarily that's the Editor abstraction. I'm not sure what Bevy's policy is with (temporarily) vendoring dependencies? What's left in that PR is to finalise parts of it in ways that aren't relevant to this PR but are relevant to cosmic-text: the Editor abstraction is currently a little broken, and migrating all of the tests and examples; got stuck on a couple of those.

If acceptable a roadmap for this PR would be:

• [ ] Accept having a single font size, and migrate to cosmic-text 0.10.0
• [ ] Alternatively, vendor the multiple font sizes PR, which is based on latest cosmic-text, then restore multiple font size functionality in the Bevy impl
• [ ] Adapt TextPipeline::create_buffer to use Buffer::set_rich_text from 0.10.0 and throw away our implementation.
• [ ] fontdb::Database::load_font_source now returns the ID so use it
• [ ] Create some issues to circle back to the to-dos

[TotalKrill]

I agree with the incremental implementation approach, and thus preferably I would like this commit to ONLY migrate to cosmic text. Event thought the system fonts did look almost finished.

The API for using the system fonts did not really feel very "Bevy", so I put the removal of that feature in a separate commit, so that it could be easily re-added in another PR after the backend switch feels okay.

Then I used the Buffer::set_rich_text from cosmic text, and removed some warnings.

When I move over to load_font_source, I believe we can start looking at a path to ensure the code quality for merging. With as minimal a change as can be possible.

[TotalKrill]

tested the performance in text_debug, and there was a 10x slowdown when running with Shaping::Advanced, compared to Shaping::Basic. Not a big fan of the regression of performance.

	Bevy	Cosmic Text Shaping::Basic	Cosmic Text Shaping::Advanced
--release	~2400fps	~2200fps	~1000fps
--debug	~200fps	~200fps	~20fps

With this test, I would suggest using Shaping::Basic for now

[TotalKrill]

I think the last part of this, might be what to do with all the sprinkled TODOs left in the codebase. I am personally a bit ambivalent, I do like them, since it would probably guide someone looking to implement additional features.

Also, maybe a new PR for the system font feature as well, since I suspect a merge would just squash these commits.

[alice-i-cecile]

I think the last part of this, might be what to do with all the sprinkled TODOs left in the codebase. I am personally a bit ambivalent, I do like them, since it would probably guide someone looking to implement additional features.

I'm fine to leave them. If we remove them, we should make an issue or ten.

Also, maybe a new PR for the system font feature as well, since I suspect a merge would just squash these commits.

Agreed. I would like that in a different PR.

[ickshonpe]

tested the performance in text_debug, and there was a 10x slowdown when running with Shaping::Advanced, compared to Shaping::Basic. Not a big fan of the regression of performance.

Bevy Cosmic Text Shaping::Basic Cosmic Text Shaping::Advanced --release ~2400fps ~2200fps ~1000fps --debug ~200fps ~200fps ~20fps With this test, I would suggest using Shaping::Basic for now

Any idea why it's so expensive, what does the advanced text shaping give us?

[nicoburns]

Any idea why it's so expensive, what does the advanced text shaping give us?

I believe it's using RustyBuzz instead of Swash for shaping + doing proper unicode line breaking. I think it gives us support for things like non-latin text and ligatures. I'm not 100% sure on the details, but it's something like that. It should only affect FPS like that for constantly changing text, as shaping only needs to happen once per string, and isn't required for relayouts (although I'm not sure if the current implementation enables this).

Iced added Shaping::Basic specifically for debug mode performance (I don't think it's intended for shipping to end users). But IMO we'd be better off just always compiling deps in release mode. Taffy is also super-slow in debug mode, and I'm sure a lot of the rendering stuff is too.

[nicoburns]

I imagine Shaping::Basic would be a good tradeoff for some games / parts of games that know that they will only be rendering really basic text though. Perhaps it could be exposed as an option on a per-text-node basis?

[TotalKrill]

Yeah I can definitely see the benefits of having advanced shaping for specific use cases.

However, I would also argue that for changing backend, this would be enough, and I rather err on the side of performance :)

One further PR could be to discuss adding a feature gate for the shaping methods. But again, I would rather put that in another PR, where discussions of why it is so slow, and what it gives us could be considered in further depth.

Since it's quite Low code / potentially high impact

On Mon, Nov 13, 2023, 16:38 Nico Burns @.***> wrote:

I imagine Shaping::Basic would be a good tradeoff for some games / parts of games that know that they will only be rendering really basic text though. Perhaps it could be exposed as an option on a per-text-node basis?

— Reply to this email directly, view it on GitHub https://github.com/bevyengine/bevy/pull/10193#issuecomment-1808399406, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYY7BXGGRGXYE62SCBMFCDYEI5F3AVCNFSM6AAAAAA6H4OQLGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBYGM4TSNBQGY . You are receiving this because you were mentioned.Message ID: @.***>

[TotalKrill]

Seems there are now merge conflicts, I consider this ready to merge, with follow up PRs preferably done after this has been merged.

Even thought it's gonna be squashed ehne merged, I assume that my branch on my fork will not be affected, so that code I removed can be collected from there afterwards

[alice-i-cecile]

Even thought it's gonna be squashed ehne merged, I assume that my branch on my fork will not be affected, so that code I removed can be collected from there afterwards

Correct :)

[rparrett]

Are multiple text colors in a single bundle also not supported?

In the example below, both sections are the same size (understandably, with current cosmic-text limitations).

But they are also both green, which I would hope would work.

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    // UI camera
    commands.spawn(Camera2dBundle::default());

    commands.spawn(TextBundle::from_sections([
        TextSection::new(
            "S1",
            TextStyle {
                font_size: 60.,
                color: Color::GREEN,
                font: asset_server.load("fonts/FiraSans-Bold.ttf"),
            },
        ),
        TextSection::new(
            "S2",
            TextStyle {
                font_size: 30.0,
                color: Color::RED,
                font: asset_server.load("fonts/FiraSans-Bold.ttf"),
            },
        ),
    ]));
}

[alice-i-cecile]

Labelling as Controversial: until we have a fix for https://github.com/pop-os/cosmic-text/issues/64 we're taking a pretty reasonable sizable of functionality. Might be the right choice, but not one I would want to make unilaterally.

[TotalKrill]

Thank you for taking the time to review this! I have rectified all the easy things. Responded to the comment with a :+1: to indicate I did it.

Others I left a comment on.

[rparrett]

It would be nice to add "bevy with the subpixel_glyph_atlas feature" into the benchmarks -- as far as I know this is something that cosmic-text is doing by default.

[alice-i-cecile]

Looking better :) I resolved conversations for you, and left a couple more notes.

[TotalKrill]

Are multiple text colors in a single bundle also not supported?

In the example below, both sections are the same size (understandably, with current cosmic-text limitations).

But they are also both green, which I would hope would work.

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .run();
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    // UI camera
    commands.spawn(Camera2dBundle::default());

    commands.spawn(TextBundle::from_sections([
        TextSection::new(
            "S1",
            TextStyle {
                font_size: 60.,
                color: Color::GREEN,
                font: asset_server.load("fonts/FiraSans-Bold.ttf"),
            },
        ),
        TextSection::new(
            "S2",
            TextStyle {
                font_size: 30.0,
                color: Color::RED,
                font: asset_server.load("fonts/FiraSans-Bold.ttf"),
            },
        ),
    ]));
}

I also think that should work? in the examples/ui/text.rs, i fI explicitly set the colors to RED and GOLD in those sections, i get the expected result. But when building your example in another repo outside of the examples, both texts are green....

This seems to be a bug in cosmic-text, since if I use shaping::Adanced, it works as expected...

[viridia]

Q: Are there plans to support "font families", in other words, the ability to select the correct font asset based on style options? As opposed to what we have now: if you want to draw text in italic you have to pick an italic font.

See https://github.com/bevyengine/bevy/issues/9725

[tigregalis]

Q: Are there plans to support "font families", in other words, the ability to select the correct font asset based on style options? As opposed to what we have now: if you want to draw text in italic you have to pick an italic font.

See #9725

that would be supported by loading multiple fonts (e.g. loading an entire font family, or loading system fonts), and the FontQuery API. i don't recall if FontQuery was removed or not for this initial implementation as part of the system fonts feature that was (temporarily) removed, but my original implementation did have this working if not battle-tested

[TotalKrill]

I removed everything regarding font families and system fonts in one (maybe two) PRs, to make it easier to make into a separate PR directly after this had landed.

My plan was to decontroversialize this as much as possible, to get cosmic-text in, since I believe the future potential with cosmic-text is better adapted for adding things such as font families etc.

[viridia]

@TotalKrill I am in 100% agreement with that approach. Small bites.

We can talk about font-families later. I think that it should be something that can be defined either in code or as an asset, and in asset form can be something simple like a RON to TOML file.

[nicoburns]

I have picked up the multiple font size / line height work in cosmic-text. There is a new PR (which is rebased on the latest cosmic-text main branch) here: https://github.com/pop-os/cosmic-text/pull/235. There's still quite a bit to sort out, but I'm going to make it a priority to get this work done so hopefully we will see relatively fast progress.

[nicoburns]

@TotalKrill Btw, cosmic-text are claiming greatly improved shaping performance in 0.11, so it might be interesting to upgrade this PR to the new version and re-run the benchmarks you ran before if you have time.

[TotalKrill]

Did a quick check on this, I get stuck on the changes in the TextureAtlas and TextureAtlasLayout

When I last checked this, the TextureAtlas contained the handle to the image it was using to draw the textureatlas information on.

Do anyone know where I should be looking for the information about which Handle<Image> is used by the Atlas, I am assuming they have to be paired somewhere else.

in this PRs code, I am lacking the information at crates/bevy_text/src/text2d.rs:141 when trying to go get it working in main.

Edit: I figured it out, added the texture information to GlyphAtlasInfo

[TotalKrill]

Reran the benches and it is indeed very much improved

Cosmic Text 0.10

	Bevy@c641882cf	Cosmic Text Shaping::Basic	Cosmic Text Shaping::Advanced
--release	~1300fps	~2200fps	~1100fps
--debug	~60fps	~190fps	~20fps

Cosmic Text 0.11

	Bevy@c641882cf	Cosmic Text Shaping::Basic	Cosmic Text Shaping::Advanced
--release	~1300fps	~2200fps	~1700fps
--debug	~60fps	~190fps	~90fps