Skip to content

Fix text wrapping due to sub-pixel floating-point differences - #26323

Open
unclepomedev wants to merge 3 commits into
bevyengine:mainfrom
unclepomedev:fix/text_wrap
Open

unclepomedev wants to merge 3 commits into
bevyengine:mainfrom
unclepomedev:fix/text_wrap

Conversation

@unclepomedev

@unclepomedev unclepomedev commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Objective

fixes #26305

Single-line text in an auto-width node with horizontal padding wraps at some fractional UiScale values (e.g. 5/6, 2/3), even though the line fits.

Solution

Snap the width used for line breaking to the max-content width when it is within max_content * f32::EPSILON * 8 of it. The 8 is arbitrary, just a few ULPs of room.

The layout width is still 119.99999. This only stops line breaking from treating that as too narrow for text that needs 120.

Testing

  • Unit test for the snap helper.
  • Visually confirmed that the UI flaw resolved.
  • Ran the repro from the issue: the WordBoundary node is 120x24 now, it was 120x48.
  • Ran a probe sweeping padding (2–16px) and font size (10–32px) with the text in a centered column and the padding on its parent: some combinations wrapped before, none after, at both 5/6 and 2/3.

AI Disclosure

  • Identifying and summarizing relevant code.
  • Consultation of solutions.
  • Creating a probe.

@unclepomedev
unclepomedev marked this pull request as ready for review October 26, 2026 11:20
@DaAlbrecht DaAlbrecht added A-UI Graphical user interfaces, styles, layouts, and widgets D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward AI-Assisted Discloses the use of AI in any way labels Oct 26, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in UI Oct 26, 2026

@ickshonpe ickshonpe left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This confused me for a bit, because I can remember fixing it before. It must have been broken again by the changes adding padding and borders support for content nodes.

The fix is fine, maybe it's too cautious even and possibly we just should ceil all the bounds.

I'd like to have a higher level regression test that produces a text layout, similar to the issue's replication example, but that's a bit more complicated to setup, it could be left to a follow up PR.

@ickshonpe ickshonpe added the A-Text Rendering and layout for characters label Oct 27, 2026

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This fix appears to work, but I'd prefer:

possibly we just should ceil all the bounds

I think that's both simpler and more robust. Just gotta make sure that measuring and drawing work the same way throughout.

@unclepomedev are you okay to tackle that in this PR, or would you rather we merge an incremental fix?

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, meant to leave a neutral review!

@alice-i-cecile
alice-i-cecile self-requested a review October 27, 2026 20:07
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 27, 2026
@unclepomedev

Copy link
Copy Markdown
Contributor Author

Switched to ceil() as suggested. measure already returns .ceil()'d sizes, so the wrapping width now follows the same rounding.

Re-ran the probe from the PR description (7 paddings × 9 font sizes, padding on the parent, centered column): 2 combinations wrapped on main, 0 now, at both 5/6 and 2/3. The earlier ULP-snap gave the same result, so this is simpler with no loss.

@unclepomedev

Copy link
Copy Markdown
Contributor Author

I'll try to add the higher-level regression test in a follow-up PR, as ickshonpe suggested.

@unclepomedev

Copy link
Copy Markdown
Contributor Author

While checking that measuring and drawing round the same way, I found that the repro from #26305 still wraps in the glyph layout when LayoutConfig { use_rounding: false } is set: the node stays 119.99999 wide. This doesn't happen with the default use_rounding: true, where the content box is already an integer.

I filed #26351 and opened #26352 (draft), which rounds up the width used for the text bounds in text_system (the node size itself is unchanged). The two changes are independent in code, but the repro in #26351 needs both, so I'm happy to fold it into this PR if you'd prefer one PR. Otherwise I'll keep them separate.

This branch has not been deployed

No deployments
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 AI-Assisted Discloses the use of AI in any way D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

Single-line text in an auto-width padded node wraps at a fractional UiScale

4 participants