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

Cosmic text #10193

Merged
merged 65 commits into from
Jul 4, 2024
Merged

Cosmic text #10193

merged 65 commits into from
Jul 4, 2024

Conversation

TotalKrill
Copy link
Contributor

@TotalKrill TotalKrill commented Oct 19, 2023

Replace ab_glyph with the more capable cosmic-text

Fixes #7616.

Cosmic-text is a more mature text-rendering library that handles scripts and ligatures better than ab_glyph, it can also handle system fonts which can be implemented in bevy in the future

Rebase of #8808

Changelog

Replaces text renderer ab_glyph with cosmic-text

The definition of the font size has changed with the migration to cosmic text. The behavior is now consistent with other platforms (e.g. the web), where the font size in pixels measures the height of the font (the distance between the top of the highest ascender and the bottom of the lowest descender). Font sizes in your app need to be rescaled to approximately 1.2x smaller; for example, if you were using a font size of 60.0, you should now use a font size of 50.0.

Migration guide

  • Text2dBounds has been replaced with TextBounds, and it now accepts Options to the bounds, instead of using f32::INFINITY to inidicate lack of bounds
  • Textsizes should be changed, dividing the current size with 1.2 will result in the same size as before.
  • TextSettings struct is removed
  • Feature subpixel_alignment has been removed since cosmic-text already does this automatically
  • TextBundles and things rendering texts requires the CosmicBuffer Component on them as well

Suggested followups:

  • TextPipeline: reconstruct byte indices for keeping track of eventual cursors in text input
  • TextPipeline: (future work) split text entities into section entities
  • TextPipeline: (future work) text editing
  • Support line height as an option. Unitless 1.2 is the default used in browsers (1.2x font size).
  • Support System Fonts and font families
  • Example showing of animated text styles. Eg. throbbing hyperlinks

@TotalKrill TotalKrill mentioned this pull request Oct 19, 2023
15 tasks
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets labels Oct 19, 2023
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Oct 19, 2023
@TotalKrill
Copy link
Contributor Author

@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

@TotalKrill TotalKrill force-pushed the cosmic-text branch 2 times, most recently from 5d72240 to 58fce52 Compare October 23, 2023 21:25
@TotalKrill
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

nicoburns commented Nov 10, 2023

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

I believe it needs:

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
Copy link
Contributor

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.

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.

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
Copy link
Contributor Author

TotalKrill commented Nov 12, 2023

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 TotalKrill changed the title Draft: Cosmic text Cosmic text Nov 12, 2023
@TotalKrill TotalKrill force-pushed the cosmic-text branch 2 times, most recently from 5d0e14f to 81d9c06 Compare November 12, 2023 15:48
@TotalKrill
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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.

@nicopap nicopap added the A-Text Rendering and layout for characters label Nov 13, 2023
@ickshonpe
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

TotalKrill commented Nov 13, 2023 via email

@TotalKrill
Copy link
Contributor Author

TotalKrill commented Nov 15, 2023

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
Copy link
Member

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
Copy link
Contributor

rparrett commented Jun 24, 2024

API for specifying text bounds (especially unbounded axes) in Text2dBundle changed as a result, but IMO much more logical.

👍 from me.

Bikeshedding requested

I guess

pub const fn new(width: f32, height: f32)
pub const fn horizontal(width: f32)
pub const fn vertical(height: f32)

would roughly match a similar pattern in UiRect.

/// Characters out of the bounds after wrapping will be truncated. Text is aligned according to the
/// specified [`JustifyText`](crate::text::JustifyText).
///
/// Note: only characters that are completely out of the bounds will be truncated, so this is not a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this comment is from the old Text2dBounds struct, but is this behavior the same in cosmic text?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking

@alice-i-cecile alice-i-cecile self-requested a review July 4, 2024 19:45
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to merge now: I'm settled on this direction and the code quality is good.

Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is in a good place for iteration in further PRs

@alice-i-cecile alice-i-cecile 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 Jul 4, 2024
@alice-i-cecile
Copy link
Member

