Repository navigation
Fix text bounds after line alignment - #3751
SamDScaread wants to merge 1 commit into
Conversation
m4sternoob
left a comment
m4sternoob
left a comment
There was a problem hiding this comment.
I reviewed the implementation and the new tests. The approach is sound: the old code computed whole-text bounds and then applied a crude post-hoc shift for center/right alignment (position.x -= size.x / 2), which broke whenever a line's visible width differed from its advance. Tracking per-line visible extents (minX/maxX) through shaping and deriving the aligned bounds from the shifted per-line extents addresses the root cause rather than the symptom.
Also good: moving the outline expansion to after the alignment pass — expanding before shifting was part of the same bug for outlined text.
I traced the sentinel logic: currentLineMinX starts at m_characterSize and the line.minX <= line.maxX guard keeps empty lines from polluting the bounds, so the sentinel can never leak into alignedMinX. That holds up.
One question: the new test locks in the single-line case nicely (bounds displacing rigidly with the glyphs across spacing/outline/italic/whitespace variants), but the per-line accumulation in outputLine/beginLineRecord really pays off with multiple lines of differing widths. Would a two-line case be worth adding to cover the per-line path?
3 participants
Description
Fixes #3745.
Centered and right-aligned text move glyphs using each line's advance, while the old bounds adjustment used the visible width of the entire text. These values can differ: with Tuffy at size 30,
Testhas a visible width of 52 and an advance of 55. Its centered bounds started at x = -25 instead of -26.5, and its right-aligned bounds started at -51 instead of -54.Record each physical line's horizontal bounds, apply the same shift used for its glyphs, then take the union of the shifted bounds. Apply the existing outline expansion afterward. Accumulating all script/direction segments within a line also preserves automatic alignment of RTL text.
Add regression checks for single and multiple lines, transformed global bounds, letter spacing, outlines, italic text, whitespace, and empty lines. Expected bounds are derived from independently positioned glyphs. These replace four hard-coded alignment checks that encoded the old width-based shift; the other existing bounds checks remain.
Tasks
clang-format-17 --dry-run --Werrorand diff whitespace checks passed.How to test this PR?
Build the Graphics tests with
SFML_BUILD_TEST_SUITE=ONandSFML_RUN_DISPLAY_TESTS=ON, then run thesf::Textcase. The same final test file produces 30 failures out of 336 assertions on the original implementation and passes all 336 assertions with this change. This was also reproduced from fresh source trees using the exported patch.For a visual check, draw
Testand its reported bounds using Left, Center, and Right alignment; repeat with two lines of different lengths. Both platforms produced the corrected centered and right-aligned coordinates above.Local validation used harnesses compiling the repository's actual Graphics test sources and utilities. Audio and Network tests were outside the validation scope. MSVC and the native NVIDIA rendering path were not tested.
AI assistance: this patch and its regression tests were prepared with OpenAI Codex. Local validation and upstream CI have completed successfully; maintainer review is pending.