fix(ui): add eventCamera support for WorldSpace UICanvas (#2793) #2870
Conversation
WalkthroughAdds eventCamera support to UICanvas (storage, getter/setter, validation), a per-camera eligibility check (_canProcessEvent), propagation of eventCamera during cloning, and updates UIPointerEventEmitter to use the new eligibility predicate; tests added to validate behaviors (some duplicated). Changes
Sequence Diagram(s)sequenceDiagram
participant Pointer as Pointer Input
participant Emitter as UIPointerEventEmitter
participant Canvas as UICanvas
participant Camera as Camera
Pointer->>Emitter: deliver pointer event (with Camera)
Emitter->>Emitter: processRaycast(camera)
loop for each canvas
Emitter->>Canvas: ask _canProcessEvent(camera)?
alt Canvas.renderMode == WorldSpace and Canvas.eventCamera != null
Canvas->>Camera: compare to eventCamera
Camera-->>Canvas: matches / not matches
else Canvas.renderMode == ScreenSpaceCamera and Canvas.renderCamera != null
Canvas->>Camera: compare to renderCamera
Camera-->>Canvas: matches / not matches
else
Canvas-->>Emitter: eligible
end
alt eligible
Emitter->>Canvas: perform raycast & dispatch pointer events
Canvas-->>Emitter: hit (may return early) / miss
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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: 0
🧹 Nitpick comments (3)
packages/ui/src/component/UICanvas.ts (1)
181-204: Consider clearingeventCamerawhenrenderModechanges away from WorldSpace.The setter correctly warns when
eventCamerais set in non-WorldSpace mode, but theeventCameravalue persists. If a user setseventCamerawhile in WorldSpace, then later changesrenderModeto ScreenSpaceCamera, theeventCamerareference remains set (though ineffective). This could cause confusion when switching back to WorldSpace.Consider clearing
eventCamerawhenrenderModechanges, or documenting this behavior explicitly. For reference, here's how therenderModesetter could be updated:set renderMode(mode: CanvasRenderMode) { const preMode = this._renderMode; if (preMode !== mode) { this._renderMode = mode; + if (mode !== CanvasRenderMode.WorldSpace && this._eventCamera) { + Logger.warn("EventCamera has been cleared because render mode is no longer WorldSpace."); + this._eventCamera = null; + } this._updateCameraObserver(); this._setRealRenderMode(this._getRealRenderMode()); } }tests/src/ui/UICanvas.test.ts (2)
304-321: Unused variablecamera2in test.The
camera2variable is created but never used in this test. Consider removing it or adding assertions that validate behavior with multiple cameras.it("EventCamera for WorldSpace", () => { - const camera2 = root.createChild("camera2").addComponent(Camera); - rootCanvas.renderMode = CanvasRenderMode.WorldSpace;
323-341: Consider adding test coverage for ScreenSpaceCamera mode.The test validates WorldSpace behavior well, but
_canProcessEventalso handles ScreenSpaceCamera mode (returningrenderCamera === camera). Adding a test case for ScreenSpaceCamera mode would improve coverage and document the expected behavior.Example additional test:
it("_canProcessEvent with ScreenSpaceCamera mode", () => { const camera2 = root.createChild("cameraSSC").addComponent(Camera); rootCanvas.renderMode = CanvasRenderMode.ScreenSpaceCamera; rootCanvas.renderCamera = camera; // @ts-ignore expect(rootCanvas._canProcessEvent(camera)).to.be.true; // @ts-ignore expect(rootCanvas._canProcessEvent(camera2)).to.be.false; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/ui/src/component/UICanvas.ts(4 hunks)packages/ui/src/input/UIPointerEventEmitter.ts(1 hunks)tests/src/ui/UICanvas.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/src/ui/UICanvas.test.ts (3)
packages/core/src/index.ts (1)
Camera(16-16)packages/ui/src/index.ts (1)
CanvasRenderMode(21-21)packages/ui/src/component/index.ts (1)
UICanvas(1-1)
🔇 Additional comments (5)
packages/ui/src/component/UICanvas.ts (3)
84-86: LGTM! Field declaration follows established patterns.The
@ignoreClonedecorator is correctly applied since theeventCamerais manually handled in_cloneTo, consistent with how_renderCamerais managed.
307-319: Logic is correct and handles all render modes appropriately.The
_canProcessEventmethod correctly:
- Returns
renderCamera === camerafor ScreenSpaceCamera mode (consistent with_canRender)- Filters by
eventCamerain WorldSpace when set, otherwise allows all cameras- Returns
truefor ScreenSpaceOverlay (implicit via the finalreturn true)However, there's a subtle difference from
_canRender: ScreenSpaceOverlay is not explicitly handled here, but since Overlay canvases are processed separately inUIPointerEventEmitter.processRaycast(lines 44-54), this is acceptable.
444-454: Cloning logic correctly mirrors therenderCamerapattern.The implementation properly resolves the eventCamera's entity within the cloned hierarchy when possible, falling back to the original reference otherwise. This ensures cloned canvases with local event cameras work correctly.
packages/ui/src/input/UIPointerEventEmitter.ts (1)
85-92: Correct integration of the new event processing predicate.The change from
_canRender(camera)to_canProcessEvent(camera)properly separates rendering eligibility from event processing eligibility. This allows WorldSpace canvases to filter which cameras can generate pointer events while_canRendercontinues to control rendering behavior.tests/src/ui/UICanvas.test.ts (1)
343-363: Test correctly validates cloning behavior for eventCamera.The test verifies both external camera references and local (child entity) camera references are properly handled during cloning, which matches the implementation in
_cloneTo.
| return this._eventCamera === camera; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
这里有点绕，比较通顺的逻辑应该是先判断 event Camera 相关
There was a problem hiding this comment.
已经重新组织了 _canProcessEvent 方法，请看最新提交
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui/src/component/UICanvas.ts (1)
307-325: Consider basing_canProcessEventon_realRenderModeand unifying comment languageRight now
_canProcessEventbranches onthis._renderMode. However,_getRealRenderMode()can downgradeScreenSpaceCameratoScreenSpaceOverlaywhenrenderCamerais missing/disabled. In that fallback, the canvas will render as overlay but_canProcessEventwill still behave as if it wereScreenSpaceCameraand returnfalsefor all cameras because_renderCameraisnull. If the intent is for pointer events to follow the actual effective render mode, using_realRenderMode(with aNonefallback to_renderMode) would be more consistent.For example:
- _canProcessEvent(camera: Camera): boolean { - // WorldSpace模式下，如果设置了eventCamera，则只允许该相机处理事件 - if (this._renderMode === CanvasRenderMode.WorldSpace && this._eventCamera) { + _canProcessEvent(camera: Camera): boolean { + const mode = + this._realRenderMode !== CanvasRealRenderMode.None ? this._realRenderMode : this._renderMode; + + // In WorldSpace, if eventCamera is set, only that camera can process events. + if (mode === CanvasRenderMode.WorldSpace && this._eventCamera) { return this._eventCamera === camera; } - // ScreenSpaceCamera模式下，只允许renderCamera处理事件（与_canRender保持一致） - if (this._renderMode === CanvasRenderMode.ScreenSpaceCamera) { + // In ScreenSpaceCamera, only renderCamera can process events (consistent with _canRender). + if (mode === CanvasRenderMode.ScreenSpaceCamera) { return this._renderCamera === camera; } - // ScreenSpaceOverlay模式和WorldSpace未设置eventCamera时，所有相机都可以处理事件 + // In ScreenSpaceOverlay and WorldSpace without eventCamera, all cameras can process events. return true; }This also replaces the mixed Chinese comments with English, which keeps the file’s comment language consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/component/UICanvas.ts(4 hunks)
🔇 Additional comments (3)
packages/ui/src/component/UICanvas.ts (3)
85-87: Initialization and cloning flags for_eventCameraare consistent with existing camera fields
_eventCamerais initialized tonulland marked with@ignoreClone, with cloning handled explicitly in_cloneTo, mirroring how_renderCamerais treated. This is a clean, predictable way to manage the optional reference.
181-205:eventCameraaccessor semantics match the documented behaviorThe getter/setter and warnings align with the documentation: the camera is only effective in
WorldSpace, but the value is still stored (so it starts working immediately ifrenderModeis later switched toWorldSpace), and entity nesting checks mirrorrenderCamera’s validation. No functional issues spotted here.
449-459: Event camera cloning logic correctly mirrorsrenderCamerahandlingThe
_cloneToblock foreventCamerareuses_targetTempPathandEntity._getEntityHierarchyPath/_getEntityByHierarchyPathin the same way asrenderCamera, with a safe fallback to the originaleventCamerareference when the hierarchy lookup fails. This is a solid, predictable cloning strategy.
| } | ||
|
|
||
| // ScreenSpaceCamera mode: only renderCamera can process events (consistent with _canRender) | ||
| if (this._renderMode === CanvasRenderMode.ScreenSpaceCamera) { |
There was a problem hiding this comment.
this._renderMode === CanvasRenderMode.ScreenSpaceCamera && this._renderCamara
| Entity._getEntityByHierarchyPath(targetRoot, paths).getComponent(Camera) | ||
| : renderCamera; | ||
| } | ||
| const eventCamera = this._eventCamera; |
There was a problem hiding this comment.
这两个 if 大部分代码都相同，可以试试复用一下。
cptbtptpbcptdtptp
left a comment
cptbtptpbcptdtptp
left a comment
There was a problem hiding this comment.
质量很好的 PR，有两个小建议可以修复一下。
- Extract _cloneCamera method to reduce code duplication - Add null check for _renderCamera in _canProcessEvent
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/ui/src/component/UICanvas.ts (3)
85-87: eventCamera lifecycle and warnings look good; minor optional behavior tweakThe
_eventCamerafield, its getter/setter, and the clearing logic inrenderModewhen leavingWorldSpacetogether match the intended behavior and keep existing setups unchanged wheneventCamerais unset. This looks solid.One small behavioral nit:
eventCameracurrently logs a warning when set whilerenderModeis notWorldSpacebut still accepts and stores the value. If you expect users to often “preconfigure”eventCamerabefore switchingrenderModetoWorldSpace, this warning may be a bit noisy; you might consider either:
- Downgrading it (e.g., to an info/debug‑level log), or
- Only warning when leaving
WorldSpacewhileeventCamerais set (you already do this in therenderModesetter).Not required for correctness, just a usability consideration.
Also applies to: 141-149, 186-209
308-310: Align _canProcessEvent with _realRenderMode to match ScreenSpaceCamera → Overlay fallback
_canProcessEventcurrently branches on_renderModeand_renderCamera. WhenrenderMode === ScreenSpaceCamerabut the render camera is effectively not usable (e.g., disabled),_getRealRenderMode()downgrades the canvas toScreenSpaceOverlay, but_canProcessEventwill still only allow events from_renderCamera. That can lead to a situation where the canvas renders as overlay but never receives events from active cameras.To make event routing follow the actual mode (as used by
_prepareRenderand_updateSortDistance), consider basing the decision on_realRenderModeinstead of_renderMode:- _canProcessEvent(camera: Camera): boolean { - // WorldSpace mode: if eventCamera is set, only that camera can process events - if (this._renderMode === CanvasRenderMode.WorldSpace && this._eventCamera) { - return this._eventCamera === camera; - } - - // ScreenSpaceCamera mode: only renderCamera can process events (consistent with _canRender) - if (this._renderMode === CanvasRenderMode.ScreenSpaceCamera && this._renderCamera) { - return this._renderCamera === camera; - } - - // ScreenSpaceOverlay mode and WorldSpace without eventCamera: all cameras can process events - return true; - } + _canProcessEvent(camera: Camera): boolean { + const mode = this._realRenderMode; + + // WorldSpace mode: if eventCamera is set, only that camera can process events + if (mode === CanvasRenderMode.WorldSpace && this._eventCamera) { + return this._eventCamera === camera; + } + + // ScreenSpaceCamera mode: only the active renderCamera can process events + if (mode === CanvasRenderMode.ScreenSpaceCamera && this._renderCamera) { + return this._renderCamera === camera; + } + + // ScreenSpaceOverlay, WorldSpace without eventCamera, or None: all cameras can process events + return true; + }This keeps the new
eventCamerabehavior but avoids divergence between “real” render mode and event routing in ScreenSpaceCamera fallback cases.Also applies to: 312-329, 705-712
441-463: Make _cloneCamera null‑aware and type‑safe for both renderCamera and eventCamera
_cloneCamerais typed as taking and returningCamera, but:
- It is called with
this._eventCamera, which isCamera | null.- Its implementation already handles a falsy
camera(if (!camera) return camera;), so the signature doesn’t reflect actual usage.- If
Entity._getEntityByHierarchyPathorgetComponent(Camera)fail, you can end up returningundefined/nullwhere aCamerais expected.To better match usage and avoid future typing issues (especially if
strictNullChecksis enabled), you can make_cloneCamerageneric overCamera | nulland be defensive about missing targets:- private _cloneCamera(camera: Camera, srcRoot: Entity, targetRoot: Entity): Camera { - if (!camera) { - return camera; - } - const paths = UICanvas._targetTempPath; - // @ts-ignore - const success = Entity._getEntityHierarchyPath(srcRoot, camera.entity, paths); - // @ts-ignore - return success - ? // @ts-ignore - Entity._getEntityByHierarchyPath(targetRoot, paths).getComponent(Camera) - : camera; - } + private _cloneCamera<T extends Camera | null>(camera: T, srcRoot: Entity, targetRoot: Entity): T { + if (!camera) { + return camera; + } + const paths = UICanvas._targetTempPath; + // @ts-ignore + const success = Entity._getEntityHierarchyPath(srcRoot, camera.entity, paths); + if (!success) { + return camera; + } + // @ts-ignore + const targetEntity = Entity._getEntityByHierarchyPath(targetRoot, paths); + const targetCamera = targetEntity?.getComponent(Camera) as T | null; + return (targetCamera ?? camera) as T; + }With this,
target.renderCamera = this._cloneCamera(this._renderCamera, ...)infersT = Camera, andtarget.eventCamera = this._cloneCamera(this._eventCamera, ...)infersT = Camera | null, matching the property types and avoiding implicitany/null leakage.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| return this._eventCamera === camera; | ||
| } | ||
| return true; | ||
| } |
3 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Fix
What is the current behavior?
Closes #2793
当前在 WorldSpace 模式下，场景中存在多个相机时，所有相机都可以触发 UI 事件，无法指定特定相机处理交互，导致多相机场景下的事件处理歧义。
What is the new behavior?
eventCamera属性：允许为 WorldSpace 模式的 UICanvas 指定事件处理相机_canProcessEvent()方法实现基于相机的事件过滤Does this PR introduce a breaking change?
否 - 完全向后兼容，现有代码无需修改
eventCamera.mov
Summary by CodeRabbit
New Features
Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.