Camera input mapping system with backward-compatible legacy flag support#18379
Camera input mapping system with backward-compatible legacy flag support#18379georginahalpern wants to merge 52 commits into
Conversation
… cameraInputPlanning
Add a public resetInputMap() method to the CameraMovement base class (resets to empty array) and override it in ArcRotateCameraMovement and GeospatialCameraMovement to restore their camera-type default inputMap configurations. Extract default inputMap construction into private _createDefaultInputMap() methods to avoid duplication between constructors and resetInputMap(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…teCurrentFrameDeltas Add useMovementSystem getter/setter that creates ArcRotateCameraMovement on demand. Modify _checkInputs() to branch on this.movement: when enabled, call computeCurrentFrameDeltas() and apply rotation/zoom/pan deltas to alpha/beta/radius/target; when disabled (default), use the unchanged legacy inertial offset path. The panning logic mirrors the legacy path (view matrix inverse, panningAxis, mapPanning, panningDistanceLimit). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add _syncLegacyFlagsToInputMap() to ArcRotateCamera that applies legacy flag values (_useCtrlForPanning, _panningMouseButton, useAltToZoom) to the movement system's inputMap. Called when useMovementSystem is enabled and when attachControl is called with legacy flag arguments. This ensures backward compatibility: setting _useCtrlForPanning=false before enabling the movement system correctly removes the ctrl+keyboard→pan entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…meraMovement Add cameraMovement.test.ts with tests for: - resolveInteraction: source matching, first-match-wins, modifier matching (exact/absent/partial), button matching, touch count, 'none' fallback, empty inputMap - computeCurrentFrameDeltas: accumulator reset, speed multipliers, per-axis rotation speeds, inertia decay, zero inertia, zero-delta Add arcRotateCameraMovement.test.ts with tests for: - Default inputMap (6 entries with correct interaction mappings) - Default handlers (accumulate into base class accumulators) - resetInputMap (restores defaults after modification) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… cameraInputPlanning
… inputMapperBackCompat
⚡ Performance Test Results🟢 All performance tests passed — no regressions detected. |
… inputMapperBackCompat
… inputMapperBackCompat
🟢 Memory Leak Test Results13 passed, 0 leaked out of 13 scenarios 🟢 All memory leak tests passed — no leaks detected. Passed Scenarios (13)
|
|
WebGL2 visualization test reporter: |
⚡ Performance Test Results🟢 All performance tests passed — no regressions detected. |
|
Visualization tests for WebGPU |
RaananW
left a comment
RaananW
left a comment
There was a problem hiding this comment.
This is really big :)
A wonderful idea. I'll be honest and say I couldn't find any breaking changes, so it LGTM.
Would be great to combine a few of the visualization tests, as they take time and resources to render, if possible.
- arcRotateCameraKeyboardMoveInput: extract _applyUseAltToZoomToInputMap helper and call from both the setter and attachControl so a value cached before the camera is bound is flushed to the inputMap once the camera becomes available. - geospatialCameraKeyboardInput / geospatialCameraPointersInput: guard the deprecated sensitivity getter/setter shims with this.camera?. and ?? [] so they no longer crash when read or written before the input is attached to a camera. - inputMapper: add setInteractions (plural) that updates every matching entry and returns the count, with setInteraction JSDoc updated to point users at it (and getEntries) for multi-match scenarios. - Tests: regression test for the useAltToZoom flush-on-attach path, plus three setInteractions tests covering all-match update, no-match return, and wildcard condition matching. - Visualization: consolidate the three ArcRotate legacy inertia tests into a single side-by-side comparison test (#UAPORV#0) using vertical viewports with 2x hardware scaling for AA. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… inputMapperBackCompat
🟢 Memory Leak Test Results13 passed, 0 leaked out of 13 scenarios 🟢 All memory leak tests passed — no leaks detected. Passed Scenarios (13)
|
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
⚡ Performance Test Results❌ Failed Tests (1)
|
Master's recent BabylonJS#18428 made missing-doc warnings fail the TypeDoc CI on files changed by the PR. Add real descriptions to previously-empty @param tags and JSDoc comments to previously-undocumented members so the check passes again. - geospatialCamera.ts: describe @param tags on updateFlyToDestination and flyToAsync (targetYaw/Pitch/Radius/Center, flightDurationMs, easingFunction). - geospatialCameraMovement.ts: add doc comment on the constructor's parameter property limits, and add a real description on the computeCurrentFrameDeltas override. - geospatialCameraPointersInput.ts: add JSDoc + @param descriptions on the onButtonDown and onMultiTouch overrides. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🟢 Memory Leak Test Results13 passed, 0 leaked out of 13 scenarios 🟢 All memory leak tests passed — no leaks detected. Passed Scenarios (13)
|
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
⚡ Performance Test Results🟢 All performance tests passed — no regressions detected. |
|
Thanks for the review @RaananW! Pushed fixes for all three inline comments in 5dce2e5:
For the viz-test consolidation suggestion: collapsed the three Also pushed 7fc2ad1 to address the new TypeDoc CI failures from your #18428 — added missing param descriptions and JSDoc on a few previously-undocumented members in the geospatial files. |
| const camera = this.camera; | ||
| const input = camera.movement.input; | ||
|
|
||
| this._keyboardConditions.modifiers!.ctrl = this._ctrlPressed; |
There was a problem hiding this comment.
It'd be great if we could avoid this non-null assertion operator, maybe store the modifiers object seperately?
| * with framerate-independent semantics (via `CameraMovement.getFrameIndependentDecay`) | ||
| * without disturbing the general inertialRadiusOffset back-compat surface. | ||
| */ | ||
| private _zoomToMouseRadiusImpulse: number = 0; |
There was a problem hiding this comment.
The term impulse is confusing to me here, isn't this just an accumulation of delta? I think of impulse as the rate of change of acceleration, but that's not what this is right? Maybe more detail in the docstring would be helpful, particularly about the units of this value.
|
|
||
| private _isPanClick: boolean = false; | ||
| /** Cached resolved inputMap entry for the current pointer gesture */ | ||
| private _activeEntry: PointerInputMapEntry | null = null; |
There was a problem hiding this comment.
Nit: should we be using Nullable<> for consistency?
| public override onButtonDown(evt: IPointerEvent): void { | ||
| this._isPanClick = evt.button === this.camera._panningMouseButton; | ||
| this._pointerConditions.button = evt.button; | ||
| this._pointerConditions.modifiers!.ctrl = evt.ctrlKey; |
There was a problem hiding this comment.
It'd be great to avoid the non-null assertion operator - maybe also store modifiers seperately?
| const input = camera.movement.input; | ||
|
|
||
| // Update cached modifier state | ||
| this._keyboardConditions.modifiers!.ctrl = this._ctrlPressed; |
There was a problem hiding this comment.
It'd be great to avoid the ! here too - maybe store separately?
| break; | ||
|
|
||
| this._pointerConditions.button = evt.button; | ||
| this._pointerConditions.modifiers!.ctrl = evt.ctrlKey; |
There was a problem hiding this comment.
Could we avoid ! by storing modifiers seperately?
| * Used when calculating inertial decay. Default to 60fps | ||
| */ | ||
| private _prevFrameTimeMs: number = FrameDurationAt60FPS; | ||
| private _prevFrameTimeMs: number = 1000 / DefaultReferenceFrameRate; |
There was a problem hiding this comment.
Won't this get stale if the user sets referenceFrameRate later?
There was a problem hiding this comment.
Maybe we need a test to confirm that changing referenceFrameRate has the desired effect?
|
|
||
| switch (entry.source) { | ||
| case "pointer": | ||
| if ("button" in conditions && entry.button !== conditions.button) { |
There was a problem hiding this comment.
Curious about the use of "in" to check if a property is defined - is this preferrable to saying if (conditions.button !== undefined && ...)?
Summary
Replaces the scattered, ad-hoc input handling on
ArcRotateCameraandGeospatialCamerawith a declarative two-layer input mapping system, while preserving the behavior of all existing legacy boolean flags.Motivation
Camera input configuration today is spread across ad-hoc boolean flags (
useCtrlForPanning,multiTouchPanAndZoom,panningMouseButton,useAltToZoom, …) and hardcoded button-to-action logic in each input class. Adding new configurations requires another flag, another conditional, and often cross-input snooping. This doesn't scale.The new system replaces that with a declarative
inputMapresolved byresolveInteraction()that produces typedhandlers(orbit/zoom/pan, etc.). Per-frame deltas are accumulated and applied viacomputeCurrentFrameDeltas()for framerate-independent inertia.Playground demos
setInteraction()— remap ctrl+dragWhat's in this PR
Core (
packages/dev/core/src/Cameras)CameraMovementbase class with input-map resolution, typed handlers, and framerate-independent accumulators / inertia.ArcRotateCameraMovementsubclass and refreshedGeospatialCameraMovementsubclass, each with default input maps.cameraInteractions.tswith shared interaction descriptors andInputMapperclass.InputMapper.setInteraction(source, conditions, interaction)— targeted remapping of individual input entries without rebuilding the full inputMap (e.g. swap ctrl+left-drag from pan to rotate).InputMapper.getEntry(source, interaction)— find an entry to tweak properties like sensitivity.ArcRotateCameraandGeospatialCameranow always route through the movement system;_checkInputs()callscomputeCurrentFrameDeltas()and applies the resulting rotation / zoom / pan deltas. The legacyuseMovementSystemgetter/setter is kept as a deprecated no-op for source compatibility.Camera.inertiaconverted from a plain property to a get/set accessor soArcRotateCameracan override it to sync with the movement system._syncLegacyFlagsToInputMap()maps_useCtrlForPanning,_panningMouseButton, anduseAltToZoomonto the inputMap so existing apps that toggle these flags keep behaving the same.arcRotateCameraGamepadInput.ts) now feedmovement.rotationAccumulatedPixels/panAccumulatedPixels/zoomAccumulatedPixelsinstead of legacy inertial offsets.resetInputMap()on the base class and subclasses to restore default mappings after user customization.Tests
cameraMovement.test.ts,arcRotateCameraMovement.test.ts,geospatialCameraMovement.test.ts,arcRotateCameraFramerateIndependence.test.ts.arcRotateCameraInteraction.interaction.test.ts— 7 tests covering left-drag rotation, vertical drag, right-drag pan, ctrl+left-drag pan, mouse wheel zoom, arrow key rotation, and ctrl+arrow key panning.Backward compatibility
_syncLegacyFlagsToInputMap(), so apps that set e.g._useCtrlForPanning = falseor change_panningMouseButtonstill see the expected mapping changes.useMovementSystemproperty remains as a deprecated no-op getter/setter so any code referencing it still compiles.Out of scope / follow-ups
FreeCamera,FollowCamera,FlyCamera, VR / touch-joystick input sources — to be handled in subsequent PRs.