Skip to content

Upgrade to cosmic-text 0.13#18239

Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom
rparrett:cosmic-13
Mar 12, 2025
Merged

Upgrade to cosmic-text 0.13#18239
alice-i-cecile merged 2 commits intobevyengine:mainfrom
rparrett:cosmic-13

Conversation

@rparrett
Copy link
Copy Markdown
Contributor

@rparrett rparrett commented Mar 10, 2025 •
edited
Loading

Objective

Upgrade to cosmic-text 0.13

https://github.com/pop-os/cosmic-text/releases

This should include some performance improvements for layout and system font loading.

Solution

Bump version, fix the one changed API.

Testing

Tested some examples locally, will invoke the example runner.

Layout Perf

main fps cosmic-13 fps
many_buttons --recompute-text --no-borders 6.79 9.42 🟩 +38.7%
many_text2d --no-frustum-culling --recompute 3.19 4.28 🟩 +34.0%
many_glyphs --recompute-text 7.09 11.17 🟩 +57.6%
text_pipeline 140.15 139.90 ⬜ -0.2%

System Font Loading Perf

I tested on macOS somewhat lazily by adding the following system to the system_fonts example from #16365.

Expand code 
fn exit_on_load(
    mut reader: EventReader<bevy::text::SystemFontsAvailable>,
    mut writer: EventWriter<AppExit>,
) {
    for _evt in reader.read() {
        writer.write(AppExit::Success);
    }
}

And running:

cargo build --release --example system_fonts --features=system_font
hyperfine target/release/examples/system_fonts

And there was seemingly no change.

Expand results

Before

Benchmark 1: target/release/examples/system_fonts
  Time (mean ± σ):     258.0 ms ±  14.2 ms    [User: 136.2 ms, System: 95.8 ms]
  Range (min … max):   245.9 ms … 294.0 ms    10 runs

After

Benchmark 1: target/release/examples/system_fonts
  Time (mean ± σ):     261.9 ms ±   8.9 ms    [User: 141.8 ms, System: 95.5 ms]
  Range (min … max):   252.3 ms … 281.4 ms    10 runs

@github-actions github-actions bot mentioned this pull request Mar 10, 2025
Closed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 10, 2025

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-18239

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Mar 10, 2025
@alice-i-cecile alice-i-cecile added C-Dependencies A change to the crates that Bevy depends on A-Text Rendering and layout for characters S-Needs-Testing Testing must be done before this is safe to merge X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered labels Mar 10, 2025
alice-i-cecile
alice-i-cecile approved these changes Mar 10, 2025
View reviewed changes
Copy link
Copy Markdown
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've looked over the Pixel Eagle output and the diff seems totally fine. There are differences, but I can't say that either is better or worse.

@rparrett
Copy link
Copy Markdown
Contributor Author

rparrett commented Mar 10, 2025

I think the minor differences can be attributed to swash 0.2.0, but it's difficult to follow the changes to that library and its dependencies.

@rparrett
Copy link
Copy Markdown
Contributor Author

rparrett commented Mar 10, 2025

I did initiate a full example run just in case, still pending.

@rparrett
Copy link
Copy Markdown
Contributor Author

rparrett commented Mar 10, 2025 •
edited
Loading

Not seeing anything concerning in there, just the same minor differences noticed by the testbed.

However, it seems that cosmic-text is failing to build for android in the example runner.

@mnmaita
Copy link
Copy Markdown
Member

mnmaita commented Mar 10, 2025

pop-os/cosmic-text#349 looks like someone had the same issue with the previous version too?

@rparrett
Copy link
Copy Markdown
Contributor Author

rparrett commented Mar 11, 2025

Reverting pop-os/cosmic-text@829a59b seems to fix it. I guess android targets used to take the "no-op" path in other.rs and that's not happening anymore.

I have a branch that works but I'm not 100% confident in. Will open a PR for discussion.

@rparrett
Copy link
Copy Markdown
Contributor Author

rparrett commented Mar 11, 2025

pop-os/cosmic-text#365

@alice-i-cecile alice-i-cecile modified the milestones: 0.16, 0.17 Mar 11, 2025
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Mar 11, 2025

Bumping from 0.17; we shouldn't wait on a fix IMO. This is not essential to ship immediately.

@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes and removed S-Needs-Testing Testing must be done before this is safe to merge labels Mar 11, 2025
@rparrett
Copy link
Copy Markdown
Contributor Author

rparrett commented Mar 11, 2025

cosmic-text 0.13.2 was released which should fix the Android issue.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Mar 11, 2025
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Mar 11, 2025

Excellent, thanks @jackpot51 <3

Carter0
Carter0 approved these changes Mar 12, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@Carter0 Carter0 left a comment

Choose a reason for hiding this comment

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

Thank you!

@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 Mar 12, 2025
@alice-i-cecile alice-i-cecile modified the milestones: 0.17, 0.16 Mar 12, 2025
@alice-i-cecile alice-i-cecile changed the title Upgrade to cosmic-text 0.13.0 Upgrade to cosmic-text 0.13 Mar 12, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 12, 2025
Merged via the queue into bevyengine:main with commit 81c0900 Mar 12, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+1 more reviewer

@Carter0 Carter0 Carter0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Text Rendering and layout for characters C-Dependencies A change to the crates that Bevy depends on D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon

Projects

None yet

Milestone

0.16

Development

Successfully merging this pull request may close these issues.

4 participants

@rparrett @mnmaita @alice-i-cecile @Carter0