feat: add pointer up down and move event#1243
Conversation
WalkthroughAdds pointer events and data models (pointerdown/pointerup/pointermove, PointerEventData, RaycastResult); renames Region field Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Canvas
participant EventSystem
participant Player
participant Composition
participant Item
rect rgb(200,230,255)
note over User,Canvas: Pointer Down
User->>Canvas: native pointerdown
Canvas->>EventSystem: native event
EventSystem->>EventSystem: build PointerEventData (position, delta, raycast)
EventSystem->>Player: emit 'pointerdown'(PointerEventData)
Player->>Player: hitTest -> Region (includes item)
Player->>Composition: emit 'pointerdown'(PointerEventData, region)
Composition->>Item: emit 'pointerdown'(PointerEventData, region.item)
end
rect rgb(220,255,220)
note over User,Canvas: Pointer Move
User->>Canvas: native pointermove
Canvas->>EventSystem: native event
EventSystem->>Player: emit 'pointermove'(PointerEventData)
alt skipPointerMovePicking == false
Player->>Player: hitTest -> Region
Player->>Composition: emit 'pointermove'(PointerEventData, region)
Composition->>Item: emit 'pointermove'(PointerEventData, region.item)
else skipPointerMovePicking == true
Player->>Composition: emit 'pointermove'(PointerEventData) %% skip hitTest
end
end
rect rgb(255,240,200)
note over User,Canvas: Pointer Up / Click
User->>Canvas: native pointerup
Canvas->>EventSystem: native event
EventSystem->>Player: emit 'pointerup'(PointerEventData)
Player->>Player: hitTest -> Region
Player->>Composition: emit 'pointerup'(PointerEventData, region)
Composition->>Item: emit 'pointerup'(PointerEventData, region.item)
alt region.type == NOTIFY
Player->>Composition: emit 'click'(region with deprecated composition fields)
Player->>Item: emit 'click'(region.item info)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/effects-core/src/comp-vfx-item.ts (1)
169-179: Remove redundantcompContentfield.The region object now contains both
compContent: this.item(line 171) anditem(line 178), which is redundant. Since theRegiontype inclick-handler.tswas updated to use theitemfield, thecompContentfield should be removed.Apply this diff to remove the redundant field:
if (success) { const region = { - compContent: this.item, id: item.id, name: item.name, position: hitPositions[hitPositions.length - 1], parentId: item.parentId, hitPositions, behavior: hitParams.behavior, item, };packages/effects-core/src/composition.ts (1)
664-667: Fix 'end' payload to match new type {}.CompositionEvent['end'] is now an empty object. Emitting
{ composition: this }breaks the type contract and downstream handlers.Apply:
- this.emit('end', { composition: this }); + this.emit('end', {});packages/effects-core/src/plugins/interact/event-system.ts (2)
60-83: Unreachable disposed-target guard; velocity calc likely wrong.
if (!this.target)can’t trigger under the non‑null contract; remove it.vx/vycompute acceleration:(dx - lastTouch.dx)/dt. For pointer delta, use per‑event displacement/time:dx/dt,dy/dt. Also ensurelastTouchupdates every move.Apply:
- if (!this.target) { - logger.warn('Trigger TouchEvent after EventSystem is disposed.'); - - return { - x, y, vx: 0, vy, dx, dy, ts, width: 0, height: 0, origin: event, - }; - } - const { width, height } = this.target; + const { width, height } = this.target; @@ - if (lastTouch) { - const dt = ts - lastTouch.ts; - - vx = ((dx - lastTouch.dx) / dt) || 0; - vy = ((dy - lastTouch.dy) / dt) || 0; - lastTouch = { dx, dy, ts }; - } + if (lastTouch) { + const dt = ts - lastTouch.ts; + vx = (dx / dt) || 0; + vy = (dy / dt) || 0; + } + lastTouch = { dx, dy, ts };
115-121: Update currentTouch on move to report per‑event deltas.
dx = x - currentTouch.xuses the position from touchstart becausecurrentTouchisn’t updated. Update it each move to avoid growing deltas.Apply:
[touchmove]: event => { if (currentTouch && this.enabled) { - const cood = getCoord(getTouch(event)); - - x = cood.x; - y = cood.y; - this.dispatchEvent(EVENT_TYPE_TOUCH_MOVE, getTouchEventValue(event, x, y, x - currentTouch.x, y - currentTouch.y)); + const touch = getTouch(event); + const cood = getCoord(touch); + const nx = cood.x; + const ny = cood.y; + const dx = nx - currentTouch.x; + const dy = ny - currentTouch.y; + x = nx; y = ny; + this.dispatchEvent(EVENT_TYPE_TOUCH_MOVE, getTouchEventValue(event, x, y, dx, dy)); + currentTouch = { ...currentTouch, x, y, clientX: touch.clientX, clientY: touch.clientY, ts: performance.now() }; } },
🧹 Nitpick comments (4)
packages/effects-core/src/plugins/interact/event-system.ts (2)
41-43: Non‑null target contract — clean up downstream null checks.With
private target: HTMLCanvasElement, branches guarding!this.targetand?.optional chaining become dead code. Remove them or maketargetnullable again and set to null on dispose—pick one and be consistent.Apply:
- constructor ( - private target: HTMLCanvasElement, - public allowPropagation = false, - ) { } + constructor (private target: HTMLCanvasElement, public allowPropagation = false) { }Then (see below diffs) remove the dead guard in getTouchEventValue and
?.in dispose.
177-186: Dispose: remove optional chaining and redundant guard.Align with non‑null target.
Apply:
- dispose (): void { - if (this.target) { - this.handlers = {}; - - Object.keys(this.nativeHandlers).forEach(name => { - this.target?.removeEventListener(String(name), this.nativeHandlers[name]); - }); - this.nativeHandlers = {}; - } - } + dispose (): void { + this.handlers = {}; + Object.keys(this.nativeHandlers).forEach(name => { + this.target.removeEventListener(String(name), this.nativeHandlers[name]); + }); + this.nativeHandlers = {}; + }packages/effects-core/src/events/types.ts (1)
12-24: Item pointer events: types OK; fix pointerup doc.Payloads align with usage (down/up no args, move carries PointerEventData). The comment for pointerup duplicates “按下”; should read “抬起”.
Apply:
/** - * 元素按下事件 + * 元素抬起事件 */ ['pointerup']: [],packages/effects/src/player.ts (1)
58-61: Expose skipPointerMovePicking via config (optional).Consider adding a PlayerConfig flag to control this at construction time.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/effects-core/src/comp-vfx-item.ts(1 hunks)packages/effects-core/src/composition.ts(1 hunks)packages/effects-core/src/events/types.ts(4 hunks)packages/effects-core/src/plugins/interact/click-handler.ts(3 hunks)packages/effects-core/src/plugins/interact/event-system.ts(3 hunks)packages/effects/src/player.ts(5 hunks)packages/effects/src/types.ts(3 hunks)plugin-packages/editor-gizmo/demo/src/gizmo-3d.ts(1 hunks)plugin-packages/editor-gizmo/demo/src/gizmo-wireframe.ts(1 hunks)plugin-packages/editor-gizmo/demo/src/gizmo.ts(1 hunks)plugin-packages/editor-gizmo/demo/src/scene.ts(1 hunks)web-packages/demo/src/interactive.ts(2 hunks)web-packages/test/unit/src/effects/player-event.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/effects-core/src/composition.ts (3)
packages/effects-core/src/events/event-emitter.ts (1)
EventEmitter(19-91)packages/effects-core/src/events/types.ts (1)
CompositionEvent(34-72)packages/effects-core/src/utils/index.ts (2)
Disposable(36-38)LostHandler(44-46)
packages/effects-core/src/plugins/interact/event-system.ts (1)
packages/effects-core/src/utils/index.ts (1)
Disposable(36-38)
web-packages/demo/src/interactive.ts (1)
packages/effects-core/src/vfx-item.ts (2)
composition(184-186)composition(191-199)
packages/effects-core/src/plugins/interact/click-handler.ts (1)
plugin-packages/model/src/runtime/math.ts (2)
Vector2(10-10)Vector3(11-11)
packages/effects-core/src/events/types.ts (1)
packages/effects-core/src/plugins/interact/click-handler.ts (2)
PointerEventData(79-83)Region(58-66)
packages/effects/src/player.ts (6)
packages/effects-core/src/events/event-emitter.ts (1)
EventEmitter(19-91)packages/effects/src/types.ts (1)
PlayerEvent(160-217)packages/effects-core/src/plugins/interact/event-system.ts (5)
EVENT_TYPE_CLICK(4-4)EVENT_TYPE_TOUCH_START(5-5)EVENT_TYPE_TOUCH_END(7-7)EVENT_TYPE_TOUCH_MOVE(6-6)TouchEventType(9-20)packages/effects-core/src/plugins/interact/click-handler.ts (2)
Region(58-66)PointerEventData(79-83)packages/effects-core/src/vfx-item.ts (2)
composition(184-186)composition(191-199)packages/effects-core/src/composition.ts (1)
Composition(132-1028)
packages/effects/src/types.ts (1)
packages/effects-core/src/plugins/interact/click-handler.ts (2)
Region(58-66)PointerEventData(79-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (22)
plugin-packages/editor-gizmo/demo/src/gizmo-3d.ts (1)
16-23: LGTM! Simplified event destructuring.The change correctly removes
playerfrom the event payload destructuring and relies on the outer-scopedplayervariable. This aligns with the PR's goal of simplifying event payloads.plugin-packages/editor-gizmo/demo/src/gizmo.ts (1)
21-28: LGTM! Consistent with other demo files.The event payload destructuring is correctly updated to use only
id, relying on the outer-scopedplayervariable for composition and item access.plugin-packages/editor-gizmo/demo/src/gizmo-wireframe.ts (1)
16-23: LGTM! Event destructuring properly updated.The change correctly simplifies the event payload by extracting only
id, consistent with the updated event payload structure across the codebase.plugin-packages/editor-gizmo/demo/src/scene.ts (1)
12-19: LGTM! Consistent event payload handling.The event destructuring is correctly updated in line with the other demo files, removing the redundant
playerextraction.packages/effects-core/src/plugins/interact/click-handler.ts (3)
1-2: LGTM! More specific import.The direct import of
Vector2from the specific module is appropriate and may help with tree-shaking.
58-66: LGTM! Clearer field naming.The rename from
compContenttoitemin theRegiontype improves clarity and better represents the field's purpose.
79-88: LGTM! Well-structured pointer event types.The new
PointerEventDataandRaycastResultclasses provide a clean structure for pointer event handling, with clear property types and appropriate defaults.web-packages/test/unit/src/effects/player-event.spec.ts (1)
25-33: LGTM! Test correctly updated for simplified payload.The test handler signature is properly updated to reflect the removal of
playerfrom the update event payload, checking only theplayingboolean property as expected.web-packages/demo/src/interactive.ts (3)
22-30: LGTM! Player-level pointer events added.The new pointer event handlers (
pointerdown,pointerup,pointermove) properly demonstrate the new pointer event functionality, accessing the appropriate payload properties (pointerCurrentRaycast.item,position,delta).
57-65: LGTM! Composition-level pointer events added.The updated click handler and new pointer event handlers properly demonstrate composition-level event handling with the new event payload structure.
69-74: LGTM! Item-level pointer events added.The new pointer event handlers for the item level properly demonstrate pointer interaction tracking at the item scope.
packages/effects-core/src/composition.ts (1)
132-132: Type alignment with new event map looks good.Extending EventEmitter with the non‑generic CompositionEvent matches the updated event typings.
packages/effects-core/src/plugins/interact/event-system.ts (2)
28-32: New PointerEventType enum — OK.Enum values and names are clear and consistent with usage.
146-147: Binding without optional chaining — good.Safe under the non‑null target contract.
packages/effects-core/src/events/types.ts (2)
34-51: Composition pointer events: types OK.Non‑generic CompositionEvent with pointer payloads looks consistent.
66-66: 'end' payload changed to empty object — ensure emitters updated.See Composition.update; emit
{}instead of{ composition: this }.packages/effects/src/types.ts (3)
1-1: Expanded imports — OK.
PointerEventDataimport matches new event payloads.
160-179: PlayerEvent adds pointer events — OK.Shapes align with core. Click keeps composition metadata, which is useful at Player level.
201-201: Update event payload simplified — OK.Matches emitter usage in Player.
packages/effects/src/player.ts (3)
30-30: Type change to non‑generic PlayerEvent — OK.Matches updated public types.
158-164: New event bindings — OK.Wires click/touch to new pointer handlers as expected.
861-893: Click flow — OK; confirm multi‑hit semantics.Emits once per hit region across compositions. Ensure consumers expect multiple click events for a single pointer action.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/effects/src/player.ts (1)
925-963: Fix pointermove payload to item and correct delta to use dx/dy.
- ItemEvent['pointermove'] requires PointerEventData; currently
hitItem.emit(eventName)omits it.eventData.deltashould use per-event displacemente.dx/e.dy, not velocitye.vx/e.vy.Apply:
- eventData.position.x = (x + 1) / 2 * width; - eventData.position.y = (y + 1) / 2 * height; - eventData.delta.x = e.vx * width; - eventData.delta.y = e.vy * height; + eventData.position.x = (x + 1) / 2 * width; + // TODO: verify origin; if 0 at top-left is desired, map Y via (1 - y) / 2 * height. + eventData.position.y = (y + 1) / 2 * height; + eventData.delta.x = e.dx * width; + eventData.delta.y = e.dy * height; @@ - hitComposition.emit(eventName, eventData); - hitItem.emit(eventName); + hitComposition.emit(eventName, eventData); + if (eventName === 'pointermove') { + hitItem.emit(eventName, eventData); + } else { + hitItem.emit(eventName); + }Optional improvements:
- Use the actual hit point if available.
- if (hitRegion) { - raycast.point = hitRegion.position; - raycast.item = hitRegion.item; - } + if (hitRegion) { + raycast.point = hitRegion.hitPositions?.[hitRegion.hitPositions.length - 1] ?? hitRegion.position; + raycast.item = hitRegion.item; + }
- Consider resolving top-most by render order when scanning compositions (e.g., iterate
this.getCompositions()or sort bygetIndex()).
🧹 Nitpick comments (2)
packages/effects-core/src/events/types.ts (1)
12-23: Confirm payload design: ItemEvent pointerdown/up vs pointermove.ItemEvent has [] for pointerdown/up but PointerEventData for pointermove. Is this asymmetry intentional? If consumers need coordinates on press/release too, consider carrying PointerEventData for all three for consistency. If not, please document the rationale.
packages/effects/src/player.ts (1)
863-897: Click emits for every hit region; confirm intent vs top-most only.You collect all hit results across compositions and emit per hit. Pointer path, however, targets a single (last/top-most) region. Please confirm if click should:
- emit once for the top-most region (consistent with pointer), or
- emit for all hits (current behavior).
If single target is desired, select the last entry only:
- for (const hitResult of hitResults) { + const hitResult = hitResults.at(-1); + if (hitResult) { const behavior = hitResult.behavior || spec.InteractBehavior.NOTIFY; const hitComposition = hitResult.item.composition; if (!hitComposition) { - continue; + return; } if (behavior === spec.InteractBehavior.NOTIFY) { const clickInfo = { player: this, ...hitResult, compositionId: hitComposition.id, compositionName: hitComposition.name }; this.emit('click', clickInfo); hitComposition.emit('click', clickInfo); } else if (behavior === spec.InteractBehavior.RESUME_PLAYER) { void this.resume(); } hitResult.item.emit('click', hitResult); - } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/effects-core/src/events/types.ts(4 hunks)packages/effects/src/player.ts(4 hunks)packages/effects/src/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effects/src/types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/effects/src/player.ts (4)
packages/effects-core/src/plugins/interact/event-system.ts (1)
TouchEventType(9-20)packages/effects-core/src/plugins/interact/click-handler.ts (2)
Region(58-66)PointerEventData(79-83)packages/effects-core/src/vfx-item.ts (2)
composition(184-186)composition(191-199)packages/effects-core/src/composition.ts (1)
Composition(132-1028)
packages/effects-core/src/events/types.ts (1)
packages/effects-core/src/plugins/interact/click-handler.ts (1)
PointerEventData(79-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/effects/src/player.ts (1)
160-164: Event bindings look good.Click and touch listeners are registered on EventSystem as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/effects/src/player.ts (1)
929-930: Critical: Usedx/dyfor per-event delta, notvx/vy.The delta calculation uses
e.vxande.vy, which represent velocity values, not per-event pixel deltas. According to theTouchEventTypedefinition, the correct fields aree.dxande.dy.Note: This issue was previously flagged and marked as "Addressed in commits d6c3037 to ebf52f5", but the code still uses
vx/vy. Please verify whether the fix was applied or reverted.Apply this diff to use the correct delta fields:
- eventData.delta.x = e.vx * width; - eventData.delta.y = e.vy * height; + eventData.delta.x = e.dx * width; + eventData.delta.y = e.dy * height;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects-core/src/events/types.ts(4 hunks)packages/effects/src/player.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effects-core/src/events/types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/effects/src/player.ts (4)
packages/effects-core/src/plugins/interact/event-system.ts (5)
EVENT_TYPE_CLICK(4-4)EVENT_TYPE_TOUCH_START(5-5)EVENT_TYPE_TOUCH_END(7-7)EVENT_TYPE_TOUCH_MOVE(6-6)TouchEventType(9-20)packages/effects-core/src/plugins/interact/click-handler.ts (2)
Region(58-66)PointerEventData(79-83)packages/effects-core/src/vfx-item.ts (2)
composition(184-186)composition(191-199)packages/effects-core/src/composition.ts (1)
Composition(132-1028)
🔇 Additional comments (4)
packages/effects/src/player.ts (4)
1-17: LGTM!The new imports for pointer event types and constants are correctly structured and align with the enhanced pointer event handling functionality.
58-61: LGTM!The
skipPointerMovePickingoptimization is well-implemented and defaults totruefor performance. This means pointer move events will skip hit testing by default, which is reasonable for reducing overhead in high-frequency events.
160-163: LGTM!Event listener bindings correctly map touch events to the new pointer event handlers.
863-897: LGTM!The refactored
onClickhandler correctly aggregates hit test results across all compositions and propagates click events with proper payloads to player, composition, and item targets. The null check for composition is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/effects/src/player.ts (2)
702-703: Fix delta calculation: use dx/dy instead of vx/vy.The delta should represent the per-event pixel movement, not velocity. According to the
TouchEventTypedefinition,dxanddyare the appropriate fields for this purpose, whilevxandvyrepresent velocity.- eventData.delta.x = e.vx * width; - eventData.delta.y = e.vy * height; + eventData.delta.x = e.dx * width; + eventData.delta.y = e.dy * height;
729-736: Add defensive null check for composition.While
hitItem.compositionshould exist when a region is hit, the cast on line 731 lacks defensive validation. Consider adding a null check to prevent potential runtime errors.if (hitRegion) { const hitItem = hitRegion.item; const hitComposition = hitItem.composition as Composition; + if (!hitComposition) { + return; + } + this.emit(eventName, eventData);
🧹 Nitpick comments (2)
packages/effects/src/player.ts (2)
225-228: Remove commented-out code.The old click handler is commented out but should be removed entirely to keep the codebase clean.
- // this.event.addEventListener(EVENT_TYPE_CLICK, this.onClick.bind(this)); this.event.addEventListener(EVENT_TYPE_TOUCH_START, this.onPointerDown.bind(this));
688-696: Consider simplifying the hit-testing condition.The double negative
!(type === PointerEventType.PointerMove && this.skipPointerMovePicking)is harder to read than necessary.- if (!(type === PointerEventType.PointerMove && this.skipPointerMovePicking)) { + const shouldSkipPicking = type === PointerEventType.PointerMove && this.skipPointerMovePicking; + + if (!shouldSkipPicking) { for (const composition of this.compositions) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/effects-core/src/comp-vfx-item.ts(1 hunks)packages/effects-core/src/events/types.ts(3 hunks)packages/effects-core/src/plugins/interact/click-handler.ts(3 hunks)packages/effects-core/src/plugins/interact/event-system.ts(2 hunks)packages/effects/src/player.ts(4 hunks)packages/effects/src/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/effects/src/types.ts
- packages/effects-core/src/comp-vfx-item.ts
- packages/effects-core/src/plugins/interact/event-system.ts
- packages/effects-core/src/plugins/interact/click-handler.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T07:51:18.728Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:162-164
Timestamp: 2025-08-05T07:51:18.728Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the //ts-expect-error usage for accessing siblingComponent.splits (around lines 162-164) is temporary debugging code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
packages/effects/src/player.ts
🧬 Code graph analysis (2)
packages/effects/src/player.ts (3)
packages/effects-core/src/plugins/interact/event-system.ts (4)
EVENT_TYPE_TOUCH_START(9-9)EVENT_TYPE_TOUCH_END(11-11)EVENT_TYPE_TOUCH_MOVE(10-10)TouchEventType(13-24)packages/effects-core/src/plugins/interact/click-handler.ts (2)
Region(59-68)PointerEventData(81-85)packages/effects-core/src/composition.ts (1)
Composition(132-1032)
packages/effects-core/src/events/types.ts (1)
packages/effects-core/src/plugins/interact/click-handler.ts (2)
PointerEventData(81-85)Region(59-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/effects-core/src/events/types.ts (1)
1-77: LGTM! Well-structured pointer event integration.The new
PointerEventtype is properly integrated into bothItemEventandCompositionEvent, maintaining backward compatibility while adding new pointer interaction capabilities. The deprecation annotations follow proper conventions and provide clear migration signals.packages/effects/src/player.ts (3)
1-49: LGTM! Pointer event infrastructure properly set up.The imports and
skipPointerMovePickingfield are correctly added to support the new pointer event system. Defaulting totruefor skipping pointermove hit-testing is a sensible performance optimization.
672-682: LGTM! Handler methods properly delegate to unified logic.The three handler methods follow a clean delegation pattern, keeping the event-specific logic centralized in
handlePointerEvent.
733-733: Verify: Should Player emit pointer events globally?Currently, the Player only emits pointer events when
hitRegionexists (line 733 is inside theif (hitRegion)block). Depending on requirements, consumers might expect the Player to emit these events regardless of whether any interactive region was hit, allowing global pointer tracking.Please confirm whether this is the intended behavior or if Player should emit pointer events unconditionally.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/effects-core/src/plugins/interact/event-system.ts (2)
248-249: Use per-event delta (dx/dy) instead of velocity (vx/vy).
e.vxande.vyrepresent velocity (pixels per millisecond), not per-event delta. ForPointerEventData.delta, you should usee.dxande.dywhich represent the pixel delta since the last event.Apply this diff:
eventData.position.x = (x + 1) / 2 * width; eventData.position.y = (y + 1) / 2 * height; - eventData.delta.x = e.vx * width; - eventData.delta.y = e.vy * height; + eventData.delta.x = e.dx * width; + eventData.delta.y = e.dy * height;
275-282: Add defensive null check for composition.While
hitRegion.item.compositionshould typically exist when a region is hit, adding a null check prevents potential runtime errors if the composition reference is unexpectedly missing.Apply this diff:
if (hitRegion) { const hitItem = hitRegion.item; const hitComposition = hitItem.composition as Composition; + if (!hitComposition) { + return; + } + hitItem.emit(eventName, eventData); hitComposition.emit(eventName, eventData); this.engine.emit(eventName, eventData); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/effects-core/src/engine.ts(2 hunks)packages/effects-core/src/plugins/interact/event-system.ts(5 hunks)packages/effects/src/player.ts(3 hunks)web-packages/demo/src/interactive.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
Repo: galacean/effects-runtime PR: 691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
packages/effects/src/player.ts
📚 Learning: 2025-08-05T07:51:18.728Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:162-164
Timestamp: 2025-08-05T07:51:18.728Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the //ts-expect-error usage for accessing siblingComponent.splits (around lines 162-164) is temporary debugging code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
packages/effects/src/player.ts
🧬 Code graph analysis (3)
packages/effects-core/src/engine.ts (1)
packages/effects-core/src/plugins/interact/click-handler.ts (2)
Region(59-68)PointerEventData(81-85)
packages/effects-core/src/plugins/interact/event-system.ts (3)
packages/effects-core/src/utils/index.ts (1)
Disposable(36-38)packages/effects-core/src/plugins/interact/click-handler.ts (2)
Region(59-68)PointerEventData(81-85)packages/effects-core/src/composition.ts (1)
Composition(132-1032)
packages/effects/src/player.ts (1)
packages/effects-core/src/plugins/interact/click-handler.ts (2)
Region(59-68)PointerEventData(81-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/effects/src/player.ts (1)
45-54: LGTM: Clean public API for pointer move optimization.The
skipPointerMovePickingproperty is well-documented and correctly delegates to the underlying engine event system. The JSDoc clearly explains the performance trade-off.packages/effects-core/src/engine.ts (1)
40-43: LGTM: Pointer event signatures are well-typed.The new event signatures correctly use
Regionfor click events andPointerEventDatafor pointer lifecycle events, providing proper type safety for consumers.packages/effects-core/src/plugins/interact/event-system.ts (1)
275-282: Verify intended behavior: pointer events only emitted when over a hit target.The current implementation only emits pointer events (pointerdown/pointerup/pointermove) to the engine when
hitRegionexists (lines 275-282). This means:
- Player/composition listeners won't receive pointer events when the pointer is not over an interactive item
- For
pointermove, users tracking cursor position may miss events when moving over non-interactive areasIs this the intended behavior? Many pointer event systems emit events regardless of hit targets, with the hit information being null/empty when nothing is hit.
If you want pointer events to always emit (with or without a hit target), consider emitting to engine outside the
if (hitRegion)block:if (hitRegion) { const hitItem = hitRegion.item; const hitComposition = hitItem.composition as Composition; if (hitComposition) { hitItem.emit(eventName, eventData); hitComposition.emit(eventName, eventData); } } // Always emit to engine, regardless of hit this.engine.emit(eventName, eventData);
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/effects-core/src/plugins/interact/event-system.ts (2)
192-192: Translate comment to English.For codebase consistency, comments should be in English.
- // 收集所有的点击测试结果，click 回调执行可能会对 composition 点击结果有影响,放在点击测试执行完后再统一触发。 + // Collect all hit test results first. Click callbacks may affect composition hit test results, so emit events after all tests complete.
256-271: Consider simplifying event name mapping.The switch statement could be replaced with a more concise lookup approach.
- let eventName: 'pointerdown' | 'pointerup' | 'pointermove' = 'pointerdown'; - - switch (type) { - case PointerEventType.PointerDown: - eventName = 'pointerdown'; - - break; - case PointerEventType.PointerUp: - eventName = 'pointerup'; - - break; - case PointerEventType.PointerMove: - eventName = 'pointermove'; - - break; - } + const eventNameMap = { + [PointerEventType.PointerDown]: 'pointerdown', + [PointerEventType.PointerUp]: 'pointerup', + [PointerEventType.PointerMove]: 'pointermove', + } as const; + const eventName = eventNameMap[type];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects-core/src/plugins/interact/event-system.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/effects-core/src/plugins/interact/event-system.ts (4)
packages/effects-core/src/utils/index.ts (1)
Disposable(36-38)packages/effects-core/src/plugins/interact/click-handler.ts (2)
Region(59-68)PointerEventData(81-85)packages/effects-core/src/vfx-item.ts (2)
composition(184-186)composition(191-199)packages/effects-core/src/composition.ts (1)
Composition(132-1032)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/effects-core/src/plugins/interact/event-system.ts (2)
39-39: Verify pointer move event emission behavior.With
skipPointerMovePicking = true(the default), pointer move events skip hit testing (line 232), resulting inhitRegionbeing null. Since events are only emitted whenhitRegionis not null (lines 273-280), pointer move events are never emitted by default.Confirm whether this is intentional or if pointer move events should be emitted to the engine even when they don't hit any items.
155-158: The bound method references are properly stored and managed.The
.bind(this)methods added at lines 155-158 are stored inthis.handlersby theaddEventListenermethod and can be properly removed duringdispose(). Additionally,bindListenersis never called multiple times on the same EventSystem instance in the codebase—in engine.ts it's called once during initialization, and in three-display-object.ts a new EventSystem is instantiated each time rather than reusing the same instance. The listener cleanup concern does not apply.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/effects-core/src/events/types.ts (1)
37-50: Add explicit migration guidance in JSDoc comments for deprecated fields.The deprecated
compositionNameandcompositionIdfields are not used in the codebase. The implicit migration path is to useRegion.nameandRegion.idinstead, which are already available in theclickInfoobject. While the deprecation is safe, consider adding explicit JSDoc@deprecatedcomments with migration instructions to help users update their code.Example:
/** * @deprecated 2.8.0 - Use clickInfo.name instead */ compositionName: string, /** * @deprecated 2.8.0 - Use clickInfo.id instead */ compositionId: string,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects-core/src/events/types.ts(3 hunks)packages/effects/src/types.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
Repo: galacean/effects-runtime PR: 691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
packages/effects/src/types.ts
🧬 Code graph analysis (2)
packages/effects/src/types.ts (2)
packages/effects-core/src/events/types.ts (1)
PointerEvent(4-17)packages/effects-core/src/plugins/interact/click-handler.ts (1)
Region(59-68)
packages/effects-core/src/events/types.ts (1)
packages/effects-core/src/plugins/interact/click-handler.ts (2)
PointerEventData(81-85)Region(59-68)
🔇 Additional comments (6)
packages/effects-core/src/events/types.ts (3)
2-2: LGTM!The import correctly adds
PointerEventData, which is used by the newPointerEventtype definition.
4-17: LGTM!The
PointerEventtype is well-structured with appropriate payload types and clear JSDoc documentation for all three pointer events.
22-22: LGTM!Extending
ItemEventwithPointerEventvia intersection type correctly adds pointer events while preserving existing click and message events.packages/effects/src/types.ts (3)
1-1: LGTM!The import correctly adds
PointerEventfrom@galacean/effects-core, which is used to extend thePlayerEventtype.
161-178: Deprecation structure verified—code change is sound.The
PlayerEventtype correctly extendsPointerEventand the deprecated fields (player,compositionId,compositionName) are properly marked with@deprecated 2.8.0. Verification confirms:
- Migration path is clear: The implementation (packages/effects/src/player.ts:212-213) demonstrates the replacement pattern—use
e.composition.idinstead ofe.compositionIdande.composition.nameinstead ofe.compositionName.- Region type provides all necessary data: The
Regionobject includes thecompositionproperty, ensuring users have direct access to replacement values.- Deprecation is consistent: Matches the pattern applied to
CompositionEventandItemEvent.- Backwards compatible: Deprecated fields are still populated, allowing gradual migration.
200-206: The review comment is incorrect.The update event has always used an object payload, not a tuple. Evidence from the codebase shows:
- Actual emit pattern (player.ts lines 235–236, 538–539):
this.emit('update', { player: this, playing: ... })- Listener patterns throughout the codebase already expect objects:
player.on('update', (info: { playing: boolean }) => { ... })(player-event.spec.ts:25)player.on('update', e => { ... e.playing })(interactive.ts:36)The old tuple pattern you described (
(player, playing) => { ... }) does not exist in the codebase. The@deprecated 2.8.0marker on theplayerfield is a managed deprecation, not an unannounced breaking change. Existing listeners are already compatible with the current structure.Likely an incorrect or invalid review comment.
2 participants
Summary by CodeRabbit
New Features
Improvements
Deprecations