feat: focus 3D viewer on point under cursor#12424
Draft
kijai wants to merge 3 commits into
Draft
Conversation
dosubot
Bot
added
the
size:L
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds external render callback and animated/immediate target APIs to ControlsManager, a focus-under-cursor module that raycasts on F-key press, and Load3d integration wiring plus tests for both animation and focus behavior. ChangesFocus animation integration
Sequence DiagramsequenceDiagram
participant User
participant Load3d
participant FocusHandler
participant Raycaster
participant ControlsManager
participant OrbitControls
participant Renderer
User->>FocusHandler: F key + pointer over object
FocusHandler->>FocusHandler: pointer to NDC (canvas bounds + CSS transform)
FocusHandler->>Raycaster: raycast(ndc, camera, scene)
Raycaster->>FocusHandler: hit mesh + point
FocusHandler->>FocusHandler: distance = boundingSphere.radius × factor / FOV
FocusHandler->>Load3d: focusOn(point, distance)
Load3d->>ControlsManager: animateTarget(point, distance, 450ms)
loop animation frames
ControlsManager->>ControlsManager: t = elapsed / 450
ControlsManager->>OrbitControls: target ← lerp(start, end, t)
ControlsManager->>OrbitControls: position ← lerp(start, end, t)
ControlsManager->>Load3d: requestRender?.()
Load3d->>Renderer: forceRender()
end
ControlsManager->>Load3d: cameraChanged event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 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 |
🎨 Storybook:
|
🎭 Playwright: ❌ 1633 passed, 1 failed · 3 flaky❌ Failed Tests📊 Browser Reports
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/extensions/core/load3d/ControlsManager.ts`:
- Around line 52-57: When durationMs <= 0 the code updates this.controls.target,
this.camera.position and emits 'cameraChanged' but doesn't trigger a render;
update the immediate-path branch in ControlsManager (the block handling
durationMs <= 0) to request a frame after emitting cameraChanged by calling the
viewer render trigger—e.g. add this.eventManager.emitEvent('requestRender') (or,
if the codebase uses a different method to schedule a frame, call that like
this.viewer.requestRender()) right after
this.eventManager.emitEvent('cameraChanged', this.buildCameraState()) so the new
camera state is actually rendered.
In `@src/extensions/core/load3d/load3dFocusUnderCursor.ts`:
- Around line 80-137: Add unit tests for attachFocusUnderCursor covering: (1)
key gating — simulate keydown with 'f' and with modifier keys and with
deps.isDisabled returning true to assert focusOn is only called when allowed;
(2) pointer-to-NDC mapping — mock deps.canvas size and deps.getRenderRegion to
test getNDCFromPointer math (including letterboxed regions) by setting
pointerOnCanvas and triggering the keydown so raycaster.setFromCamera receives
the expected ndc; (3) raycast and focus behavior — stub deps.getModel,
deps.getCamera, and raycaster.intersectObject (or simulate hits) to confirm
deps.focusOn is called with hit.point and computeFocusDistance result; and (4)
cleanup — call the returned disposer and assert event listeners are removed (or
that AbortController.signal.aborted is true) so the
pointermove/pointerleave/keydown handlers stop firing; use the exported symbols
attachFocusUnderCursor, FocusUnderCursorDeps, getNDCFromPointer,
computeFocusDistance when wiring the tests.
- Around line 58-77: The NDC conversion at the end uses region.width and
region.height without checking for zero, which yields invalid coordinates for
raycasting; in the function that computes the focus under cursor (using canvas,
pointer, region and returning new THREE.Vector2), add a guard after the existing
bounds check to return null if region.width === 0 or region.height === 0 (or
otherwise handle zero-sized render regions) so you never perform the
normalization when dimensions are zero.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd9e1a26-9015-45a4-80bb-21973fa8d731
📒 Files selected for processing (5)
src/extensions/core/load3d/ControlsManager.test.tssrc/extensions/core/load3d/ControlsManager.tssrc/extensions/core/load3d/Load3d.tssrc/extensions/core/load3d/interfaces.tssrc/extensions/core/load3d/load3dFocusUnderCursor.ts
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #12424 +/- ##
===========================================
- Coverage 74.58% 60.16% -14.42%
===========================================
Files 1529 1416 -113
Lines 92750 72581 -20169
Branches 25381 20165 -5216
===========================================
- Hits 69176 43671 -25505
- Misses 22725 28437 +5712
+ Partials 849 473 -376
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1028 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Collaborator
|
I will test it |
Collaborator
|
as discussed on slack, I drafted proposal for node level keybinding https://www.notion.so/comfy-org/Node-Level-Keybindings-Design-Proposal-3696d73d365080ffa536eb315410abdb |
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.
This is personally really important QoL feature for me after years of Blender use, extremely helpful when inspecting generated meshes etc. The code is generated by Claude, but I've done multiple iterations of reviews and also tested this quite a bit in practice. Future features could include configurable hotkey and snap distance.
Summary
Adds Blender-style "focus under cursor" — pressing F while hovering a 3D viewer raycasts to the surface under the cursor, then smoothly dollies the camera so the new pivot is the hit point and the camera lands at a tight close-up distance derived from the hit object's bounding sphere.
Changes
attachFocusUnderCursorhelper insrc/extensions/core/load3d/load3dFocusUnderCursor.ts(window keydown gated by pointer-over-canvas, NDC-from-pointer math that accounts for graph zoom,THREE.Raycasteragainst the loaded model). NewControlsManager.animateTarget(point, distance, durationMs)that tweens bothcamera.positionandcontrols.targetwith easeOutCubic and cancels on user interaction;setTargetcollapses toanimateTarget(...0)for snap.setRequestRendersetter wiresLoad3d.forceRenderinto the tween.ControlsManagerInterfaceupdated to reflect the new public methods.Review Focus
getNDCFromPointercorrects for ancestor CSS transforms (rect.width !== clientWidth). Without this, F lands off-target when the LiteGraph canvas is zoomed in/out. Worth a careful read.pointerOnCanvasclosure ensures only the hovered viewer responds. N instances on a graph = N listeners, each bails early except the hovered one.cancelAnimationremoves thestartlistener and aborts the RAF chain so we don't fight user input.cancelAnimationnulls the cleanup ref after invocation so it can't double-fire.computeFocusDistancederives the landing distance from the hit object's bounding sphere (FOV-corrected), not from the current camera distance. Repeated F presses on the same area settle, not compound.Load3d.remove():disposeFocusUnderCursorandcontrolsManager.dispose()both run beforerenderer.dispose(), so the in-flight RAF can't touch a torn-down WebGL context.OrbitControlsmock withremoveEventListener;animateTargettest stubsperformance.now+requestAnimationFrameand restores viaafterEach.Screenshots
comfy_3d_focus.mp4