Add camera-based event filtering for WorldSpace UICanvas#2870
Add camera-based event filtering for WorldSpace UICanvas#2870cptbtptpbcptdtptp merged 13 commits intogalacean:dev/2.0from
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds eventCamera support to UICanvas (storage, getter/setter, validation), per-camera event eligibility via _canProcessEvent, propagation of eventCamera during cloning, updates UIPointerEventEmitter to consult _canProcessEvent for pointer raycasts, and adds tests (with duplicated blocks) for these behaviors. 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 each canvas
Emitter->>Canvas: _canProcessEvent(camera)?
alt Canvas.renderMode == WorldSpace and Canvas.eventCamera != null
Canvas->>Camera: compare provided Camera to eventCamera
Camera-->>Canvas: match / no match
else Canvas.renderMode == ScreenSpaceCamera and Canvas.renderCamera != null
Canvas->>Camera: compare provided Camera to renderCamera
Camera-->>Canvas: match / no match
else
Canvas-->>Emitter: eligible
end
alt eligible
Emitter->>Canvas: perform raycast & dispatch events
Canvas-->>Emitter: hit (may return early) / miss
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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.
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ui/src/component/UICanvas.ts`:
- Around line 444-455: In _cloneCamera, when
Entity._getEntityByHierarchyPath(targetRoot, paths).getComponent(Camera) can be
null you should preserve the original camera instead of dropping it: after
resolving the target entity (using Entity._getEntityByHierarchyPath and the same
paths from Entity._getEntityHierarchyPath) check the result of
getComponent(Camera) and return that component if non-null, otherwise return the
original camera parameter; target the _cloneCamera function and the calls to
Entity._getEntityHierarchyPath / Entity._getEntityByHierarchyPath to add this
null-coalescing fallback.
- Around line 145-149: In UICanvas, stop clearing the stored event camera when
toggling render modes: remove the assignment this._eventCamera = null (and the
associated Logger.warn about clearing) inside the block that checks preMode ===
CanvasRenderMode.WorldSpace && mode !== CanvasRenderMode.WorldSpace; instead
leave this._eventCamera intact so it’s simply ignored when mode !==
CanvasRenderMode.WorldSpace, preserving user configuration across mode switches.
- Around line 317-322: The event filtering in _canProcessEvent currently checks
_renderMode instead of _realRenderMode, causing events to be rejected when a
render mode falls back (e.g., ScreenSpaceCamera -> ScreenSpaceOverlay); change
the condition to use _realRenderMode so the method reads the effective mode when
deciding whether to require _eventCamera, and keep the rest of the logic
(_eventCamera comparison and fallback to _canRender(camera)) unchanged; update
references in _canProcessEvent to replace _renderMode with _realRenderMode to
ensure ScreenSpaceCamera fallback behavior is handled correctly.
| // Clear eventCamera when renderMode changes away from WorldSpace | ||
| if (preMode === CanvasRenderMode.WorldSpace && mode !== CanvasRenderMode.WorldSpace && this._eventCamera) { | ||
| Logger.warn("EventCamera has been cleared because render mode is no longer WorldSpace."); | ||
| this._eventCamera = null; | ||
| } |
There was a problem hiding this comment.
Don’t clear eventCamera on render mode switches.
On Line 146–149, clearing the reference loses user configuration when toggling modes at runtime. Ignoring it outside WorldSpace is enough; state reset is unnecessary and can break expected behavior.
💡 Proposed fix
- // Clear eventCamera when renderMode changes away from WorldSpace
- if (preMode === CanvasRenderMode.WorldSpace && mode !== CanvasRenderMode.WorldSpace && this._eventCamera) {
- Logger.warn("EventCamera has been cleared because render mode is no longer WorldSpace.");
- this._eventCamera = null;
- }
+ if (preMode === CanvasRenderMode.WorldSpace && mode !== CanvasRenderMode.WorldSpace && this._eventCamera) {
+ Logger.warn("eventCamera is ignored unless renderMode is WorldSpace.");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/component/UICanvas.ts` around lines 145 - 149, In UICanvas,
stop clearing the stored event camera when toggling render modes: remove the
assignment this._eventCamera = null (and the associated Logger.warn about
clearing) inside the block that checks preMode === CanvasRenderMode.WorldSpace
&& mode !== CanvasRenderMode.WorldSpace; instead leave this._eventCamera intact
so it’s simply ignored when mode !== CanvasRenderMode.WorldSpace, preserving
user configuration across mode switches.
GuoLei1990
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2870 +/- ##
===========================================
- Coverage 78.96% 78.15% -0.82%
===========================================
Files 857 880 +23
Lines 93517 96111 +2594
Branches 9378 9585 +207
===========================================
+ Hits 73846 75113 +1267
- Misses 19522 20836 +1314
- Partials 149 162 +13
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:
|
4 participants
Summary
WorldSpace canvases previously had no way to control which camera handles their UI events — all cameras with overlapping viewports could trigger events, leading to unintended interactions in multi-camera setups.
This PR extends the camera property to WorldSpace mode for event detection:
camerais assigned, only that camera can trigger UI events on this canvas. If unset, events fall back to the highest-priority camera (existing behavior).Additionally:
renderCamera→camera(withrenderCameradeprecated), since it now serves both rendering and event roles across modes._canProcessEvent()to decouple event filtering from render filtering (_canRender), fixing a bug where ScreenSpaceCamera fallback to ScreenSpaceOverlay would block all events.User Value
Minimap without RenderTarget: A minimap camera uses a small viewport overlapping the main camera's full-screen viewport. Both cameras can see and raycast against WorldSpace UI in the scene. When the player clicks in the overlapping area, the minimap camera's ray could incorrectly trigger WorldSpace UI events meant for the main scene.
Overlay cameras without color clearing: Multiple cameras render to the same viewport without clearing the color buffer (e.g., for layered composition). All cameras can raycast against the same WorldSpace UI, causing duplicate or unintended event triggers.
By assigning
canvas.camera = mainCamera, developers can restrict event detection to the intended camera in both scenarios.Behavior Changes
Changes
_canProcessEvent()to decouple event filtering from render filtering (_canRender)_canProcessEvent()inUIPointerEventEmitterfor event dispatchrenderCamera→camera, deprecaterenderCameraas proxy_renderCamera→_cameraTest Plan
renderCameradeprecated proxy still works