fix(text): propagate WorldPosition dirty in _onRootCanvasModify when ReferenceResolutionPerUnit changes#2981
Conversation
…eLocalData
Both Text (UI) and TextRenderer share a `bounds` getter that runs
`_updateLocalData` then checks `WorldPosition` dirty. `_updateLocalData`
internally `_freeTextChunks` + `_buildChunk → allocateSubChunk`, which
under PrimitiveChunk's first-fit + free-list-merge allocator can return
a slot previously owned by another renderer. `_buildChunk` writes UV
and color but never pos (pos is `_updatePosition`'s job), so the new
slot retains the previous owner's pos floats as residue.
Before this fix, when a path sets only `LocalPositionBounds` dirty
(e.g. `Text._onRootCanvasModify(ReferenceResolutionPerUnit)` in UI
Text), the bounds getter would:
1. see LocalPositionBounds → run _updateLocalData (slot may swap)
2. see WorldPosition not dirty → skip _updatePosition
3. _setDirtyFlagFalse(Font) clear all dirty bits at once
The next _render also sees clean dirty bits and uploads the residue
pos to GPU — the renderer ends up rendering at someone else's old
world position. In practice this manifested as text glyphs jumping
to the wrong spot or appearing missing after UI tab switches that
free + reallocate chunk slots in the same frame.
Fix: force WorldPosition dirty at the end of _updateLocalData so the
contract "after this call, pos must be rewritten" is unconditionally
honored regardless of which caller invoked it.
Tests cover three layers:
- dirty-flag invariant: _updateLocalData must leave WorldPosition
dirty on exit
- corrupted-slot: bounds getter with only LocalPositionBounds dirty
rewrites pos even when the slot memory is poisoned
- full slot-reuse repro: destroy a sibling renderer occupying a
lower offset, then trigger bounds getter on the survivor — its
pos must remain correct after the slot moves
Without the fix, all three regression tests fail with the survivor
rendering at the destroyed sibling's old position.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA one-line fix in ChangesText world-position dirty flag fix
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2981 +/- ##
===========================================
+ Coverage 78.04% 78.14% +0.09%
===========================================
Files 906 906
Lines 99892 99902 +10
Branches 10190 10173 -17
===========================================
+ Hits 77960 78067 +107
+ Misses 21763 21665 -98
- Partials 169 170 +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:
|
🤖 Augment PR SummarySummary: Fixes a text rendering edge case where vertex-position data can be left as “residue” after text chunk slot reallocation, causing glyphs to jump/misrender. Changes:
Technical Notes: The fix ensures callers that only mark 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| */ | ||
| describe("Text - bounds-getter slot residue regression", async () => { | ||
| const canvas = document.createElement("canvas"); | ||
| const engine = await WebGLEngine.create({ canvas }); |
There was a problem hiding this comment.
tests/src/ui/Text.test.ts:170: This new regression suite creates a WebGLEngine but never calls engine.destroy(), and this file now creates two engines total. That can leak WebGL contexts across tests and make CI runs flaky due to context/resource exhaustion.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2981 +/- ##
===========================================
+ Coverage 78.04% 78.26% +0.21%
===========================================
Files 906 906
Lines 99892 99892
Branches 10190 10193 +3
===========================================
+ Hits 77960 78178 +218
+ Misses 21763 21544 -219
- Partials 169 170 +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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/core/2d/text/TextRenderer.test.ts (1)
401-403: 💤 Low valueConsider adding a note about maintaining sync with source enum.
These constants duplicate internal
DirtyFlagenum values fromTextRenderer.ts. If those values change, tests may silently pass/fail incorrectly.Consider adding a comment noting this dependency, or alternatively importing/exporting the enum (if feasible for the project's architecture).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/core/2d/text/TextRenderer.test.ts` around lines 401 - 403, Tests define TR_DIRTY_LOCAL_POSITION_BOUNDS and TR_DIRTY_WORLD_POSITION which duplicate the internal DirtyFlag enum from TextRenderer; update the test to either import/export the DirtyFlag enum from the TextRenderer module (preferred) or add a clear comment above TR_DIRTY_LOCAL_POSITION_BOUNDS and TR_DIRTY_WORLD_POSITION stating they must remain in sync with DirtyFlag in TextRenderer and reference the enum names, so future changes to DirtyFlag will be noticed and the test values updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/src/core/2d/text/TextRenderer.test.ts`:
- Around line 401-403: Tests define TR_DIRTY_LOCAL_POSITION_BOUNDS and
TR_DIRTY_WORLD_POSITION which duplicate the internal DirtyFlag enum from
TextRenderer; update the test to either import/export the DirtyFlag enum from
the TextRenderer module (preferred) or add a clear comment above
TR_DIRTY_LOCAL_POSITION_BOUNDS and TR_DIRTY_WORLD_POSITION stating they must
remain in sync with DirtyFlag in TextRenderer and reference the enum names, so
future changes to DirtyFlag will be noticed and the test values updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c1b44e90-72b6-4c6d-934c-7e0f6fd29443
📒 Files selected for processing (4)
packages/core/src/2d/text/TextRenderer.tspackages/ui/src/component/advanced/Text.tstests/src/core/2d/text/TextRenderer.test.tstests/src/ui/Text.test.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Previous fix added _setDirtyFlagTrue(WorldPosition) at the end of _updateLocalData in both TextRenderer and UI Text. That treats the output side as the place to declare invalidation, which conflates two concerns: dirty flags should declare staleness from input semantics, and update methods should be pure compute units that don't propagate flags themselves. Root cause is on the input side: _onRootCanvasModify(ReferenceResolutionPerUnit) declared LocalPositionBounds dirty but not WorldPosition, even though ReferenceResolutionPerUnit affects both local layout and the world positions derived from it. Fix the declaration where the input semantic event lives. TextRenderer needs no change — it has no entry point that dirties LocalPositionBounds without also dirtying WorldPosition (all setters use DirtyFlag.Position which includes both). Tests rewritten from white-box (poking private _dirtyFlag, hardcoded enum values) to public-API integration tests that drive the bug through uiCanvas.referenceResolutionPerUnit and assert observable vertex position changes. The new tests fail without the fix (maxDelta = 0, positions don't update) and pass with it.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
增量审查（第三轮）。PR 经过重构：从输出端 _updateLocalData 末尾强制 WorldPosition dirty 改为输入端 _onRootCanvasModify 标记。方向正确——dirty flag 应在输入语义处声明失效，update 方法应是纯计算单元。TextRenderer 无需修改（所有输入路径均通过 DirtyFlag.Position 设置，已包含 WorldPosition）。
但当前修复漏了一个 flag：_onRootCanvasModify 只设了 LocalPositionBounds | WorldPosition（0x18），而 referenceResolutionPerUnit 变更实际影响 local layout、world position 和 world bounding volume 三个维度，应设 DirtyFlag.Position（0x19 = LocalPositionBounds | WorldPosition | WorldVolume）。
已关闭问题
- [P2] UI Text 回归测试缺少
afterAll(engine.destroy())— 已在43c16a2修复，后在d61472de0重写但保留了afterAll。
问题
-
[P2] packages/ui/src/component/advanced/Text.ts:313 —
_onRootCanvasModify设置了DirtyFlag.LocalPositionBounds | DirtyFlag.WorldPosition（0x18），缺少WorldVolume（0x1）。referenceResolutionPerUnit变更会导致 local positions 和 world positions 改变，world bounding volume 同样会变。当前boundsgetter 中_isContainDirtyFlag(RendererUpdateFlags.WorldVolume)为 false，_updateBounds被跳过，返回 staleBoundingBox——可能影响 frustum culling 和 raycasting。修复：用
DirtyFlag.Position替代手工组合，语义完整且更简洁：_onRootCanvasModify(flag: RootCanvasModifyFlags): void { if (flag === RootCanvasModifyFlags.ReferenceResolutionPerUnit) { this._setDirtyFlagTrue(DirtyFlag.Position); } }
-
[P2] PR 标题 — "mark WorldPosition dirty after slot reallocation in _updateLocalData" 描述的是旧方案（输出端修改），当前实现是在
_onRootCanvasModify输入端标记。建议更新为 "fix(text): propagate WorldPosition dirty in _onRootCanvasModify when ReferenceResolutionPerUnit changes"。
简化建议
代码干净。上述 DirtyFlag.Position 替换同时是简化建议：用命名组合常量替代手工 OR。
自检 Checklist
- 重复检查 —
WorldVolume缺失是本轮新发现，不在已关闭清单中；PR 标题问题同理 - 前置检查 — 追溯
RendererUpdateFlags.WorldVolume = 0x1，确认与DirtyFlag.Position中的 0x1 bit 是同一 flag；boundsgetter 中_updateBounds依赖此 bit（Text.ts:236），当前修复不会触发 - 代码验证 — 验证了
RendererUpdateFlags.WorldVolume = 0x1（Renderer.ts:520）、DirtyFlag.Position = 0x19（Text.ts:694）、_onRootCanvasModify当前设置0x18（Text.ts:313），确认 0x1 bit 缺失
…erUnit change Use DirtyFlag.Position (= LocalPositionBounds | WorldPosition | WorldVolume) instead of the manual two-flag combination. ReferenceResolutionPerUnit also affects world bounding volume; without the WorldVolume bit, _updateBounds is skipped in the bounds getter and stale BoundingBox leaks into frustum culling and raycasting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cptbtptpbcptdtptp
changed the title
2 participants
Summary
_updateLocalData(bothTextRendererand UIText) calls_freeTextChunks+_buildChunk → allocateSubChunk, which underPrimitiveChunk's first-fit + free-list-merge allocator can hand back a slot previously owned by another renderer._buildChunkwrites UV/color but not pos, so the new slot retains the previous owner's pos floats as residue.boundsgetter path runs_updateLocalDatathen checksWorldPosition. When onlyLocalPositionBoundsis dirty (e.g. UI Text's_onRootCanvasModify(ReferenceResolutionPerUnit)),_updatePositionis skipped and_setDirtyFlagFalse(Font)clears all dirty bits at once. The next_renderthen uploads the residue pos to GPU — text glyphs jump to the wrong spot or appear missing after UI tab switches that free + reallocate chunk slots in the same frame.WorldPositiondirty at the end of_updateLocalDataso the contract "after this call, pos must be rewritten" is unconditionally honored regardless of caller.Test plan
_updateLocalDatamust leaveWorldPositiondirty on exit (dirty-flag invariant)boundsgetter with onlyLocalPositionBoundsdirty rewrites pos even when slot memory is poisoned (corrupted-slot)boundsgetter on the survivor, keeps the survivor's pos correct after the slot moves (full slot-reuse repro)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests