Support mid-drag modifier changes - #733
Merged
Merged
Conversation
Previously a scene-node drag froze its (button, modifier) combo at drag-start: changing the held modifier mid-gesture had no effect. This partitions a physical drag into one logical segment per combo. When the held modifier changes, the client ends the current segment and -- if the new combo is bound -- starts a fresh one under it, preserving the grab geometry (plane, grab point, instance) so the drag continues without a visual jump; only the addressed callback set changes. Modifier changes are picked up from both pointermove and (for stationary switches) window keydown/keyup during the drag. A switch to an unbound combo drops the gesture into a dormant gap (no messages) so callbacks see properly paired start/end per bound combo; re-entering a bound combo starts a new segment. The wire message already carries `modifier`, so this is client-side segmentation plus docs -- no message-schema change. `SceneNodeDragEvent` keeps a single `modifier` field that now identifies the active segment. Tests: planModifierTransition unit cases (vitest), a server-side multi-segment dispatch/bookkeeping test, and an e2e test that adds Shift mid-drag and asserts the cmd/ctrl segment ends before release.
Covers the gap surfaced in review: switching to an unbound modifier combo mid-drag must suspend the gesture (dormant), not end it. Asserts the active segment ends on the switch (before release), and that re-entering a bound combo resumes with a fresh segment within the same button press -- which only holds if dormant is a suspend rather than a teardown. Both transitions are driven by key events with the pointer stationary, so this also exercises the keydown/keyup path.
Existing tests each exercised a single modifier switch. These add sequences within one drag: - server: one drag cycling cmd/ctrl -> cmd/ctrl+shift -> cmd/ctrl -> cmd/ctrl+shift (re-entering both combos), asserting each callback sees clean ordered start...end pairs and the active-drag map fully releases. - e2e: one continuous press cycling cmd/ctrl -> cmd/ctrl+shift -> dormant -> cmd/ctrl+shift -> cmd/ctrl, asserting exactly two segments per combo across repeated switching, re-entry, and resume-after-dormant. Uses async callbacks so the counters settle deterministically.
…ches Two review findings on the mid-drag modifier-switch mechanism: 1. The opening segment is now attributed to the modifier held on the promoting pointermove, not the one sampled at pointerdown. The drag layer's key listeners only install at promotion, so a change inside the pointerdown->threshold window was invisible: a promote-and-release gesture opened (and closed) a segment under a stale modifier. When the promotion-time combo is unbound the drag now begins dormant and resumes on the next bound combo (new planDragStart, unit-pinned). 2. transitionDragModifier reads bindings live from the scene-tree store instead of a drag-start snapshot, so callbacks registered or removed mid-drag take effect at the next switch. The snapshot field is gone from ActiveDragState. E2E pins for both: a pre-promotion Ctrl release must not fire a stale segment (and must resume on re-press), and a binding registered mid-drag must own the post-switch segment.
The pointerdown->promotion window fix attributed the opening segment to the promotion-time modifier but still planned it against the bindings array captured at pointerdown. A binding added or removed inside that window was invisible to the opening plan (e.g. a combo bound mid-window began dormant instead of owning the segment). beginDrag now reads dragBindings live from the scene-tree store for the opening plan; the pointerdown snapshot remains only as the candidacy gate.
Ending a modifier-drag the natural way -- modifier up a beat before the button -- previously fired a degenerate start/end pair on whatever combo remained bound (e.g. a bare on_drag registered alongside a modifier binding), since humans can't release both within the same frame. This reintroduced exactly the degenerate pair the promotion motion-threshold eliminated, one boundary later. A mid-drag switch now ends the old segment immediately but holds the new segment's start PENDING until confirmed: a 100ms timer (SWITCH_START_DELAY_MS), or sooner by pointer motion past the promotion threshold from the switch point. Release/cancel/blur inside the window discards the pending start outright -- no message was sent, none is owed -- and a further modifier change abandons it silently, which also swallows key-rollover pass-through combos when releasing multi-key combos one key at a time. Stationary switches still fire (timer path, imperceptibly later); the opening segment is never deferred (promotion's motion threshold is its confirmation). E2E pins the release order: bare + cmd/ctrl both bound, ctrl-drag ended modifier-first -> bare sees nothing, ctrl sees one clean pair, and a follow-up bare drag proves the binding was live.
The discard pin alone was satisfiable with the keyup path broken entirely (the pointerup would end the ctrl segment and bare would still read zero), and its keyup->mouseup gap raced the 100ms deferral timer on slow CI. Split into two gestures: A holds the button through the window so the keyup's pending bare start CONFIRMS on the timer (proving the keyup creates a pending segment, and doubling as the bare-binding positive control); B dispatches keyup + pointerup in one JS task -- both listeners are window-level -- making the inside-the-window release deterministic before asserting bare never budged. Also correct the on_drag docs: start fires when the press is confirmed as a drag (motion threshold), not at press.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We previously ignored modifier changes that happened mid-drag.
New behavior for modifier changes:
on_dragregistered alongside a modifier binding).Bindings are also read live at each switch, so callbacks registered or removed mid-drag take effect at the next modifier change.
cc mujocolab/mjviser#21, @kevinzakka if this sounds reasonable?