Add characterSpacing and change spacing units to em#2908
Merged
cptbtptpbcptdtptp merged 25 commits intogalacean:dev/2.0from Mar 1, 2026
Merged
Add characterSpacing and change spacing units to em#2908cptbtptpbcptdtptp merged 25 commits intogalacean:dev/2.0from
cptbtptpbcptdtptp merged 25 commits intogalacean:dev/2.0from
Conversation
…pacing # Conflicts: # packages/core/src/2d/text/TextUtils.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Unify spacing units - both lineSpacing and characterSpacing now use em (ratio of fontSize) for consistency. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughAdds character spacing support across text rendering: new public characterSpacing accessors in core and UI text classes, measurement utilities updated to accept characterSpacing, tests and an E2E visual case added, and a small UI event-dispatch guard change. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/core/2d/text/TextUtils.test.ts (1)
142-143: Use a named constant for the newcharacterSpacingargument instead of repeating literal0.This improves readability and makes future API/unit adjustments less error-prone.
Refactor sketch
+ const DEFAULT_CHARACTER_SPACING_EM = 0; let result = TextUtils.measureTextWithWrap( textRendererTruncate, textRendererTruncate.width * _pixelsPerUnit, textRendererTruncate.height * _pixelsPerUnit, textRendererTruncate.lineSpacing * _pixelsPerUnit, - 0 + DEFAULT_CHARACTER_SPACING_EM );Also applies to: 176-177, 205-206, 239-240, 260-261, 294-295, 323-324, 357-358, 374-375, 386-387, 405-406, 417-418, 429-430, 441-442, 461-462, 474-475, 487-488, 499-500, 511-512, 523-524
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/2d/text/TextUtils.test.ts` around lines 142 - 143, Replace the repeated literal 0 used for the trailing characterSpacing argument with a named constant (e.g., DEFAULT_CHARACTER_SPACING = 0) to improve readability and maintainability; update the calls that compute height using textRendererTruncate.lineSpacing * _pixelsPerUnit, 0 (and the other listed occurrences) to pass DEFAULT_CHARACTER_SPACING instead of 0, and add the constant near the top of TextUtils.test.ts so all tests reference the same named value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/src/core/2d/text/TextUtils.test.ts`:
- Around line 142-143: The tests use textRendererTruncate.lineSpacing *
_pixelsPerUnit which is incorrect; update all calls to convert em-based
lineSpacing using the renderer's font size instead (multiply lineSpacing by
textRendererTruncate.fontSize), e.g. wherever TextUtils.measureTextWithWrap or
measureTextWithoutWrap is invoked with a pixel lineSpacing argument, replace
uses of _pixelsPerUnit with the renderer instance's fontSize property; ensure
references are to TextRenderer (textRendererTruncate) and its fontSize so
lineHeight calculations match the implementation (this._lineSpacing *
this._fontSize).
---
Nitpick comments:
In `@tests/src/core/2d/text/TextUtils.test.ts`:
- Around line 142-143: Replace the repeated literal 0 used for the trailing
characterSpacing argument with a named constant (e.g., DEFAULT_CHARACTER_SPACING
= 0) to improve readability and maintainability; update the calls that compute
height using textRendererTruncate.lineSpacing * _pixelsPerUnit, 0 (and the other
listed occurrences) to pass DEFAULT_CHARACTER_SPACING instead of 0, and add the
constant near the top of TextUtils.test.ts so all tests reference the same named
value.
Comment on lines
+142
to
+143
| textRendererTruncate.lineSpacing * _pixelsPerUnit, | ||
| 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Inspect unit semantics in core text APIs ==="
echo "--- TextUtils.ts method signatures and lineSpacing handling ---"
rg -n 'measureTextWithWrap|measureTextWithoutWrap|lineSpacing|characterSpacing' \
packages/core/src/2d/text/TextUtils.ts -C3 | head -200
echo ""
echo "--- TextRenderer.ts lineSpacing/characterSpacing context ---"
rg -n 'lineSpacing|characterSpacing' \
packages/core/src/2d/text/TextRenderer.ts -C3 | head -200
echo ""
echo "=== 2) Inspect current test conversions and call arguments ==="
echo "--- Test lineSpacing conversions ---"
rg -n 'lineSpacing\s*\*\s*_pixelsPerUnit' \
tests/src/core/2d/text/TextUtils.test.ts -C1
echo ""
echo "--- Test method calls with arguments ---"
rg -n 'measureTextWithWrap\(|measureTextWithoutWrap\(' \
tests/src/core/2d/text/TextUtils.test.ts -C2 | head -150Repository: galacean/engine
Length of output: 14405
Correct lineSpacing unit conversion in test calls — use fontSize instead of _pixelsPerUnit.
The tests incorrectly convert lineSpacing by multiplying by _pixelsPerUnit. According to the TextRenderer.ts implementation (lines 535, 541), em-based lineSpacing should be converted using _fontSize, not _pixelsPerUnit:
// Correct conversion in TextRenderer.ts:
this._lineSpacing * this._fontSize
// Incorrect in tests:
textRendererTruncate.lineSpacing * _pixelsPerUnitSince TextUtils.measureTextWithWrap/WithoutWrap expect pixel-based lineSpacing (used as lineHeight = fontSizeInfo.size + lineSpacing), update all test calls to multiply by the renderer's fontSize property.
Affected lines: 142-143, 176-177, 205-206, 239-240, 260-261, 294-295, 323-324, 357-358, 374-375, 386-387, 405-406, 417-418, 429-430, 441-442, 461-462, 474-475, 487-488, 499-500, 511-512, 523-524.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/core/2d/text/TextUtils.test.ts` around lines 142 - 143, The tests
use textRendererTruncate.lineSpacing * _pixelsPerUnit which is incorrect; update
all calls to convert em-based lineSpacing using the renderer's font size instead
(multiply lineSpacing by textRendererTruncate.fontSize), e.g. wherever
TextUtils.measureTextWithWrap or measureTextWithoutWrap is invoked with a pixel
lineSpacing argument, replace uses of _pixelsPerUnit with the renderer
instance's fontSize property; ensure references are to TextRenderer
(textRendererTruncate) and its fontSize so lineHeight calculations match the
implementation (this._lineSpacing * this._fontSize).
…t measurement logic
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2908 +/- ##
===========================================
- Coverage 78.36% 78.35% -0.01%
===========================================
Files 880 881 +1
Lines 96111 96304 +193
Branches 9602 9626 +24
===========================================
+ Hits 75315 75459 +144
- Misses 20634 20682 +48
- Partials 162 163 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/2d/text/TextUtils.ts`:
- Around line 184-185: The wrap check is using glyph width variable `w` while
the line accumulation logic uses `xAdvance` (+ `characterSpacing`), causing
incorrect wrapping; update the conditional(s) that currently compare `lineWidth
+ w > rendererWidth` (appearing near the checks at the locations referencing
lines 184 and 205-206) to use the same metric used for accumulation (i.e., use
`xAdvance` and include `characterSpacing` where appropriate) so the wrap
decision matches how `lineWidth` is incremented.
- Around line 303-305: The current logic computes curWidth using
characterSpacing and then only adds the line when curWidth > 0, which causes
non-empty lines to be dropped if characterSpacing is large negative; change the
push condition to check for a non-empty line (use lineLength > 0 or line.length
> 0) rather than curWidth > 0 so lines are preserved regardless of spacing, and
apply the same fix to the analogous block handling lines at the 319-322 region;
reference symbols: curWidth, lineLength, characterSpacing and the code path that
pushes the line into the result.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/2d/text/TextUtils.tspackages/ui/src/component/UICanvas.tspackages/ui/src/component/advanced/Text.ts
Comment on lines
184
to
+185
| if (lineWidth + w > rendererWidth && lineWidth > 0) { | ||
| lineWidth -= characterSpacing; |
There was a problem hiding this comment.
Wrap checks use w while line accumulation uses xAdvance; this can mis-wrap lines.
Line 184 and Line 205 compare against glyph width (w) but widths are accumulated with xAdvance (plus spacing), so wrap boundaries can be off for many fonts/kerning cases.
🛠️ Suggested fix
- if (lineWidth + w > rendererWidth && lineWidth > 0) {
+ if (lineWidth + charInfo.xAdvance > rendererWidth && lineWidth > 0) {
lineWidth -= characterSpacing;
this._pushLine(lines, lineWidths, lineMaxSizes, line, lineWidth, lineMaxAscent, lineMaxDescent);
textWidth = Math.max(textWidth, lineWidth);
notFirstLine = true;
if (isSpace) {
@@
- if (wordWidth + charInfo.w > rendererWidth) {
+ if (wordWidth + charInfo.xAdvance > rendererWidth) {
if (lineWidth > 0) {
lineWidth -= characterSpacing;
this._pushLine(lines, lineWidths, lineMaxSizes, line, lineWidth, lineMaxAscent, lineMaxDescent);Also applies to: 205-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/2d/text/TextUtils.ts` around lines 184 - 185, The wrap
check is using glyph width variable `w` while the line accumulation logic uses
`xAdvance` (+ `characterSpacing`), causing incorrect wrapping; update the
conditional(s) that currently compare `lineWidth + w > rendererWidth` (appearing
near the checks at the locations referencing lines 184 and 205-206) to use the
same metric used for accumulation (i.e., use `xAdvance` and include
`characterSpacing` where appropriate) so the wrap decision matches how
`lineWidth` is incremented.
Comment on lines
+303
to
305
| const lineLength = line.length; | ||
| let curWidth = lineLength > 1 ? characterSpacing * (lineLength - 1) : 0; | ||
| let maxAscent = 0; |
There was a problem hiding this comment.
Non-empty lines can disappear with large negative characterSpacing.
Line 319 only pushes when curWidth > 0; with sufficiently negative spacing, a non-empty line can be skipped entirely.
🛠️ Suggested fix
- if (curWidth > 0) {
- this._pushLine(lines, lineWidths, lineMaxSizes, line, curWidth, maxAscent, maxDescent);
- width = Math.max(width, curWidth);
+ if (lineLength > 0) {
+ const finalWidth = Math.max(0, curWidth);
+ this._pushLine(lines, lineWidths, lineMaxSizes, line, finalWidth, maxAscent, maxDescent);
+ width = Math.max(width, finalWidth);
}Also applies to: 319-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/2d/text/TextUtils.ts` around lines 303 - 305, The current
logic computes curWidth using characterSpacing and then only adds the line when
curWidth > 0, which causes non-empty lines to be dropped if characterSpacing is
large negative; change the push condition to check for a non-empty line (use
lineLength > 0 or line.length > 0) rather than curWidth > 0 so lines are
preserved regardless of spacing, and apply the same fix to the analogous block
handling lines at the 319-322 region; reference symbols: curWidth, lineLength,
characterSpacing and the code path that pushes the line into the result.
GuoLei1990
changed the title
cptbtptpbcptdtptp
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
characterSpacingproperty to TextRenderer and UI Text for controlling space between characterscharacterSpacingandlineSpacing, matching Unity TMP / Figma / CSS best practicescharInfo.offsetXfrom pen advance (both core TextRenderer and UI Text), offsetX is already consumed during atlas bakingcharacterSpacingbefore pushing word line width_canDispatchEvent: fallback to all cameras when assigned camera is inactive (_phasedActiveInScene)packages/ui): addcharacterSpacingproperty, changelineSpacingto em units, fixoffsetXpen advance bugcharacterSpacingparameterBreaking Changes
lineSpacingunit changed from world units to em (ratio of fontSize) in both TextRenderer and UI TextTest plan
text-character-spacingcase