@TotalKrill once CI is green (there's a new lint), ping me and I'll get this merged ASAP.

@alice-i-cecile alice-i-cecile enabled auto-merge July 4, 2024 20:31
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 4, 2024
Merged via the queue into bevyengine:main with commit 5986d5d Jul 4, 2024
31 checks passed
MrGVSV pushed a commit to MrGVSV/bevy that referenced this pull request Jul 5, 2024
# Replace ab_glyph with the more capable cosmic-text

Fixes bevyengine#7616.

Cosmic-text is a more mature text-rendering library that handles scripts
and ligatures better than ab_glyph, it can also handle system fonts
which can be implemented in bevy in the future

Rebase of bevyengine#8808

## Changelog

Replaces text renderer ab_glyph with cosmic-text

The definition of the font size has changed with the migration to cosmic
text. The behavior is now consistent with other platforms (e.g. the
web), where the font size in pixels measures the height of the font (the
distance between the top of the highest ascender and the bottom of the
lowest descender). Font sizes in your app need to be rescaled to
approximately 1.2x smaller; for example, if you were using a font size
of 60.0, you should now use a font size of 50.0.

## Migration guide

- `Text2dBounds` has been replaced with `TextBounds`, and it now accepts
`Option`s to the bounds, instead of using `f32::INFINITY` to inidicate
lack of bounds
- Textsizes should be changed, dividing the current size with 1.2 will
result in the same size as before.
- `TextSettings` struct is removed
- Feature `subpixel_alignment` has been removed since cosmic-text
already does this automatically
- TextBundles and things rendering texts requires the `CosmicBuffer`
Component on them as well

## Suggested followups:

- TextPipeline: reconstruct byte indices for keeping track of eventual
cursors in text input
- TextPipeline: (future work) split text entities into section entities
- TextPipeline: (future work) text editing
- Support line height as an option. Unitless `1.2` is the default used
in browsers (1.2x font size).
- Support System Fonts and font families
- Example showing of animated text styles. Eg. throbbing hyperlinks

---------

Co-authored-by: tigregalis <[email protected]>
Co-authored-by: Nico Burns <[email protected]>
Co-authored-by: sam edelsten <[email protected]>
Co-authored-by: Dimchikkk <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Rob Parrett <[email protected]>
SkiFire13 added a commit to SkiFire13/bevy that referenced this pull request Jul 11, 2024
robtfm pushed a commit to robtfm/bevy that referenced this pull request Aug 19, 2024
Fixes bevyengine#7616.

Cosmic-text is a more mature text-rendering library that handles scripts
and ligatures better than ab_glyph, it can also handle system fonts
which can be implemented in bevy in the future

Rebase of bevyengine#8808

Replaces text renderer ab_glyph with cosmic-text

The definition of the font size has changed with the migration to cosmic
text. The behavior is now consistent with other platforms (e.g. the
web), where the font size in pixels measures the height of the font (the
distance between the top of the highest ascender and the bottom of the
lowest descender). Font sizes in your app need to be rescaled to
approximately 1.2x smaller; for example, if you were using a font size
of 60.0, you should now use a font size of 50.0.

- `Text2dBounds` has been replaced with `TextBounds`, and it now accepts
`Option`s to the bounds, instead of using `f32::INFINITY` to inidicate
lack of bounds
- Textsizes should be changed, dividing the current size with 1.2 will
result in the same size as before.
- `TextSettings` struct is removed
- Feature `subpixel_alignment` has been removed since cosmic-text
already does this automatically
- TextBundles and things rendering texts requires the `CosmicBuffer`
Component on them as well

- TextPipeline: reconstruct byte indices for keeping track of eventual
cursors in text input
- TextPipeline: (future work) split text entities into section entities
- TextPipeline: (future work) text editing
- Support line height as an option. Unitless `1.2` is the default used
in browsers (1.2x font size).
- Support System Fonts and font families
- Example showing of animated text styles. Eg. throbbing hyperlinks

---------

Co-authored-by: tigregalis <[email protected]>
Co-authored-by: Nico Burns <[email protected]>
Co-authored-by: sam edelsten <[email protected]>
Co-authored-by: Dimchikkk <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Rob Parrett <[email protected]>
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1653 if you'd like to help out.

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-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to cosmic-text