Skip to content

fix(ui): invalidate overlay canvas sorting when sortOrder changes - #3115

Open
cptbtptpbcptdtptp wants to merge 1 commit into
galacean:dev/2.0from
cptbtptpbcptdtptp:fix/ui-overlay-canvas-sort-invalidation
Open

cptbtptpbcptdtptp wants to merge 1 commit into
galacean:dev/2.0from
cptbtptpbcptdtptp:fix/ui-overlay-canvas-sort-invalidation

Conversation

@cptbtptpbcptdtptp

@cptbtptpbcptdtptp cptbtptpbcptdtptp commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

问题

UICanvas.sortOrder 的 setter 给一个不存在的成员赋值:

this._realRenderMode === CanvasRenderMode.ScreenSpaceOverlay &&
  // @ts-ignore
  (this.scene._componentsManager._overlayCanvasesSortingFlag = true);   // ← 该成员不存在

ComponentsManager 记录的脏标记实际叫 _overlayCanvasesSortingDirtypackages/core/src/ComponentsManager.ts:23),它由 addUICanvas / removeUICanvas 置位,并只在 sortOverlayUICanvases() 中消费与清除。那行 @ts-ignore 让拼写错误完全无声:内部成员被 stripInternal 从发布声明中剥离,所以类型检查本来也发现不了它(该问题正由 #3105 一并处理)。

影响

Engine._render 每帧在 UIUtils.renderOverlay() 之前调用 sortOverlayUICanvases(),但它只在脏标记为真时才真正重排。因此在已处于 ScreenSpaceOverlay 模式的画布上修改 sortOrder

  • 脏标记保持为假,重排被跳过;
  • _canvasIndex 维持旧值,渲染顺序与 Overlay 命中顺序都继续使用过期顺序;
  • 直到某个无关的画布增删(或引擎停机重建)才顺带纠正。

修复

仅改为给正确的成员赋值。@ts-ignore 暂时保留,因为 _overlayCanvasesSortingDirty 目前仍是 @internal、不在本分支编译所用的声明中;#3105 合并后可以删除(见下方说明)。

测试

新增用例创建两块 Overlay 画布(sortOrder 为 1 和 2),排序后断言索引递增,再把较低者的 sortOrder 改为 3 并重新排序,断言其索引变大。修复前该断言失败(expected +0 to be above 1),修复后通过。

验证

tests/src/ui/UICanvas.test.ts

场景 结果
修复源码(@galacean/engine-ui 解析到 packages/ui/src 7 / 7 通过
未修复实现(同一用例,解析到修复前的产物) 1 失败expected +0 to be above 1

tests/src/ui/ 全量:69 / 69 通过

#3105 的关系

本 PR 是从 #3105 中拆出的行为修复部分;#3105 剩下的是声明契约修复(stripInternal)。两者可以独立合入,互不依赖:

Summary by CodeRabbit

  • Bug Fixes

    • Fixed screen-space overlay canvases so their display order updates correctly after changing sort order.
  • Tests

    • Added coverage verifying that overlay canvases are reordered when their sort order changes.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 85b3b093-f018-4234-bb72-6149c7991d4e

📥 Commits

Reviewing files that changed from the base of the PR and between d2ea4f8 and 90f0f6d.

📒 Files selected for processing (1)
  • packages/ui/src/component/UICanvas.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/ui/src/component/UICanvas.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.


Walkthrough

UICanvas now marks overlay canvas sorting as dirty when sortOrder changes. A test verifies that resorting reverses canvas indices when sort orders change.

Changes

Overlay Canvas Sorting

Layer / File(s) Summary
Sorting invalidation and validation
packages/ui/src/component/UICanvas.ts, tests/src/ui/UICanvas.test.ts
The setter writes _overlayCanvasesSortingDirty to ComponentsManager. The test verifies overlay canvas indices before and after a sort-order change.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 90f0f

The overlay canvas sorting invalidation fix is covered by the stated regression tests, with no remaining merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: invalidating overlay canvas sorting when sortOrder changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

A rabbit hops where canvas layers gleam
A changed sort order shifts the scene
The dirty flag wakes sorting bright
Two indices trade their place just right
Tests guard the order through the night

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.75%. Comparing base (1d06406) to head (90f0f6d).
⚠️ Report is 1 commits behind head on dev/2.0.

Additional details and impacted files
@@             Coverage Diff             @@
##           dev/2.0    #3115      +/-   ##
===========================================
- Coverage    85.94%   85.75%   -0.20%     
===========================================
  Files          811      811              
  Lines        94826    94749      -77     
  Branches     11620    11625       +5     
===========================================
- Hits         81497    81248     -249     
- Misses       13239    13409     +170     
- Partials        90       92       +2     
Flag Coverage Δ
unittests 85.75% <100.00%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cptbtptpbcptdtptp
cptbtptpbcptdtptp force-pushed the fix/ui-overlay-canvas-sort-invalidation branch from d2ea4f8 to 471a8f7 Compare September 11, 2026 10:14
`Engine._render` only re-sorts overlay canvases when
`ComponentsManager._overlayCanvasesSortingDirty` is set, but `UICanvas.sortOrder`
set a different member, so the flag stayed clean and the re-sort was skipped.
Changing `sortOrder` on a live `ScreenSpaceOverlay` canvas therefore kept the
stale `_canvasIndex` order for both rendering and overlay hit-testing until an
unrelated canvas add/remove happened to dirty the flag.
@cptbtptpbcptdtptp
cptbtptpbcptdtptp force-pushed the fix/ui-overlay-canvas-sort-invalidation branch from 471a8f7 to 90f0f6d Compare September 11, 2026 10:15

@GuoLei1990 GuoLei1990 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.

🫧 尘小沫

结论

阻塞级别:P1。实际 review 动作:REQUEST_CHANGES。目标 HEAD:90f0f6d6894e96f61bf61f2c93221c61ba8688c5。本 PR 已修正渲染侧的排序失效,但下一次 Overlay 命中仍可能读取旧顺序。自动 CR 不替代人工 Reviewer 的合入门禁,仍需人工 Reviewer 审核确认。

问题

  • [P1] packages/ui/src/input/UIPointerEventEmitter.ts:45 读取 _overlayCanvases 后直接反向 raycast,没有先消费 _overlayCanvasesSortingDirtyEngine.update() 先在 packages/core/src/Engine.ts:357 调用 inputManager._firePointerScript(),最后才在 :388 进入 _render(),而后者才在 :614 调用 sortOverlayUICanvases()。因此,在两帧之间(或首帧渲染前)把已注册 Overlay canvas 的 sortOrder 改为新值后,下一次 pointer event 仍会按旧数组顺序命中;新 dirty flag 要到该次事件之后的 render 才被消费。请保持 ComponentsManager 为唯一的排序、dirty 和 _canvasIndex owner:在 UIPointerEventEmitter.processRaycast() 消费 Overlay 数组前调用既有的 sortOverlayUICanvases(),或在 _firePointerScript() 前由 Engine 统一调用它。不要在 emitter 复制 comparator、写 _canvasIndex 或增加镜像 dirty state。将当前直接调用 manager 并断言私有 _canvasIndex 的用例改为从 pointer event/handler 断言重新置顶的 canvas 首先接收事件,覆盖“改 order 后下一次输入早于 render”的时序;删除该实现细节断言,不保留第二份测试契约。

架构、熵增与测试治理

UICanvas.sortOrder 是配置输入,_realRenderMode 是 Overlay 资格的权威事实;ComponentsManager 唯一拥有 Overlay 注册数组、dirty 状态、排序和派生的 _canvasIndexEngine._renderUIUtils.renderOverlayUIPointerEventEmitter 都是该数组的下游消费者。本次一行修复正确地恢复了输入端到该 owner 的 invalidation,未新增持久状态、兼容分支、转换或校验路径;P1 是输入消费者没有在读取前 reconcile 的剩余断点,应复用 manager 的既有收口而非新建第二套状态机。

当前测试能证明旧拼写不会触发 manager 内部排序,但它锁定私有实现,且遗漏 input-before-render 控制流。修复 P1 时应按实际用户可观察的点击结果重写该用例,不保留私有索引断言、fixture/mock 或为旧测试增加任何生产兼容逻辑。

验证

  • 按 PR merge-base 增量审阅:1 个提交、2 个文件;已追踪 setter → ComponentsManager → render / pointer 两个直接下游。
  • GitHub CI 的 lint、三平台 build、4 个 e2e 分片和 codecov patch 均通过。codecov/project 仍为失败状态,但其报告显示改动可覆盖行均已覆盖,且比较的 dev/2.0 基线落后 1 个提交;未将其推断为本次代码 finding。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants