feat(tools): touch interaction support with dispatcher fallback to mouse callbacks#2815
Open
sedghi wants to merge 2 commits into
Open
feat(tools): touch interaction support with dispatcher fallback to mouse callbacks#2815sedghi wants to merge 2 commits into
sedghi wants to merge 2 commits into
Conversation
…use callbacks Touch dispatchers now fall back to the mouse counterpart callback when a tool has no touch-specific handler (touchDragCallback to mouseDragCallback, pre/postTouchStartCallback to pre/postMouseDownCallback, touchTapCallback to mouseClickCallback), gated by the tool declaring Touch in supportedInteractionTypes. This makes the previously mouse-only entry tools (Brush, Scissors, PaintFill, MIPJumpToClick, ClickSegment, Sculptor, RegionSegment, WholeBodySegment) reachable by touch, and the rendering engine now sets touch-action none on enabled viewport elements so the browser no longer consumes viewport gestures.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTouch support is expanded across viewport touch-action handling, event callback fallback, gesture-aware tools, segmentation drawing lifecycles, a touch-focused example, and optional LAN development-server configuration. ChangesTouch interaction support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant TouchListener
participant Dispatcher
participant Tool
Browser->>TouchListener: emit touch start, drag, or tap
TouchListener->>Dispatcher: dispatch touch event
Dispatcher->>Tool: resolve touch callback
Dispatcher->>Tool: invoke mouse fallback when applicable
Tool->>Browser: update interaction state and rendering
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tools/src/tools/annotation/WholeBodySegmentTool.ts (1)
129-135: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuarantee cleanup when grow-cut fails.
If
runGrowCut()rejects,growCutDataremains populated and the cursor stays hidden. Move state, cursor, and render cleanup intofinally.Proposed fix
this._deactivateDraw(element); - await this.runGrowCut(); - - this.growCutData = null; - - resetElementCursor(element); - triggerAnnotationRenderForViewportUIDs([viewport.id]); + try { + await this.runGrowCut(); + } finally { + this.growCutData = null; + resetElementCursor(element); + triggerAnnotationRenderForViewportUIDs([viewport.id]); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/tools/annotation/WholeBodySegmentTool.ts` around lines 129 - 135, Update the flow around runGrowCut in WholeBodySegmentTool so cleanup always executes when it resolves or rejects: place growCutData reset, resetElementCursor(element), and triggerAnnotationRenderForViewportUIDs([viewport.id]) in a finally block. Preserve the existing grow-cut error propagation while ensuring state, cursor, and render cleanup are performed on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tools/examples/touchAllTools/index.ts`:
- Around line 179-189: Add ClickSegmentTool to every required demo setup:
register it, include it in the volume tool group, and append its toolName to the
selectable segmentation list alongside the existing segmentation tools. Use the
existing ClickSegmentTool symbol and preserve the current ordering and
configuration conventions.
- Around line 151-189: Add CrosshairsTool.toolName to the selectableToolNames
and selection setup, and update the volume tool group configuration to create
and register a second orthographic volume viewport. Ensure both volume viewports
participate in the Crosshairs linking/deactivation flow so Crosshairs is
reachable and functional with two viewports.
In `@packages/tools/src/tools/annotation/ClickSegmentTool.ts`:
- Around line 820-836: Update the touch path around runHoverProbe and
preMouseDownCallback so it awaits the runConfinementDryRun verdict before
invoking segmentation. Propagate or explicitly await the dry-run promise,
ensuring lastProbe reflects the completed 3D confinement check and rejected
regions return through notifySegmentationError without starting segmentation.
In `@packages/tools/src/tools/CrosshairsTool.ts`:
- Line 223: Update the interaction-mode selection around the enabled setting in
CrosshairsTool so coarse secondary pointers on hybrid devices do not force
mobile behavior for mouse input. Use the primary-pointer capability or determine
mobile behavior from the current interaction, while preserving mobile behavior
for genuinely primary touch interactions and ensuring mouse gestures set
state.isInteractingWithTool.
In `@packages/tools/src/tools/segmentation/BrushTool.ts`:
- Around line 464-470: Update BrushTool._clearCursor to trigger annotation
rendering for every viewport recorded by _refreshCursor in viewportIdsToRender,
rather than only the source enabledElement viewport. Clear or preserve
_hoverData as needed so the stored viewport list remains available until all
affected viewports are scheduled for rerender, preventing stale cursors in
linked viewports.
In `@packages/tools/src/tools/TrackballRotateTool.ts`:
- Around line 73-90: Complete touch termination cleanup in TrackballRotateTool
and VolumeCroppingTool: update TrackballRotateTool’s cleanup registration and
removal to include touchcancel alongside touchend and mouseup, and add touchend
and touchcancel cleanup in VolumeCroppingTool before enabling touch fallback.
In `@utils/ExampleRunner/template-config.js`:
- Around line 119-129: Update the CS3D_ALLOW_LAN condition in the template
configuration so LAN exposure is enabled only when process.env.CS3D_ALLOW_LAN
explicitly equals the string '1'. Preserve the existing restricted allowedHosts
configuration for all other values, including '0' and 'false'.
---
Outside diff comments:
In `@packages/tools/src/tools/annotation/WholeBodySegmentTool.ts`:
- Around line 129-135: Update the flow around runGrowCut in WholeBodySegmentTool
so cleanup always executes when it resolves or rejects: place growCutData reset,
resetElementCursor(element), and
triggerAnnotationRenderForViewportUIDs([viewport.id]) in a finally block.
Preserve the existing grow-cut error propagation while ensuring state, cursor,
and render cleanup are performed on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7044fb68-29bd-464b-878f-0d090f99224c
📒 Files selected for processing (37)
packages/core/src/RenderingEngine/BaseRenderingEngine.tspackages/core/src/RenderingEngine/ContextPoolRenderingEngine.tspackages/core/src/RenderingEngine/TiledRenderingEngine.tspackages/core/src/RenderingEngine/helpers/elementTouchAction.tspackages/core/src/utilities/getCurrentVolumeViewportSlice.tspackages/tools/examples/touchAllTools/index.tspackages/tools/src/eventDispatchers/shared/getTouchCallbackWithMouseFallback.tspackages/tools/src/eventDispatchers/touchEventHandlers/touchDrag.tspackages/tools/src/eventDispatchers/touchEventHandlers/touchStart.tspackages/tools/src/eventDispatchers/touchEventHandlers/touchStartActivate.tspackages/tools/src/eventDispatchers/touchEventHandlers/touchTap.tspackages/tools/src/eventDispatchers/touchToolEventDispatcher.tspackages/tools/src/eventListeners/touch/touchStartListener.tspackages/tools/src/store/ToolGroupManager/ToolGroup.tspackages/tools/src/tools/AdvancedMagnifyTool.tspackages/tools/src/tools/CrosshairsTool.tspackages/tools/src/tools/MagnifyTool.tspackages/tools/src/tools/TrackballRotateTool.tspackages/tools/src/tools/VolumeCroppingTool.tspackages/tools/src/tools/VolumeRotateTool.tspackages/tools/src/tools/annotation/ArrowAnnotateTool.tspackages/tools/src/tools/annotation/ClickSegmentTool.tspackages/tools/src/tools/annotation/DragProbeTool.tspackages/tools/src/tools/annotation/LivewireContourTool.tspackages/tools/src/tools/annotation/ProbeTool.tspackages/tools/src/tools/annotation/RegionSegmentTool.tspackages/tools/src/tools/annotation/SplineROITool.tspackages/tools/src/tools/annotation/WholeBodySegmentTool.tspackages/tools/src/tools/segmentation/BrushTool.tspackages/tools/src/tools/segmentation/CircleScissorsTool.tspackages/tools/src/tools/segmentation/RectangleROIStartEndThresholdTool.tspackages/tools/src/tools/segmentation/RectangleScissorsTool.tspackages/tools/src/tools/segmentation/SegmentLabelTool.tspackages/tools/src/tools/segmentation/SegmentSelectTool.tspackages/tools/src/tools/segmentation/SphereScissorsTool.tspackages/tools/src/utilities/touch/index.tsutils/ExampleRunner/template-config.js
Crosshairs skips the global interaction lock per touch gesture instead of per device capability so mouse drags on hybrid hardware keep the lock; brush cursor clearing rerenders all linked viewports; TrackballRotate and VolumeCropping clean up on touchend/touchcancel; touchAllTools example description matches the two-viewport roster and drops vestigial Crosshairs wiring; example runner LAN flag requires CS3D_ALLOW_LAN=1 exactly.
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
1 participant
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.
Context
An audit of all viewport tools across the clinical modes found that touch failures were architectural, not per-tool: tools entering through
preMouseDownCallback/mouseClickCallbackwere unreachable by touch because the touch pipeline never calls those handlers, hover-dependent affordances have no touch equivalent, and in-draw precision targets stayed at mouse sizes. This PR addresses the tool-framework side of that audit in one pass.Foundation: dispatcher fallback
eventDispatchers/shared/getTouchCallbackWithMouseFallback.ts: touch dispatchers resolve the explicit touch callback when a tool implements it, otherwise fall back to the mouse counterpart (touchDragCallback -> mouseDragCallback,preTouchStartCallback -> preMouseDownCallback,postTouchStartCallback -> postMouseDownCallback,touchTapCallback -> mouseClickCallback).'Touch'insupportedInteractionTypes- the first runtime consumer of that field. Explicit touch callbacks keep working regardless of declaration.mouseButton,startPoints/lastPoints/deltaPointson tap details).touchTap.tsis now a dedicated handler (first-Active-tool-wins semantics preserved);touchStartActivategained the same try/catch as its mouse twin; theTOUCH_TAPlistener leak intouchToolEventDispatcher.disableis fixed.touchStartListener: a pendingTOUCH_TAPtimer no longer fires into a new in-progress gesture (it used to end brush/scissors strokes mid-drag).['Mouse'] -> ['Mouse','Touch']; hover-only SegmentSelect/SegmentLabel narrowed to['Mouse']; VolumeCropping and RectangleROIStartEndThreshold gained declarations.touchendcleanup beside itsmouseup-only sample-distance restore.Engine
touch-action: noneon every enabled viewport element (restored on disable) viaRenderingEngine/helpers/elementTouchAction.ts, so browsers no longer consume viewport gestures (scroll, pinch zoom, double-tap zoom). Apps no longer need per-app workarounds.Per-tool touch work
touchCloseCurveDistance); Livewire's live path preview tracks the finger during a drag.touchCanvasOffset, tunable,{x:0,y:0}disables).rotateDragDegreesPerPixel); wheel behavior unchanged.isPointNearToolhonors the dispatcher-provided proximity (36px on touch instead of the hardcoded 6); themobileconfig block defaults on for coarse-pointer platforms (same(any-pointer:coarse)check the examples use;configuration.mobile.enabled = falseopts out).Fixes found during device testing
ToolGroup.clone()crashed on tools added to a group without ever receiving a mode (undefinedtoolOptionsread) - hit by AdvancedMagnify's tool-group cloning outside OHIF-style setups.getCurrentVolumeViewportSliceerror message named Livewire but the utility is shared with WindowLevelRegion; generalized.Example
touchAllToolsexample: sagittal volume + stack viewport, every touch-capable tool selectable onto Primary (1-finger), brush size/segment index controls, an on-page log console for device debugging, and mobile-safe init (isMobile, capability-gatedpreferSizeOverAccuracy). The example runner gained an env-gated LAN mode (CS3D_ALLOW_LAN=1) for phone testing.Behavior notes for reviewers
mouseClickCallbackthat declares'Touch'(e.g. MIPJumpToClick) now receives taps; in groups where such a tool precedes another tap-consumer in add order, tap delivery can change.['Mouse'](they are hover-only; consumers can still pass their ownsupportedInteractionTypes).Testing
tsc --noEmitclean on core and tools; oxlint clean (no new warnings).Summary by CodeRabbit