Skip to content

Conversation

@ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Dec 6, 2025

Objective

Remove the queue_text function as redundant.

Solution

  • Remove queue_text from TextPipeline.
  • In update_text2d_layout, instead of queue_text, call TextPipeline::update_buffer and then TextPipeline::update_text_layout.

@ickshonpe ickshonpe added C-Code-Quality A section of code that is hard to understand or change A-Text Rendering and layout for characters D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 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-22051

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.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 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-22051

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.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 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-22051

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.

1 similar comment
@github-actions
Copy link
Contributor

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-22051

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.

@github-actions
Copy link
Contributor

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-22051

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.

@github-actions
Copy link
Contributor

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-22051

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.

buffer.set_size(font_system, bounds.width, bounds.height);

// Workaround for alignment not working for unbounded text.
// See https://github.com/pop-os/cosmic-text/issues/343
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a bevy issue too to track this too?

Copy link
Contributor Author

@ickshonpe ickshonpe Dec 10, 2025

Choose a reason for hiding this comment

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

Well the workaround here works, so I don't see any need especially.

I suppose we could add a Blocked "remove-this-hack-when-cosmic-text-fixes-it-upstream" issue.

@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 Dec 10, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 10, 2025
Merged via the queue into bevyengine:main with commit 176d5a3 Dec 10, 2025
38 checks passed
@mockersf
Copy link
Member

mockersf commented Dec 12, 2025

This PR tanked the perfs of the many_text2d stress test, could you check why?
It went from ~400fps to 8 on my laptop

noticed on https://metrics.bevy.org/stress-tests.html#176d5a39b7c146bc6885e095bd105871fba24054

Screenshot 2025-12-12 at 21 00 13

github-merge-queue bot pushed a commit that referenced this pull request Dec 12, 2025
# Objective

Fix performance regression caused by #22051 breaking text2d's change
detection.

Fixes #22099.

## Solution

Set the text entity's `ComputedTextBlock::needs_rerender` flag to false
at the start of `update_text_buffer` and `update_text_layout_info`.

## Testing

```
cargo run --example many_text2d --release
```

should run about 400x main's FPS
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 C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants