3d annotation #6424
3d annotation #6424
Conversation
WalkthroughAdds a full 3D annotation suite, multi-panel viewer, bounding-box and plane utilities, many new annotation UI components/hooks/state, mesh cloning safeguards for multi-panel rendering, refined Fo3d canvas/context controls, utilities/tests, and a small Modal keyboard-handling tweak for Escape events. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant TB as AnnotationToolbar
participant S as State (Recoil/Jotai)
participant C as Fo3dSceneContent / Canvas
participant SP as SegmentPolylineRenderer
participant PM as PolylinePointMarker
participant AP as AnnotationPlane
U->>TB: enable annotate mode
TB->>S: set modalMode / transformMode
S->>C: render annotation controls (plane, polylines, toolbar)
U->>C: pointer events (click / move)
C->>SP: raycast -> compute/add temp vertex
SP->>S: update segmentPolylineState / tempPolylines
S->>C: render temp polyline & markers
U->>PM: drag marker
PM->>S: update polylinePointTransforms
S->>C: applyTransformsToPolyline -> update visuals
U->>AP: manipulate TransformControls
AP->>S: set isCurrentlyTransforming true
AP->>S: onTransformEnd -> set isCurrentlyTransforming false
Note right of S: Escape (keydown, ignore repeats) triggers async is3dVisible check before cancelling segmentation/transform
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
sashankaryal
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1)
246-276: IncludeisSegmentingPointerDownwhen updating camera controls
updateCameraControlsConfig()only re-runs whenisCurrentlyTransformingchanges (Line 249 effect). When we press the segmentation pointer without entering a transform,isSegmentingPointerDownflips totruebut the effect never fires, so camera controls stay enabled while segmenting. This lets users rotate/pan the scene mid-segmentation, breaking the intended lock.Add
isSegmentingPointerDownto the effect dependencies (and optionally call the updater immediately) so controls disable as soon as segmentation starts and re-enable when it ends.- useEffect(() => { - updateCameraControlsConfig(); - }, [isCurrentlyTransforming]); + useEffect(() => { + updateCameraControlsConfig(); + }, [isCurrentlyTransforming, isSegmentingPointerDown]);app/packages/looker-3d/src/utils.test.ts (1)
96-106: Remove stray console.log in testsAvoid noisy output in CI.
- console.log(result);
🧹 Nitpick comments (12)
app/packages/looker-3d/src/StatusBar.tsx (1)
250-281: Consider extracting StatRowItem to avoid recreation.
StatRowItemis currently defined insidePerfStats, causing it to be recreated on every render (every 500ms). While the performance impact is minimal in this context, extracting it as a separate component outsidePerfStatswould follow React best practices.Apply this refactor to extract
StatRowItem:+const StatRowItem = ({ + icon, + label, + value, + barKey, + statBarColors, + statMax, +}: { + icon: JSX.Element; + label: string; + value: number; + barKey: keyof typeof statBarColors; + statBarColors: Record<string, string>; + statMax: Record<string, number>; +}) => ( + <div style={{ width: "100%" }}> + <StatRow> + <StatLabel> + {icon} + {label} + </StatLabel> + <StatValue>{value.toLocaleString()}</StatValue> + </StatRow> + <StatBarTrack> + <div + style={{ + width: `${Math.min(100, (value / statMax[barKey]) * 100)}%`, + height: "100%", + background: statBarColors[barKey], + borderRadius: 3, + transition: "width 0.4s cubic-bezier(.4,2,.6,1)", + }} + /> + </StatBarTrack> + </div> +); + const PerfStats = () => { // ... existing code ... - const StatRowItem = ({ - icon, - label, - value, - barKey, - }: { - icon: JSX.Element; - label: string; - value: number; - barKey: keyof typeof statBarColors; - }) => ( - // ... existing implementation ... - );app/packages/looker-3d/src/types.ts (3)
49-57: Suggest more specific types for event handlers.The
onClickevent parameter andtooltipproperty useanytypes. Consider using more specific types for better type safety:export interface BaseOverlayProps { opacity: number; rotation: THREE.Vector3Tuple; selected: boolean; - onClick: (e: any) => void; - tooltip: any; + onClick: (e: React.MouseEvent | THREE.Event) => void; + tooltip: React.ReactNode; label: OverlayLabel; color: string; }
59-65: Consider typing the transform controls ref more specifically.The
transformControlsRefusesanytype. If the ref points to a specific TransformControls instance, consider typing it accordingly:export interface TransformProps extends TransformControlsProps { isSelectedForTransform?: boolean; onTransformStart?: () => void; onTransformEnd?: () => void; onTransformChange?: () => void; - transformControlsRef?: React.RefObject<any>; + transformControlsRef?: React.RefObject<THREE.TransformControls>; }
72-76: Consider more specific typing for event handlers.The
restEventHandlersusesanytype. Consider constraining it to React event handler types:export interface EventHandlers { onPointerOver: () => void; onPointerOut: () => void; - restEventHandlers: Record<string, any>; + restEventHandlers: Record<string, React.EventHandler<any>>; }app/packages/looker-3d/src/annotation/AnnotationPlane.tsx (3)
121-131: Axis selection uses float equality; prefer index-of-max with toleranceComparing components to max via strict equality is brittle on floats and ties. Compute the index of the max component and set flags accordingly.
Example:
- const isXUp = Math.abs(upVector.x) === maxComponent; - const isYUp = Math.abs(upVector.y) === maxComponent; - const isZUp = Math.abs(upVector.z) === maxComponent; + const comps = [upAbs.x, upAbs.y, upAbs.z] as const; + const idx = comps.indexOf(maxComponent); + const isXUp = idx === 0; + const isYUp = idx === 1; + const isZUp = idx === 2;
92-99: Narrow useMemo deps to reduce recomputesDepending on the whole annotationPlane object causes unnecessary recalcs/renders.
- const position = useMemo( - () => new THREE.Vector3(...annotationPlane.position), - [annotationPlane] - ); - const quaternion = useMemo( - () => new THREE.Quaternion(...annotationPlane.quaternion), - [annotationPlane] - ); + const position = useMemo( + () => new THREE.Vector3(...annotationPlane.position), + [annotationPlane.position] + ); + const quaternion = useMemo( + () => new THREE.Quaternion(...annotationPlane.quaternion), + [annotationPlane.quaternion] + );
167-171: Stop pointer propagation on mouse down/up to avoid unintended camera dragsClicks on the plane can bubble to scene controls during quick drags.
-const handleMouseDown = useCallback((event: any) => { +const handleMouseDown = useCallback((event: any) => { + event.stopPropagation?.(); setIsMouseDown(true); setDragStartPosition({ x: event.clientX, y: event.clientY }); }, []); -const handleMouseUp = useCallback(() => { +const handleMouseUp = useCallback((event?: any) => { + event?.stopPropagation?.(); setIsMouseDown(false); setDragStartPosition(null); setTimeout(() => setIsDragging(false), 0); }, []);Also applies to: 187-193
app/packages/looker-3d/src/labels/shared/TransformControls.tsx (1)
77-91: On Escape, also detach/disable controls to cancel interactionOnly toggling the transforming flag can leave controls attached.
const handleKeyDown = (event: KeyboardEvent) => { if (event.key === "Escape" && isCurrentlyTransforming) { setIsCurrentlyTransforming(false); + try { + (transformControlsRef as any)?.current?.detach?.(); + } catch {} event.stopImmediatePropagation(); event.preventDefault(); } };app/packages/looker-3d/src/annotation/annotation-toolbar/CoordinateInputs.tsx (1)
85-135: Use functional state inside the setter to avoid stale-axis overwrites.When multiple controls update the annotation plane (multi-panel, transform HUD, etc.), copying
annotationPlane.position/quaternionfrom the render closure can drop updates that landed after this component rendered. Build the new tuple fromprevinside the setter instead so parallel edits merge correctly.- const newPosition: [number, number, number] = [ - ...annotationPlane.position, - ]; - if (axis === "x") newPosition[0] = numValue; - else if (axis === "y") newPosition[1] = numValue; - else if (axis === "z") newPosition[2] = numValue; - - setAnnotationPlane((prev) => ({ - ...prev, - position: newPosition, - })); - }, - [annotationPlane.position] + setAnnotationPlane((prev) => { + const newPosition = [...prev.position] as [number, number, number]; + if (axis === "x") newPosition[0] = numValue; + else if (axis === "y") newPosition[1] = numValue; + else if (axis === "z") newPosition[2] = numValue; + + return { + ...prev, + position: newPosition, + }; + }); + }, + [setAnnotationPlane] ); const handleQuaternionChange = useCallback( @@ - const newQuaternion: [number, number, number, number] = [ - ...annotationPlane.quaternion, - ]; - if (axis === "x") newQuaternion[0] = numValue; - else if (axis === "y") newQuaternion[1] = numValue; - else if (axis === "z") newQuaternion[2] = numValue; - else if (axis === "w") newQuaternion[3] = numValue; - - setAnnotationPlane((prev) => ({ - ...prev, - quaternion: newQuaternion, - })); - }, - [annotationPlane.quaternion] + setAnnotationPlane((prev) => { + const newQuaternion = [...prev.quaternion] as [ + number, + number, + number, + number + ]; + if (axis === "x") newQuaternion[0] = numValue; + else if (axis === "y") newQuaternion[1] = numValue; + else if (axis === "z") newQuaternion[2] = numValue; + else if (axis === "w") newQuaternion[3] = numValue; + + return { + ...prev, + quaternion: newQuaternion, + }; + }); + }, + [setAnnotationPlane] );app/packages/looker-3d/src/annotation/utils/polyline-utils.ts (2)
151-152: Consider returningnullinstead of[0, 0, 0]for invalid indices.Returning
[0, 0, 0]for invalid indices could mask bugs by placing points at the origin rather than failing explicitly. Consider returningnullto make invalid access more visible.Apply this diff if you prefer explicit null returns:
export function getCurrentVertexPosition( segmentIndex: number, pointIndex: number, originalPoints: Vector3Tuple[][], transforms: PolylinePointTransform[] -): [number, number, number] { +): [number, number, number] | null { // Look for existing transform const transform = transforms.find( (t) => t.segmentIndex === segmentIndex && t.pointIndex === pointIndex ); if (transform) { return transform.position; } // Fall back to original position if ( segmentIndex < originalPoints.length && pointIndex < originalPoints[segmentIndex].length ) { return originalPoints[segmentIndex][pointIndex]; } // Return default position for invalid indices - return [0, 0, 0]; + return null; }Note: This change would require updating call sites to handle
nullreturns.
215-219: Simplify type assertion with explicit tuple type.The nested type assertion is awkward and could be simplified by typing the return value of the helper function.
Apply this diff to improve readability:
// Helper to get current effective point for (segmentIndex, pointIndex) const getCurrent = ( segmentIndex: number, pointIndex: number - ): [number, number, number] => { + ): Vector3Tuple => { const t = currentTransforms.find( (x) => x.segmentIndex === segmentIndex && x.pointIndex === pointIndex ); - return (t?.position ?? originalPoints[segmentIndex][pointIndex]) as [ - number, - number, - number - ]; + return t?.position ?? originalPoints[segmentIndex][pointIndex]; };app/packages/looker-3d/src/state.ts (1)
132-140: Consider persisting transform mode and space preferences.The
annotationToolbarPositionAtom(lines 132-140) is persisted, buttransformModeAtomandtransformSpaceAtom(lines 244-252) are not. Users might prefer their last-used transform mode/space to persist across sessions.If persistence is desired, apply this diff to both atoms:
export const transformModeAtom = atom<TransformMode>({ key: "fo3d-transformMode", default: "translate", + effects: [getBrowserStorageEffectForKey("fo3d-transformMode")], }); export const transformSpaceAtom = atom<TransformSpace>({ key: "fo3d-transformSpace", default: "world", + effects: [getBrowserStorageEffectForKey("fo3d-transformSpace")], });Also applies to: 244-252
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/packages/looker-3d/package.jsonis excluded by!**/*.jsonapp/packages/looker-3d/src/assets/icons/magnet.svgis excluded by!**/*.svg,!**/*.svgapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (41)
app/packages/core/src/components/Modal/Modal.tsx(2 hunks)app/packages/looker-3d/src/Looker3d.tsx(2 hunks)app/packages/looker-3d/src/StatusBar.tsx(13 hunks)app/packages/looker-3d/src/annotation/AnnotationPlane.tsx(1 hunks)app/packages/looker-3d/src/annotation/Crosshair3D.tsx(1 hunks)app/packages/looker-3d/src/annotation/MultiPanelView.tsx(1 hunks)app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx(1 hunks)app/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsx(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/AnnotationToolbar.tsx(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/CoordinateInputs.tsx(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/index.ts(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsx(1 hunks)app/packages/looker-3d/src/annotation/types.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-delete.test.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-delete.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.test.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.ts(1 hunks)app/packages/looker-3d/src/constants.ts(4 hunks)app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx(1 hunks)app/packages/looker-3d/src/fo3d/Gizmos.tsx(3 hunks)app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(15 hunks)app/packages/looker-3d/src/fo3d/context.tsx(2 hunks)app/packages/looker-3d/src/fo3d/mesh/Ply.tsx(1 hunks)app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx(1 hunks)app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx(1 hunks)app/packages/looker-3d/src/hooks/use-3d-label-color.ts(2 hunks)app/packages/looker-3d/src/hooks/use-bounds.ts(6 hunks)app/packages/looker-3d/src/hooks/use-empty-canvas-interaction.ts(1 hunks)app/packages/looker-3d/src/labels/cuboid.tsx(6 hunks)app/packages/looker-3d/src/labels/index.tsx(6 hunks)app/packages/looker-3d/src/labels/line.tsx(0 hunks)app/packages/looker-3d/src/labels/polyline.tsx(3 hunks)app/packages/looker-3d/src/labels/shared.tsx(1 hunks)app/packages/looker-3d/src/labels/shared/TransformControls.tsx(1 hunks)app/packages/looker-3d/src/labels/shared/hooks.ts(1 hunks)app/packages/looker-3d/src/labels/shared/index.ts(1 hunks)app/packages/looker-3d/src/state.ts(4 hunks)app/packages/looker-3d/src/types.ts(3 hunks)app/packages/looker-3d/src/utils.test.ts(2 hunks)app/packages/looker-3d/src/utils.ts(6 hunks)app/packages/looker-3d/src/vite-env.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker-3d/src/labels/line.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/labels/shared/index.tsapp/packages/core/src/components/Modal/Modal.tsxapp/packages/looker-3d/src/annotation/annotation-toolbar/index.tsapp/packages/looker-3d/src/Looker3d.tsxapp/packages/looker-3d/src/annotation/utils/polyline-delete.test.tsapp/packages/looker-3d/src/hooks/use-empty-canvas-interaction.tsapp/packages/looker-3d/src/fo3d/Gizmos.tsxapp/packages/looker-3d/src/annotation/annotation-toolbar/CoordinateInputs.tsxapp/packages/looker-3d/src/fo3d/context.tsxapp/packages/looker-3d/src/utils.tsapp/packages/looker-3d/src/annotation/utils/polyline-utils.test.tsapp/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsxapp/packages/looker-3d/src/labels/index.tsxapp/packages/looker-3d/src/labels/cuboid.tsxapp/packages/looker-3d/src/annotation/AnnotationPlane.tsxapp/packages/looker-3d/src/hooks/use-3d-label-color.tsapp/packages/looker-3d/src/annotation/MultiPanelView.tsxapp/packages/looker-3d/src/labels/polyline.tsxapp/packages/looker-3d/src/annotation/utils/polyline-delete.tsapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsxapp/packages/looker-3d/src/labels/shared/hooks.tsapp/packages/looker-3d/src/labels/shared.tsxapp/packages/looker-3d/src/StatusBar.tsxapp/packages/looker-3d/src/state.tsapp/packages/looker-3d/src/fo3d/mesh/Ply.tsxapp/packages/looker-3d/src/annotation/utils/polyline-utils.tsapp/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsxapp/packages/looker-3d/src/fo3d/Fo3dCanvas.tsxapp/packages/looker-3d/src/annotation/annotation-toolbar/AnnotationToolbar.tsxapp/packages/looker-3d/src/hooks/use-bounds.tsapp/packages/looker-3d/src/annotation/Crosshair3D.tsxapp/packages/looker-3d/src/annotation/PolylinePointMarker.tsxapp/packages/looker-3d/src/types.tsapp/packages/looker-3d/src/vite-env.d.tsapp/packages/looker-3d/src/labels/shared/TransformControls.tsxapp/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsxapp/packages/looker-3d/src/annotation/types.tsapp/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsxapp/packages/looker-3d/src/constants.tsapp/packages/looker-3d/src/utils.test.ts
🧠 Learnings (1)
📚 Learning: 2024-07-27T04:14:08.421Z
Learnt from: sashankaryal
PR: voxel51/fiftyone#4310
File: app/packages/looker-3d/src/fo3d/Gizmos.tsx:110-0
Timestamp: 2024-07-27T04:14:08.421Z
Learning: The grid rendering logic in `Gizmos.tsx` is part of an external component and not directly modifiable within the project's codebase.
Applied to files:
app/packages/looker-3d/src/fo3d/Gizmos.tsx
🔇 Additional comments (17)
app/packages/looker-3d/src/StatusBar.tsx (4)
1-31: LGTM! Imports and type updates are well-structured.The imports are properly organized, and the addition of
useTheme,InfoOutlinedIcon,styled-components, andOrthographicCameratype align with the new UI requirements. ThesegmentPolylineStateAtomimport supports the new annotation hint feature.
33-143: LGTM! Styled components follow best practices.The styled components are properly structured with TypeScript typing using transient props (
$prefix). The fixed positioning and z-index values are appropriate for overlay elements in a 3D viewer context.
147-188: LGTM! Type signature update is correct.The updated type signature correctly accepts both
PerspectiveCameraandOrthographicCamera, which aligns with the multi-view support mentioned in the PR objectives. The position update logic and cleanup are implemented correctly.
370-430: LGTM! StatusBar implementation is correct.The component correctly integrates theme support, segment polyline state, and conditional rendering for the performance panel and segment hint overlay. The UX guidance in the
SegmentHintoverlay is a valuable addition for users during annotation.However, verify whether internationalization (i18n) is a concern for this application. The hardcoded hint text on lines 404-406 may need to be externalized if the app supports multiple languages.
app/packages/core/src/components/Modal/Modal.tsx (1)
148-151: Verify 3D visibility state lifecycle to prevent modal close lockout.The expanded Escape key guard now includes
is3dVisible. Iffos.groupMediaIs3dVisibledoesn't properly reset when 3D content is dismissed or hidden, users may be unable to close the modal with Escape.Run the following script to verify how
groupMediaIs3dVisibleis managed:app/packages/looker-3d/src/fo3d/mesh/Ply.tsx (1)
11-11: LGTM! Import path updated consistently.The import path change reflects the component reorganization where
HoveredPointMarkerhas been moved up one directory level. This is consistent with the similar update inPcd.tsx.app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx (1)
12-12: LGTM! Import path updated consistently.The import path change aligns with the component reorganization, matching the pattern applied in
Ply.tsx.app/packages/looker-3d/src/vite-env.d.ts (1)
2-2: LGTM! SVGR TypeScript support added.The reference directive enables importing SVG files as React components with proper TypeScript definitions, supporting the new UI components introduced in this PR.
app/packages/looker-3d/src/types.ts (1)
78-82: LGTM! Well-defined archetype union type.The
TransformArchetypeunion type clearly defines the supported transform target types.app/packages/looker-3d/src/fo3d/context.tsx (1)
17-18: LGTM! Context extended for camera look-at controls.The
lookAtstate and setter follow the established pattern used for other context members likeupVector, maintaining consistency across the Fo3d context API.Also applies to: 35-36
app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx (1)
67-72: LGTM! Correctly adapted to updated hook API.The destructuring pattern correctly handles the updated
useFo3dBoundshook API, which now returns an object withboundingBoxandrecomputeBoundsproperties. The local rename topcdBoundingBoxmaintains clarity in this context.app/packages/looker-3d/src/labels/shared/index.ts (1)
1-2: LGTM! Clean barrel file for public API consolidation.The barrel file follows standard patterns for consolidating exports from the
labels/sharedmodule, making it easier for consumers to import shared hooks and transform controls.app/packages/looker-3d/src/Looker3d.tsx (1)
42-42: Verify usage of themodevariable.The
modevariable is declared but not used in the visible code sections. Similarly, the importedclearTransformStateSelectorandselectedLabelForAnnotationAtom(lines 18, 24) are not visibly used.Please verify:
- Is
modeused in code sections not shown in the diff?- Are the imported selectors/atoms used elsewhere in this component?
- If unused, consider removing them to maintain code cleanliness.
app/packages/looker-3d/src/annotation/utils/polyline-utils.ts (1)
174-179: Clarify the purpose of creating placeholder segments.When a segment doesn't exist at
segmentIndex, the function creates a placeholder segment with two[0, 0, 0]points. This behavior seems unexpected and could mask errors or create invalid geometry.Verify whether:
- Missing segments are expected in normal usage scenarios
- Creating placeholder segments is the intended behavior
- This should instead log a warning or throw an error for invalid transforms
If placeholder segments are intentional, add a comment explaining when and why segments might be missing.
app/packages/looker-3d/src/annotation/types.ts (1)
1-115: LGTM! Well-structured type definitions.The type definitions are comprehensive, well-documented, and follow TypeScript best practices. The interfaces provide clear contracts for the annotation system with appropriate use of optional fields, union types, and tuple types.
app/packages/looker-3d/src/state.ts (2)
266-272: Verify backend synchronization plan for transformed labels.The TODO comment indicates that
transformedLabelsAtomneeds to be synced with the backend. This is a critical requirement for persisting annotation edits across sessions.Confirm:
- Is there a tracking issue for implementing backend synchronization?
- Should this atom be persisted locally until backend sync is implemented?
- What happens to transformed labels when the user reloads the page?
Consider adding a GitHub issue reference in the comment if one exists.
378-397: LGTM! Well-designed state reset selector.The
clearTransformStateSelectorprovides a clean way to reset annotation state while preservingpolylinePointTransforms(as noted in the comment on line 389). The design decision to persist transforms is appropriate since they represent user edits that should survive mode changes.
app/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/packages/looker-3d/src/hooks/use-bounds.test.ts (3)
21-35: Consider verifying DEFAULT_BOUNDING_BOX structure in assertions.The test verifies that
boundingBoxbecomes defined after 500ms, but doesn't validate the actual values or structure. Consider adding assertions to verify that the returnedboundingBoxmatches the expected DEFAULT_BOUNDING_BOX values (e.g., checkingminandmaxcoordinates).Example enhancement:
- expect(result.current.boundingBox).not.toBeNull(); - expect(result.current.boundingBox).toBeDefined(); + expect(result.current.boundingBox).not.toBeNull(); + expect(result.current.boundingBox.min).toBeDefined(); + expect(result.current.boundingBox.max).toBeDefined(); + // Optionally verify specific DEFAULT_BOUNDING_BOX coordinates
34-34: Redundant assertion.The assertion on line 34 is redundant. In JavaScript/TypeScript, if a value is not
null(line 33), it's already defined. The.toBeDefined()check adds no additional verification.Apply this diff to remove the redundancy:
expect(result.current.boundingBox).not.toBeNull(); - expect(result.current.boundingBox).toBeDefined();
1-101: Consider adding tests forrecomputeBoundsand edge cases.According to the AI summary, the hook now returns
{ boundingBox, recomputeBounds }, but there are no tests for therecomputeBoundsfunction. Additionally, the summary mentions retry logic and fallbacks for infinite max values, which are not covered by the current tests.Consider adding test cases for:
- Testing the
recomputeBoundsfunction to ensure manual recomputation works correctly- Edge case handling for infinite bounding box max values
- Retry logic behavior
Would you like me to generate test cases for these scenarios?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/looker-3d/src/hooks/use-bounds.test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/hooks/use-bounds.test.ts
🔇 Additional comments (3)
app/packages/looker-3d/src/hooks/use-bounds.test.ts (3)
37-83: LGTM! Stabilization logic is well-tested.The test correctly verifies that the bounding box stabilizes after multiple computations and that the final values match the expected stable box. The mock setup with changing and stable boxes effectively tests the stabilization behavior.
85-100: LGTM! Predicate guard logic is properly tested.The test correctly verifies that the bounding box remains null when the predicate returns false, ensuring the hook respects the predicate guard.
11-11: Consider whether Vector3 mock needs additional implementation.The
Vector3mock is currently justvi.fn(), which returnsundefined. Verify that this minimal mock is sufficient for the test scenarios. If the code under test instantiatesVector3objects or calls methods on them, you may need a more complete mock implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/looker-3d/src/hooks/use-bounds.ts (1)
75-98: Retry cap halts future bounding box updatesWhen retries hit
MAX_BOUNDING_BOX_RETRIES, we return after setting the default box without scheduling another poll. If the asset becomes ready later (or the box turns finite after load), we never try again, so the hook stays on the fallback forever. Please keep polling after the fallback (e.g., by resetting the counter and scheduling the next timeout) so slower-loading assets can still produce their real bounds.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/packages/core/src/components/Modal/Modal.tsx(3 hunks)app/packages/looker-3d/src/annotation/AnnotationPlane.tsx(1 hunks)app/packages/looker-3d/src/annotation/MultiPanelView.tsx(1 hunks)app/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsx(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsx(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.test.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.ts(1 hunks)app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(15 hunks)app/packages/looker-3d/src/hooks/use-bounds.test.ts(4 hunks)app/packages/looker-3d/src/hooks/use-bounds.ts(6 hunks)app/packages/looker-3d/src/labels/polyline.tsx(3 hunks)app/packages/looker-3d/src/labels/shared/TransformControls.tsx(1 hunks)app/packages/looker-3d/src/state.ts(8 hunks)app/packages/looker-3d/src/utils.test.ts(2 hunks)app/packages/looker-3d/src/utils.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker-3d/src/utils.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/hooks/use-bounds.test.tsapp/packages/looker-3d/src/annotation/utils/polyline-utils.test.tsapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsxapp/packages/looker-3d/src/annotation/AnnotationPlane.tsxapp/packages/looker-3d/src/labels/polyline.tsxapp/packages/core/src/components/Modal/Modal.tsxapp/packages/looker-3d/src/annotation/utils/polyline-utils.tsapp/packages/looker-3d/src/hooks/use-bounds.tsapp/packages/looker-3d/src/annotation/MultiPanelView.tsxapp/packages/looker-3d/src/state.tsapp/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsxapp/packages/looker-3d/src/utils.test.tsapp/packages/looker-3d/src/labels/shared/TransformControls.tsxapp/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.test.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.ts(1 hunks)app/packages/looker-3d/src/labels/polyline.tsx(3 hunks)app/packages/looker-3d/src/state.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker-3d/src/annotation/utils/polyline-utils.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsxapp/packages/looker-3d/src/labels/polyline.tsxapp/packages/looker-3d/src/state.tsapp/packages/looker-3d/src/annotation/utils/polyline-utils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/packages/looker-3d/src/state.ts (1)
302-320: Fill coordinate selector from effective points when no transform exists.This is the same issue flagged in previous reviews: when a vertex hasn't been transformed yet,
polylinePointTransformsAtomhas no entry, so this selector returnsnull, leaving coordinate inputs blank for untransformed points. The fix is to fall back to reading frompolylineEffectivePointsAtombefore returningnull.Apply the previously suggested diff:
export const selectedPolylinePositionSelector = selector< [number, number, number] | null >({ key: "fo3d-selectedPolylinePositionSelector", get: ({ get }) => { const currentPoint = get(selectedPolylineVertexAtom); if (!currentPoint) return null; const transforms = get(polylinePointTransformsAtom)[currentPoint.labelId] || []; const transform = transforms.find( (t) => t.segmentIndex === currentPoint.segmentIndex && t.pointIndex === currentPoint.pointIndex ); - return transform?.position ?? null; + if (transform) { + return transform.position; + } + + const effectivePoints = + get(polylineEffectivePointsAtom(currentPoint.labelId)) ?? []; + return ( + effectivePoints[currentPoint.segmentIndex]?.[currentPoint.pointIndex] ?? + null + ); }, });
🧹 Nitpick comments (1)
app/packages/looker-3d/src/annotation/utils/polyline-utils.test.ts (1)
876-903: Add epsilon tolerance test forupdateDuplicateVertices.The test labeled "handles floating point precision in vertex matching" uses exact coordinate matches
[1.0, 1.0, 1.0], so it doesn't actually test floating-point precision issues. OnceupdateDuplicateVerticesis updated to use epsilon-based comparison (as suggested in the review ofpolyline-utils.ts), add a test case similar to the epsilon tests forfindSharedPointSegments(lines 201-216) that verifies vertices with small differences (e.g.,[1.0, 1.0, 1.0]vs[1.0000001, 1.0000001, 1.0000001]) are correctly recognized as duplicates.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsx(1 hunks)app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.test.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.ts(1 hunks)app/packages/looker-3d/src/labels/index.tsx(6 hunks)app/packages/looker-3d/src/labels/polyline.tsx(3 hunks)app/packages/looker-3d/src/labels/shared/hooks.ts(1 hunks)app/packages/looker-3d/src/state.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/packages/looker-3d/src/labels/shared/hooks.ts
- app/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/labels/polyline.tsxapp/packages/looker-3d/src/labels/index.tsxapp/packages/looker-3d/src/annotation/PolylinePointMarker.tsxapp/packages/looker-3d/src/annotation/utils/polyline-utils.test.tsapp/packages/looker-3d/src/state.tsapp/packages/looker-3d/src/annotation/utils/polyline-utils.tsapp/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx
🔇 Additional comments (4)
app/packages/looker-3d/src/labels/polyline.tsx (2)
54-72: Well-structured annotation integration.The
usePolylineAnnotationhook integration properly addresses the previous concern about vertex tearing by centralizing transform management and providing dedicated handlers for segment and point interactions. The hook encapsulates the complexity of managing effective points, transforms, and markers.
135-151: LGTM: Proper resource cleanup.The separation of geometry and material disposal into distinct effects is correct and prevents the resource leak issues flagged in previous reviews. Geometries are disposed when meshes change, and the shared material is disposed only when the material itself changes or on unmount.
app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx (1)
100-148: Good deduplication strategy, but ensure epsilon tolerance is used.The coordinate-based deduplication using a
visitedPointsSet (lines 100-116) properly addresses the previous concern about vertex tearing by ensuring only one draggable marker is created per unique physical vertex position. However, theonPointMovehandler (lines 127-144) callsupdateDuplicateVertices, which currently uses exact===comparison (as flagged inpolyline-utils.ts). Once that function is updated to use epsilon-based comparison, this marker deduplication will work correctly even with floating-point imprecision.Ensure that the fix to
updateDuplicateVertices(usingpositionsEqual) is applied before relying on this hook for production annotation workflows.app/packages/looker-3d/src/labels/index.tsx (1)
287-314: Verify handling of incomplete segment transforms.The code at line 307 assigns
segmentPoints[transform.pointIndex] = transform.position, which can create a sparse array if transforms are missing somepointIndexvalues (e.g., transforms for points 0 and 2 but not 1). Sparse arrays withundefinedelements could cause rendering issues or errors in downstream code that expects dense arrays of coordinates.Verify that all transform workflows guarantee complete, contiguous point indices within each segment, or add defensive logic to filter out
undefinedentries before creating the polyline overlay. Run the following script to check for any code that might create incomplete transforms:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/looker-3d/src/state.ts (1)
276-281: Track the backend synchronization TODO.The TODO comment indicates that
transformedLabelsAtomshould be synced with the backend. Consider creating a tracking issue to ensure this is addressed.Do you want me to open a new issue to track this backend synchronization task?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/looker-3d/src/state.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/state.ts
🔇 Additional comments (3)
app/packages/looker-3d/src/state.ts (3)
217-221: LGTM! Type safety issue resolved.The
hoveredLabelAtomnow correctly usesOverlayLabel | nullinstead ofany, which resolves the type safety concern from the previous review.
344-360: VerifysessionStorageis intentional for annotation plane state.The
annotationPlaneAtomusessessionStorage: true, which means the annotation plane state will not persist across browser sessions (unlike most other settings which use localStorage).Verify this is the intended behavior for the annotation plane configuration.
302-390: Verify ifselectedPolylinePositionSelectorwas removed or exists elsewhere.A previous review comment (referencing lines 306-320) and the AI summary both mention a
selectedPolylinePositionSelectorthat computes the current transformed position for a selected polyline vertex. This selector is not present in the annotated code.If this selector was removed, verify that no other components depend on it. If it exists outside the changed lines, ensure it implements the fallback to effective points as suggested in the previous review.
Run the following script to check if the selector exists in the codebase:
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
app/packages/looker-3d/src/labels/cuboid.tsx (1)
84-96: Restore transparent blending for non-opaque outlinesSetting
transparenttoopacity < 0.2disables blending for the majority of user-selected opacities (0.2–0.9), so outlines render fully opaque and ignore UI opacity controls—this regression was flagged earlier. Keep blending enabled for any non-fully-opaque outline.new LineMaterial({ opacity: opacity, - transparent: opacity < 0.2, + transparent: opacity < 1, color: strokeAndFillColor, linewidth: lineWidth, }),app/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsx (1)
104-155: Double-click finalization drops the last vertexWhen the user double-clicks to finish, the code slices off the most recent vertex (
slice(0, -1)) before emitting the temp polyline, so the saved polyline is missing the point they just placed. We previously flagged this regression; please retain the vertex and only overwrite its coordinates when needed.- // Remove the last added vertex from the previous click - setSegmentState((prev) => ({ - ...prev, - vertices: prev.vertices.slice(0, -1), - })); - - // Get the vertices without the duplicate - const verticesWithoutDuplicate = segmentState.vertices.slice(0, -1); + const verticesBeforeFinalize = [...segmentState.vertices]; if (snapCloseAutomatically) { const tempPolyline: TempPolyline = { id: `temp-polyline-${Date.now()}`, - vertices: verticesWithoutDuplicate, + vertices: verticesBeforeFinalize, isClosed: true, color, lineWidth, }; @@ } else { - // End the segment at the current position without closing + // End the segment at the current position without closing + const updatedVertices = [...verticesBeforeFinalize]; + if (updatedVertices.length > 0) { + updatedVertices[updatedVertices.length - 1] = [ + finalPos.x, + finalPos.y, + finalPos.z, + ]; + } + const tempPolyline: TempPolyline = { id: `temp-polyline-${Date.now()}`, - vertices: [ - ...verticesWithoutDuplicate, - [finalPos.x, finalPos.y, finalPos.z], - ], + vertices: updatedVertices, isClosed: false, color, lineWidth, };app/packages/looker-3d/src/annotation/utils/polyline-utils.ts (1)
277-290: Use epsilon tolerance for duplicate vertex detection.Lines 279-282 use exact
===equality to compare floating-point coordinates, which can miss vertices that should be considered identical due to floating-point precision issues. ThepositionsEqualhelper is already defined in this file but not used here.Apply this diff to use the existing
positionsEqualhelper:effectivePoints3d.forEach((segmentPoints, segIdx) => { segmentPoints.forEach((candidatePoint, ptIdx) => { - if ( - candidatePoint[0] === movedPoint[0] && - candidatePoint[1] === movedPoint[1] && - candidatePoint[2] === movedPoint[2] - ) { + if (positionsEqual(candidatePoint, movedPoint)) { duplicates.push({ segmentIndex: segIdx, pointIndex: ptIdx, }); } }); });
🧹 Nitpick comments (2)
app/packages/looker-3d/src/utils.ts (2)
222-229: Ensure both vectors are normalized forsetFromUnitVectors.Line 227 clones
upVectorNormalizedbut doesn't normalize it, despite the parameter name implying it's already normalized.setFromUnitVectorsexpects unit vectors, so it's safer to normalize both. Additionally, line 226 unnecessarily normalizestargetNormalsincenew Vector3(0, 1, 0)is already a unit vector.Apply this diff to ensure correctness:
- const from = targetNormal.clone().normalize(); - const to = upVectorNormalized.clone(); + const from = targetNormal.clone(); + const to = upVectorNormalized.clone().normalize(); return new Quaternion().setFromUnitVectors(from, to);
257-269: Replaceas anywith proper type assertion.Line 263 uses
as anyto bypass type checking when passingndctosetFromCamera. This eliminates type safety and could hide issues if the expected type changes.Apply this diff to use proper typing:
+import { Vector2 } from "three"; export function getPlaneIntersection( raycaster: Raycaster, camera: Camera, ndc: { x: number; y: number }, plane: Plane ): Vector3 | null { - raycaster.setFromCamera(ndc as any, camera); + raycaster.setFromCamera(new Vector2(ndc.x, ndc.y), camera); const point = new Vector3(); if (raycaster.ray.intersectPlane(plane, point)) { return point; } return null; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/packages/looker-3d/package.jsonis excluded by!**/*.jsonapp/packages/looker-3d/src/assets/icons/magnet.svgis excluded by!**/*.svg,!**/*.svgapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (44)
app/packages/core/src/components/Modal/Modal.tsx(3 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Annotate.tsx(2 hunks)app/packages/looker-3d/src/Looker3d.tsx(2 hunks)app/packages/looker-3d/src/StatusBar.tsx(13 hunks)app/packages/looker-3d/src/annotation/AnnotationPlane.tsx(1 hunks)app/packages/looker-3d/src/annotation/Crosshair3D.tsx(1 hunks)app/packages/looker-3d/src/annotation/MultiPanelView.tsx(1 hunks)app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx(1 hunks)app/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsx(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/AnnotationToolbar.tsx(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/CoordinateInputs.tsx(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/index.ts(1 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsx(1 hunks)app/packages/looker-3d/src/annotation/types.ts(1 hunks)app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-delete.test.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-delete.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.test.ts(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.ts(1 hunks)app/packages/looker-3d/src/constants.ts(4 hunks)app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx(1 hunks)app/packages/looker-3d/src/fo3d/Gizmos.tsx(3 hunks)app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(15 hunks)app/packages/looker-3d/src/fo3d/context.tsx(2 hunks)app/packages/looker-3d/src/fo3d/mesh/Ply.tsx(1 hunks)app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx(1 hunks)app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx(1 hunks)app/packages/looker-3d/src/hooks/use-3d-label-color.ts(2 hunks)app/packages/looker-3d/src/hooks/use-bounds.test.ts(4 hunks)app/packages/looker-3d/src/hooks/use-bounds.ts(6 hunks)app/packages/looker-3d/src/hooks/use-empty-canvas-interaction.ts(1 hunks)app/packages/looker-3d/src/labels/cuboid.tsx(6 hunks)app/packages/looker-3d/src/labels/index.tsx(6 hunks)app/packages/looker-3d/src/labels/line.tsx(0 hunks)app/packages/looker-3d/src/labels/polyline.tsx(3 hunks)app/packages/looker-3d/src/labels/shared.tsx(1 hunks)app/packages/looker-3d/src/labels/shared/TransformControls.tsx(1 hunks)app/packages/looker-3d/src/labels/shared/hooks.ts(1 hunks)app/packages/looker-3d/src/labels/shared/index.ts(1 hunks)app/packages/looker-3d/src/state.ts(8 hunks)app/packages/looker-3d/src/types.ts(3 hunks)app/packages/looker-3d/src/utils.test.ts(2 hunks)app/packages/looker-3d/src/utils.ts(6 hunks)app/packages/looker-3d/src/vite-env.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker-3d/src/labels/line.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- app/packages/core/src/components/Modal/Modal.tsx
- app/packages/looker-3d/src/annotation/utils/polyline-utils.test.ts
- app/packages/looker-3d/src/labels/shared.tsx
- app/packages/looker-3d/src/labels/shared/hooks.ts
- app/packages/looker-3d/src/fo3d/context.tsx
- app/packages/looker-3d/src/fo3d/mesh/Ply.tsx
- app/packages/looker-3d/src/annotation/annotation-toolbar/useAnnotationActions.tsx
- app/packages/looker-3d/src/vite-env.d.ts
- app/packages/looker-3d/src/annotation/types.ts
- app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx
- app/packages/looker-3d/src/annotation/Crosshair3D.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/annotation/MultiPanelView.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Annotate.tsxapp/packages/looker-3d/src/annotation/utils/polyline-delete.test.tsapp/packages/looker-3d/src/annotation/annotation-toolbar/index.tsapp/packages/looker-3d/src/hooks/use-3d-label-color.tsapp/packages/looker-3d/src/annotation/usePolylineAnnotation.tsxapp/packages/looker-3d/src/labels/index.tsxapp/packages/looker-3d/src/utils.test.tsapp/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsxapp/packages/looker-3d/src/labels/shared/index.tsapp/packages/looker-3d/src/labels/cuboid.tsxapp/packages/looker-3d/src/annotation/PolylinePointMarker.tsxapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsxapp/packages/looker-3d/src/types.tsapp/packages/looker-3d/src/hooks/use-bounds.test.tsapp/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsxapp/packages/looker-3d/src/hooks/use-bounds.tsapp/packages/looker-3d/src/utils.tsapp/packages/looker-3d/src/annotation/AnnotationPlane.tsxapp/packages/looker-3d/src/constants.tsapp/packages/looker-3d/src/Looker3d.tsxapp/packages/looker-3d/src/annotation/annotation-toolbar/AnnotationToolbar.tsxapp/packages/looker-3d/src/annotation/annotation-toolbar/CoordinateInputs.tsxapp/packages/looker-3d/src/labels/shared/TransformControls.tsxapp/packages/looker-3d/src/annotation/utils/polyline-delete.tsapp/packages/looker-3d/src/fo3d/Fo3dCanvas.tsxapp/packages/looker-3d/src/StatusBar.tsxapp/packages/looker-3d/src/labels/polyline.tsxapp/packages/looker-3d/src/hooks/use-empty-canvas-interaction.tsapp/packages/looker-3d/src/state.tsapp/packages/looker-3d/src/fo3d/Gizmos.tsxapp/packages/looker-3d/src/annotation/utils/polyline-utils.ts
🧠 Learnings (1)
📚 Learning: 2024-07-27T04:14:08.421Z
Learnt from: sashankaryal
PR: voxel51/fiftyone#4310
File: app/packages/looker-3d/src/fo3d/Gizmos.tsx:110-0
Timestamp: 2024-07-27T04:14:08.421Z
Learning: The grid rendering logic in `Gizmos.tsx` is part of an external component and not directly modifiable within the project's codebase.
Applied to files:
app/packages/looker-3d/src/fo3d/Gizmos.tsx
🔇 Additional comments (4)
app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx (1)
67-72: LGTM! Verify consistent usage ofuseFo3dBoundsacross the codebase.The destructuring change correctly adapts to the updated
useFo3dBoundsAPI. The downstream usage ofpcdBoundingBoxremains consistent and unchanged.Run the following script to verify all usages of
useFo3dBoundshave been updated consistently:app/packages/looker-3d/src/utils.ts (1)
240-246: LGTM! Well-documented utility functions.The new utility functions (
toNDC,createPlane,isButtonMatch,getPlaneFromPositionAndQuaternion) are correctly implemented with clear JSDoc documentation. The logic for NDC conversion, plane creation, button matching, and plane-from-pose construction is sound.Also applies to: 278-280, 289-291, 300-315
app/packages/looker-3d/src/annotation/utils/polyline-utils.ts (1)
1-541: Well-implemented polyline utilities with comprehensive features.The module provides robust utilities for polyline manipulation, including vertex positioning, centroid calculation, segment insertion, and transform management. The implementation correctly uses epsilon-based comparisons in most places (e.g.,
findSharedPointSegmentsat line 107,insertVertexInSegmentat line 403) and handles edge cases like zero-length segments (line 341).app/packages/looker-3d/src/state.ts (1)
137-387: LGTM! Comprehensive and well-typed annotation state management.The extensive state management additions for 3D annotation and transform workflows are well-structured:
- All atoms have proper TypeScript types (no more
anyusage)- Storage effects are correctly configured with appropriate options (
prependDatasetNameInKey,useJsonSerialization,sessionStorage)- The
clearTransformStateSelectorcorrectly resets transform-related state while preservingpolylinePointTransforms(line 379 comment is helpful)- Proper separation of concerns with dedicated atoms for hover state, selection, transforms, and segmentation
app/packages/looker-3d/src/annotation/annotation-toolbar/AnnotationToolbar.tsx
Show resolved
Hide resolved
app/packages/looker-3d/src/annotation/annotation-toolbar/CoordinateInputs.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/packages/looker-3d/src/labels/index.tsx (3)
48-51: Add JSDoc for the new globalOpacity prop.The
globalOpacityprop is a new addition to the public API but lacks documentation. Consider adding a brief JSDoc comment explaining its purpose and how it overridescolorScheme.opacity.export interface ThreeDLabelsProps { sampleMap: { [sliceOrFilename: string]: Sample } | fos.Sample[]; + /** Global opacity override for all labels. When provided, overrides colorScheme.opacity. */ globalOpacity?: number; }
121-134: Consider extracting transform mode validation logic.The logic that restricts polylines to translate mode (lines 126-131) is a business rule that may expand as more transform modes are supported. Consider extracting this to a utility function for easier testing and maintenance.
// In a utils file function getValidTransformMode( labelCls: string, currentMode: TransformMode ): TransformMode { if (labelCls === "Polyline" && (currentMode === "rotate" || currentMode === "scale")) { return "translate"; } return currentMode; } // In component const validMode = getValidTransformMode(label._cls, transformMode); if (validMode !== transformMode) { setTransformMode(validMode); }
363-376: Incomplete useMemo dependency array.The dependency array (lines 364-376) includes
transformMode(line 372) buttransformModeisn't actually used in the memoized computation. It's only used indirectly viahandleSelectwhich is defined outside theuseMemo. This could cause unnecessary re-renders.Remove
transformModefrom the dependency array sincehandleSelectis already included:}, [ rawOverlays, itemRotation, labelAlpha, overlayRotation, handleSelect, tooltip, settings, - transformMode, polylinePointTransforms, polylineWidth, currentSampleId, ]);Note: If
handleSelectchanges whentransformModechanges, the dependency is already tracked throughhandleSelectitself.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/looker-3d/src/labels/index.tsx(9 hunks)app/packages/looker-3d/src/labels/polyline.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/labels/index.tsxapp/packages/looker-3d/src/labels/polyline.tsx
🔇 Additional comments (7)
app/packages/looker-3d/src/labels/polyline.tsx (4)
103-113: LGTM! Material memoization is correct.The material is properly memoized based on its dependencies (
filled,strokeAndFillColor,opacity), and the separate cleanup effect at lines 147-153 ensures proper disposal.
115-135: Verify geometry disposal timing with rapid updates.The disposal pattern looks correct now (geometry-only disposal per past review), but consider potential race conditions: if
effectivePoints3dchanges rapidly (e.g., during continuous transform dragging), the previousmeshesRef.currentgeometries are disposed synchronously inside theuseMemo, but the old meshes may still be referenced by React's reconciliation until the next render completes. This is generally safe in Three.js since disposed geometries simply stop rendering, but it's worth confirming there are no console warnings during intensive editing.Run the following test scenario to verify there are no disposal warnings:
- Enter annotation mode and select a polyline
- Rapidly drag a transform control handle back and forth
- Check the browser console for any Three.js disposal or resource warnings
137-153: LGTM! Cleanup effects are properly separated.The two cleanup effects correctly handle:
- Mesh geometries disposal on unmount (lines 137-145)
- Material disposal on unmount or material change (lines 147-153)
This separation ensures the shared material isn't disposed prematurely while meshes are still using it.
163-190: Verify event handler order in annotation mode.The nested event handlers (lines 174-183) combine hover state, annotation handlers, and base handlers in a specific order. Confirm that:
- The order of
setIsHovered(true),handleAnnotationPointerOver(), andonPointerOver()doesn't cause state conflicts- Event propagation is correctly handled when both the inner group and the Transformable wrapper have pointer handlers
app/packages/looker-3d/src/labels/index.tsx (3)
168-188: LGTM! Escape key handler is properly scoped.The Escape key handler correctly:
- Only activates in annotate mode with a selected label
- Clears both the annotation selection and segment editing mode
- Properly prevents default and stops propagation
- Cleans up the event listener on unmount
136-166: Verify isSegmenting guard doesn't block valid interactions.The early return
if (isSegmenting) return;(line 142) prevents all label selection during segment editing. Confirm this is the intended behavior and doesn't inadvertently block:
- Switching between different polylines during segment editing
- Canceling segment mode by clicking another label
Based on learnings about the annotation workflow, verify the expected behavior:
- Should users be able to select a different polyline while editing segments of another?
- Should clicking a cuboid automatically exit segment editing mode?
If uncertain, you can search for
isSegmentingusage patterns:
294-361: Verify transform data integrity assumptions.The synthesis logic assumes:
- All transforms for a given segment have contiguous
pointIndexvalues- The
segmentIndexvalues form a contiguous sequence starting from 0- No orphaned transforms exist (transforms referencing deleted labels)
None of these assumptions are validated. If any are violated, the synthesized polylines may have gaps or incorrect topology.
Add runtime validation or document these invariants:
// Validation example const maxSegmentIndex = Math.max(...transforms.map(t => t.segmentIndex)); const expectedSegmentCount = maxSegmentIndex + 1; if (segmentsMap.size !== expectedSegmentCount) { console.warn(`Polyline ${labelId}: Missing segments in transform data`); }Alternatively, generate a verification script to audit existing transform data:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/looker-3d/src/annotation/Crosshair3D.tsx (1)
80-85: Verify scale behavior for PerspectiveCamera.The crosshair scale is computed as
1 / zoomforOrthographicCamerabut defaults to1for other camera types (e.g.,PerspectiveCamera). This means in perspective view, the crosshair will appear smaller when further from the camera and larger when closer, whereas in orthographic view it maintains a consistent screen-space size.Confirm this is the intended behavior. If you want consistent screen-space sizing in perspective view as well, consider calculating scale based on distance from camera.
Example for perspective camera (if consistent sizing is desired):
const scale = useMemo(() => { if (camera instanceof THREE.OrthographicCamera) { return 1 / camera.zoom; } + if (camera instanceof THREE.PerspectiveCamera && worldVector) { + const distance = camera.position.distanceTo(worldVector); + return distance / 10; // Adjust divisor to tune crosshair size + } return 1; }, [camera, worldVector]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/looker-3d/src/annotation/Crosshair3D.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/annotation/Crosshair3D.tsx
🔇 Additional comments (1)
app/packages/looker-3d/src/annotation/Crosshair3D.tsx (1)
52-116: LGTM! Solid implementation of 3D crosshair component.The component correctly:
- Clamps cursor position to scene bounds
- Projects 3D world coordinates to screen space
- Handles edge cases with appropriate early returns (behind camera, outside viewport)
- Uses React-Three-Fiber and Recoil patterns appropriately
- Applies theme colors for consistent UI styling
The projection math and NDC boundary checks (lines 92-104) are correct, ensuring the crosshair only renders when visible in the viewport.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/packages/looker-3d/src/StatusBar.tsx (5)
124-129: Prefer standard flex alignment value.Line 127 uses
justify-content: right, which is non-standard CSS. Useflex-endfor better browser compatibility and standards compliance.Apply this diff:
const CloseBar = styled.div<{ $bg: string }>` display: flex; width: 100%; - justify-content: right; + justify-content: flex-end; background-color: ${(p) => p.$bg}; `;
202-220: Type assertion bypasses safety.Line 204 uses
as anyto work around missing types from r3f-perf. While functional, this silences TypeScript's type checking.Consider:
- Import proper types from r3f-perf if available
- Define a minimal interface for the expected shape:
interface PerfState { log?: { fps?: number }; gl?: { info?: { render?: { calls?: number; triangles?: number; points?: number }; memory?: { geometries?: number; textures?: number }; programs?: { length?: number }; }; }; } const perfState = getPerf() as PerfState;
251-282: Consider defining StatRowItem outside PerfStats.Defining
StatRowIteminsidePerfStatsrecreates the component on every render. While React is optimized for this pattern, extracting it to module scope or memoizing it improves clarity.Move the component definition outside:
const StatRowItem = ({ icon, label, value, barKey, }: { icon: JSX.Element; label: string; value: number; barKey: 'calls' | 'triangles' | 'points' | 'geometries' | 'textures' | 'programs'; }) => { // ... (existing implementation) }; const PerfStats = () => { // ... rest of PerfStats };
399-409: Consider using theme for text color.Line 400 hardcodes the text color as
"#e0e0e0"instead of using a theme color. For consistency with the theming system, consider usingtheme.text.secondaryor similar.- <SegmentHint $border={theme.primary.main} $text={"#e0e0e0"}> + <SegmentHint $border={theme.primary.main} $text={theme.text.secondary}>
411-428: Consider using theme for background colors.Lines 415 and 420 use hardcoded rgba/hsla strings for background colors. For better maintainability and theme consistency, these could come from the theme object.
Example approach:
- <CloseBar $bg={`rgba(255, 109, 5, 0.06)`}> + <CloseBar $bg={theme.background.level2}> <IconButton onClick={onClickHandler}> <Close /> </IconButton> </CloseBar> - <PerfPanel $bg={`hsla(208.46, 87%, 53%, 0.20)`}> + <PerfPanel $bg={theme.background.level3}> <CameraInfo cameraRef={cameraRef} />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/looker-3d/src/StatusBar.tsx(13 hunks)app/packages/looker-3d/src/annotation/annotation-toolbar/CoordinateInputs.tsx(1 hunks)app/packages/looker-3d/src/annotation/utils/polyline-utils.ts(1 hunks)app/packages/looker-3d/src/types.ts(3 hunks)app/packages/looker-3d/src/utils.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker-3d/src/utils.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/types.tsapp/packages/looker-3d/src/annotation/annotation-toolbar/CoordinateInputs.tsxapp/packages/looker-3d/src/StatusBar.tsxapp/packages/looker-3d/src/annotation/utils/polyline-utils.ts
🧠 Learnings (1)
📚 Learning: 2024-10-22T16:25:08.017Z
Learnt from: minhtuev
PR: voxel51/fiftyone#4957
File: app/packages/components/src/components/Toast/Toast.tsx:29-36
Timestamp: 2024-10-22T16:25:08.017Z
Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `mui/material`.
Applied to files:
app/packages/looker-3d/src/types.ts
🔇 Additional comments (3)
app/packages/looker-3d/src/StatusBar.tsx (3)
1-1: LGTM! Clean import additions.The new imports support theme integration, styled components, camera type union, and segment polyline state. All are properly used throughout the component.
Also applies to: 6-6, 23-24, 27-31
99-115: Excellent fix! pointer-events: none prevents blocking clicks.The addition of
pointer-events: nonecorrectly addresses the past review comment, ensuring the hint overlay doesn't interfere with vertex placement during polyline editing.
151-151: LGTM! Camera type union supports multi-panel views.The union type
PerspectiveCamera | OrthographicCameracorrectly supports the multi-panel view feature with both perspective and orthographic cameras.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1)
678-743: AdddatasetNameto useEffect and useMemo dependency arrays.The
datasetNameis a Recoil value (line 236) that changes when switching datasets. Both the useEffect (line 678) and the useMemo (line 334) usegetCameraPositionKey(datasetName)but lackdatasetNamein their dependency arrays. When the dataset switches, neither will re-run, causing camera restoration to fail—the localStorage lookup uses the previous dataset's key instead of the current one.Add
datasetNameto:
- Line 678 useEffect dependency array
- Line 334 useMemo dependency array
♻️ Duplicate comments (1)
app/packages/looker-3d/src/hooks/use-bounds.ts (1)
44-63: Reset retryCount on successful recomputation.The
recomputeBoundscallback successfully computes a bounding box but doesn't resetretryCount. If this callback is invoked after the polling logic has exhausted retries, subsequent polling cycles may short-circuit prematurely.Apply this diff to reset the counter on success:
const recomputeBounds = useCallback(() => { setIsComputing(true); if (!objectRef.current) { setBoundingBox(null); setIsComputing(false); return; } const box = new Box3().setFromObject(objectRef.current); if (!isFiniteBox(box)) { setBoundingBox(null); setIsComputing(false); return; } + retryCount.current = 0; setBoundingBox(box); setIsComputing(false); }, [objectRef]);
🧹 Nitpick comments (2)
app/packages/looker-3d/src/hooks/use-bounds.ts (1)
77-131: Avoid rapid isComputing flickering during polling.
setIsComputing(true)is called at the start of every poll iteration (line 80), causing the flag to flicker true/false every 50ms. This can result in janky loading UI if consumers render based onisComputing.Consider setting
isComputingonce at the beginning of the polling phase and only resetting it when polling concludes (success or failure).Example approach:
const getBoundingBox = () => { if (!isMounted) return; - setIsComputing(true); - if (!objectRef.current) { retryCount.current += 1; if (retryCount.current >= MAX_BOUNDING_BOX_RETRIES) { retryCount.current = 0; setBoundingBox(null); setIsComputing(false); return; } timeOutIdRef.current = window.setTimeout( getBoundingBox, BOUNDING_BOX_POLLING_INTERVAL ); return; } const box = new Box3().setFromObject(objectRef.current); if (!isFiniteBox(box)) { retryCount.current += 1; if (retryCount.current >= MAX_BOUNDING_BOX_RETRIES) { retryCount.current = 0; setBoundingBox(null); setIsComputing(false); return; } timeOutIdRef.current = window.setTimeout( getBoundingBox, BOUNDING_BOX_POLLING_INTERVAL ); return; } if (previousBox.current && boxesAreEqual(box, previousBox.current)) { unchangedCount.current += 1; } else { unchangedCount.current = 1; } previousBox.current = box; if (unchangedCount.current >= UNCHANGED_COUNT_THRESHOLD) { retryCount.current = 0; setBoundingBox(box); setIsComputing(false); } else { timeOutIdRef.current = window.setTimeout( getBoundingBox, BOUNDING_BOX_POLLING_INTERVAL ); } }; + setIsComputing(true); // this is a hack, yet to find a better way than polling to know when the asset is done loading // callbacks in loaders are not reliable timeOutIdRef.current = window.setTimeout( getBoundingBox, BOUNDING_BOX_POLLING_INTERVAL );app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx (1)
68-73: Destructure LGTM; tighten ref typing to match useFo3dBounds.useFo3dBounds expects RefObject. Your pcdContainerRef is typed as RefObject in this file. Tighten it for type-safety and editor help.
Suggested changes (outside this hunk):
- Import Group:
import type { BufferGeometry, Quaternion, Group } from "three";- Update param type:
pcdContainerRef: React.RefObject,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/packages/looker-3d/src/MediaTypePcd.tsx(1 hunks)app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(21 hunks)app/packages/looker-3d/src/fo3d/context.tsx(2 hunks)app/packages/looker-3d/src/fo3d/mesh/Obj.tsx(1 hunks)app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx(2 hunks)app/packages/looker-3d/src/fo3d/scene-controls/SceneControls.tsx(4 hunks)app/packages/looker-3d/src/fo3d/utils.ts(1 hunks)app/packages/looker-3d/src/hooks/use-bounds.test.ts(4 hunks)app/packages/looker-3d/src/hooks/use-bounds.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/fo3d/scene-controls/SceneControls.tsxapp/packages/looker-3d/src/fo3d/utils.tsapp/packages/looker-3d/src/fo3d/context.tsxapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsxapp/packages/looker-3d/src/hooks/use-bounds.tsapp/packages/looker-3d/src/MediaTypePcd.tsxapp/packages/looker-3d/src/hooks/use-bounds.test.tsapp/packages/looker-3d/src/fo3d/mesh/Obj.tsxapp/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx
🧬 Code graph analysis (4)
app/packages/looker-3d/src/fo3d/scene-controls/SceneControls.tsx (2)
app/packages/looker-3d/src/fo3d/context.tsx (1)
useFo3dContext(54-56)app/packages/looker-3d/src/fo3d/utils.ts (1)
getCameraPositionKey(24-25)
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (8)
app/packages/state/src/recoil/groups.ts (1)
isGroup(135-140)app/packages/looker-3d/src/state.ts (9)
isActivelySegmentingSelector(309-314)isSegmentingPointerDownAtom(316-319)isCurrentlyTransformingAtom(267-270)cameraPositionAtom(50-53)activeNodeAtom(204-207)currentHoveredPointAtom(209-212)clearTransformStateSelector(370-387)selectedPolylineVertexAtom(280-283)annotationPlaneAtom(341-357)app/packages/looker-3d/src/hooks/use-bounds.ts (1)
useFo3dBounds(31-154)app/packages/looker-3d/src/constants.ts (2)
DEFAULT_BOUNDING_BOX(27-32)DEFAULT_CAMERA_POSITION(24-24)app/packages/looker-3d/src/fo3d/utils.ts (1)
getCameraPositionKey(24-25)app/packages/looker-3d/src/annotation/MultiPanelView.tsx (1)
MultiPanelView(188-305)app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx (1)
Fo3dSceneContent(89-159)app/packages/looker-3d/src/annotation/annotation-toolbar/AnnotationToolbar.tsx (1)
AnnotationToolbar(130-254)
app/packages/looker-3d/src/hooks/use-bounds.test.ts (1)
app/packages/looker-3d/src/hooks/use-bounds.ts (1)
useFo3dBounds(31-154)
app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx (2)
app/packages/looker-3d/src/hooks/use-bounds.ts (1)
useFo3dBounds(31-154)app/packages/looker-3d/src/constants.ts (1)
DEFAULT_BOUNDING_BOX(27-32)
⏰ 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). (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
🔇 Additional comments (22)
app/packages/looker-3d/src/hooks/use-bounds.ts (2)
6-21: LGTM! Comprehensive finite-value validation.The
isFiniteBoxhelper correctly validates all six components (min/max x/y/z), addressing the previous concern about only checkingbox.max.x.
153-153: LGTM! Return shape matches documentation.The return object provides the expected API:
boundingBox,recomputeBounds, andisComputing.app/packages/looker-3d/src/hooks/use-bounds.test.ts (1)
1-125: LGTM! Tests comprehensively updated for new API.All test assertions correctly target
result.current.boundingBox, and the new test for non-finite boxes (lines 84-108) provides valuable coverage for theisFiniteBoxvalidation logic.app/packages/looker-3d/src/fo3d/mesh/Obj.tsx (2)
12-12: No issues found — the implementation is sound.The
useFoLoaderhook is a thin wrapper/decorator that delegates entirely to@react-three/fiber'suseLoader. It transparently preserves all parameters, generics, and behavior while adding only credentials handling via sessionStorage. Since all Suspense integration, promise caching, and error handling are delegated to the underlyinguseLoader, the original concerns do not apply. The architectural approach is clean and safe.
97-107: No issues found—the material-then-mesh loading pattern works correctly.The interdependent loading is properly managed by React Suspense via the
useLoaderhook from@react-three/fiber. TheuseFoLoaderwrapper suspends the component until each resource loads sequentially:
- Component suspends on the
materialsuseFoLoader call- Once materials finish loading, Suspense resolves and the materials object is synchronously available
- The mesh loader's callback then executes with the fully-loaded materials object
materials.preload()andloader.setMaterials(materials)execute with valid three.js objects- Component suspends again while the mesh loads with materials applied
No race conditions or timing issues exist—Suspense enforces the correct execution order.
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (10)
5-5: LGTM: Import additions are appropriate.The new imports for
CameraControls,useAtomValue, andstyled-componentsalign with the PR's feature additions for 3D annotation capabilities.Also applies to: 8-8, 18-18
22-23: LGTM: New component and state imports.The imports for
MultiPanelView,AnnotationToolbar, annotation state atoms, andFo3dSceneContentare correctly integrated to support the multi-panel annotation workflow.Also applies to: 37-37, 48-48, 52-52
60-65: LGTM: Simple styled component.The
MainContainerprovides a clean flex layout wrapper for the canvas content.
222-223: LGTM: lookAt state management is well-structured.The lookAt state is properly initialized from the bounding box center with a defensive null check. The use of
effectiveSceneBoundingBoxensures stability.Also applies to: 305-310
248-270: LGTM: Camera control disabling during transformations.The logic correctly disables camera controls during segmenting or transforming states and re-enables them otherwise. The separate effect ensures the config updates reactively.
Also applies to: 272-277
297-303: LGTM: Bounding box computation with useFo3dBounds.The custom hook provides bounding box computation with
isComputingstatus and manualrecomputeBoundstrigger, which is appropriate for async/polling scenarios.
334-340: LGTM: Dataset-scoped camera persistence.Using
getCameraPositionKey(datasetName)enables per-dataset camera state persistence, improving UX across different datasets.
432-446: Good defensive checks in resetActiveNode.The guards for right-click events and active segmentation prevent unintended state clearing during camera panning or annotation workflows.
768-782: LGTM: Multi-panel view trigger logic.The
shouldRenderMultiPanelViewmemo and the effect to recompute bounds when entering annotation mode are well-structured. The dependencies correctly includeisAnnotationPlaneEnabledto trigger recomputation when the plane is toggled.
809-851: LGTM: Conditional rendering for annotation workflows.The conditional rendering cleanly switches between
MultiPanelViewand the single-canvas layout based on annotation mode, withAnnotationToolbarconsistently rendered in annotate mode.app/packages/looker-3d/src/MediaTypePcd.tsx (1)
4-4: LGTM: Refactored to use shared utility.Moving
isPointCloudto@fiftyone/utilitiespromotes code reuse and reduces duplication. The usage on line 74 is correct.Also applies to: 74-74
app/packages/looker-3d/src/fo3d/utils.ts (1)
24-25: LGTM: Clean utility function for dataset-scoped keys.The
getCameraPositionKeyfunction provides a consistent way to generate dataset-scoped localStorage keys. The fallback to"fiftyone"is sensible.app/packages/looker-3d/src/fo3d/scene-controls/SceneControls.tsx (3)
1-1: LGTM: State integration for dataset-scoped persistence.The imports and usage of
fosanduseRecoilValueto accessdatasetNamecorrectly enable per-dataset camera state persistence.Also applies to: 6-6, 31-31
28-28: Good optimization: Skip frame updates during bounding box computation.The early return on line 44 when
isComputingSceneBoundingBoxis true prevents excessive localStorage writes during potentially expensive computation phases.Also applies to: 43-44
58-58: LGTM: Dataset-scoped camera position key.Using
getCameraPositionKey(datasetName)ensures camera positions are stored and restored per-dataset, improving UX when switching between datasets.app/packages/looker-3d/src/fo3d/context.tsx (1)
16-16: LGTM: Context extended with bounding box and lookAt state.The new fields
isComputingSceneBoundingBox,lookAt, andsetLookAtare properly typed and initialized with sensible defaults. These additions support the multi-panel annotation workflow.Also applies to: 18-19, 35-35, 37-38
app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx (1)
4-4: Import looks good; safe fallback for null bounds.Bringing in DEFAULT_BOUNDING_BOX is a sensible guard to avoid crashes/NaNs while bounds are unavailable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/packages/looker-3d/src/hooks/use-bounds.ts (2)
44-63: Regression: retryCount not reset in recompute; also cancel pending poll and clone Box3Manual recompute can leave retries “stuck” and race with an in‑flight poll; also avoid exposing mutable Box3 via state.
const recomputeBounds = useCallback(() => { - setIsComputing(true); + // Cancel any pending poll to avoid races with manual recompute + if (timeOutIdRef.current) { + window.clearTimeout(timeOutIdRef.current); + timeOutIdRef.current = null; + } + setIsComputing(true); if (!objectRef.current) { - setBoundingBox(null); - setIsComputing(false); + retryCount.current = 0; + unchangedCount.current = 0; + previousBox.current = null; + setBoundingBox(null); + setIsComputing(false); return; } const box = new Box3().setFromObject(objectRef.current); if (!isFiniteBox(box)) { - setBoundingBox(null); - setIsComputing(false); + retryCount.current = 0; + unchangedCount.current = 0; + previousBox.current = null; + setBoundingBox(null); + setIsComputing(false); return; } - setBoundingBox(box); + retryCount.current = 0; + unchangedCount.current = 0; + previousBox.current = box.clone(); + setBoundingBox(box.clone()); setIsComputing(false); }, [objectRef]);
39-39: Fix ref type: allow null for previousBoxThis is a null-initialized ref; current typing can fail under strictNullChecks.
As per coding guidelines (TypeScript best practices).
- const previousBox = useRef<Box3>(null); + const previousBox = useRef<Box3 | null>(null);
🧹 Nitpick comments (5)
app/packages/looker-3d/src/renderables/pcd/index.tsx (1)
93-93: Good fix: explicit null/undefined check now correctly treats 0 as valid.This change fixes a bug where
minZ = 0would have been treated as falsy in a truthiness test, causing the fallback toboundingBox.min.dot(upVector). The explicit check ensures that zero values along the Z-axis (e.g., ground plane at z=0) are now handled correctly.Note: If you prefer a more concise pattern,
minZ != null(loose equality) checks both null and undefined in one comparison, though the current explicit approach is also clear and intentional.app/packages/looker-3d/src/hooks/use-bounds.ts (4)
120-126: Store/emit clones of Box3 to prevent external mutation of React stateConsumers could mutate Box3 in place; clone before persisting and emitting.
- previousBox.current = box; + previousBox.current = box.clone(); - if (unchangedCount.current >= UNCHANGED_COUNT_THRESHOLD) { + if (unchangedCount.current >= UNCHANGED_COUNT_THRESHOLD) { retryCount.current = 0; - setBoundingBox(box); + setBoundingBox(box.clone()); setIsComputing(false); } else {
142-149: Cleanup should also reset unchangedCount and previousBoxPrevents stale state across unmount/remount.
return () => { isMounted = false; - retryCount.current = 0; - setIsComputing(false); + retryCount.current = 0; + unchangedCount.current = 0; + previousBox.current = null; + setIsComputing(false); if (timeOutIdRef.current) { window.clearTimeout(timeOutIdRef.current); }
65-69: Predicate gating can starve computationIf predicate() flips true without changing function identity, this early return stops polling forever. Consider checking predicate inside the polling loop and re‑arming the timer until it returns true, instead of returning from the effect.
4-7: Make interval and retries configurable (defaults unchanged)Expose { intervalMs = 50, maxRetries = 1000, stableSamples = 6 } via hook options for tests and tuning; keep current constants as defaults.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(21 hunks)app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx(2 hunks)app/packages/looker-3d/src/fo3d/scene-controls/SceneControls.tsx(5 hunks)app/packages/looker-3d/src/fo3d/utils.ts(2 hunks)app/packages/looker-3d/src/hooks/use-bounds.ts(6 hunks)app/packages/looker-3d/src/hooks/use-mesh-material-controls.ts(3 hunks)app/packages/looker-3d/src/renderables/pcd/index.tsx(1 hunks)app/packages/looker-3d/src/state.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker-3d/src/fo3d/point-cloud/use-pcd-material.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/hooks/use-mesh-material-controls.tsapp/packages/looker-3d/src/hooks/use-bounds.tsapp/packages/looker-3d/src/renderables/pcd/index.tsxapp/packages/looker-3d/src/fo3d/scene-controls/SceneControls.tsxapp/packages/looker-3d/src/fo3d/utils.tsapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsxapp/packages/looker-3d/src/state.ts
🧬 Code graph analysis (4)
app/packages/looker-3d/src/hooks/use-mesh-material-controls.ts (2)
app/packages/looker-3d/src/state.ts (1)
avoidZFightingAtom(369-377)app/packages/looker-3d/src/fo3d/utils.ts (1)
getThreeMaterialFromFo3dMaterial(155-198)
app/packages/looker-3d/src/fo3d/scene-controls/SceneControls.tsx (3)
app/packages/looker-3d/src/fo3d/context.tsx (1)
useFo3dContext(54-56)app/packages/looker-3d/src/state.ts (1)
avoidZFightingAtom(369-377)app/packages/looker-3d/src/fo3d/utils.ts (1)
getCameraPositionKey(24-25)
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (8)
app/packages/state/src/recoil/groups.ts (1)
isGroup(135-140)app/packages/looker-3d/src/state.ts (9)
isActivelySegmentingSelector(309-314)isSegmentingPointerDownAtom(316-319)isCurrentlyTransformingAtom(267-270)cameraPositionAtom(50-53)activeNodeAtom(204-207)currentHoveredPointAtom(209-212)clearTransformStateSelector(380-397)selectedPolylineVertexAtom(280-283)annotationPlaneAtom(341-357)app/packages/looker-3d/src/hooks/use-bounds.ts (1)
useFo3dBounds(31-154)app/packages/looker-3d/src/constants.ts (2)
DEFAULT_BOUNDING_BOX(27-32)DEFAULT_CAMERA_POSITION(24-24)app/packages/looker-3d/src/fo3d/utils.ts (1)
getCameraPositionKey(24-25)app/packages/looker-3d/src/annotation/MultiPanelView.tsx (1)
MultiPanelView(188-305)app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx (1)
Fo3dSceneContent(89-159)app/packages/looker-3d/src/annotation/annotation-toolbar/AnnotationToolbar.tsx (1)
AnnotationToolbar(130-254)
app/packages/looker-3d/src/state.ts (3)
app/packages/looker-3d/src/labels/loader.ts (1)
OverlayLabel(8-27)app/packages/looker-3d/src/annotation/types.ts (9)
HoveredPolylineInfo(28-33)TransformMode(36-36)TransformSpace(37-37)TransformedLabelData(79-84)SelectedPoint(44-48)PolylinePointTransform(87-91)SegmentPolylineState(93-98)TempPolyline(100-106)AnnotationPlaneState(108-115)app/packages/looker-3d/src/types.ts (1)
TransformArchetype(79-83)
🪛 Gitleaks (8.28.0)
app/packages/looker-3d/src/state.ts
[high] 360-360: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 370-370: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
🔇 Additional comments (22)
app/packages/looker-3d/src/fo3d/utils.ts (2)
24-25: LGTM! Dataset-specific camera position keys.The new
getCameraPositionKeyfunction provides proper namespacing for per-dataset camera state persistence, preventing state collisions across different datasets.
155-162: LGTM! Z-fighting control integrated correctly.The
avoidZFightingparameter with a default value maintains backward compatibility while enabling dynamic control of thedepthWritematerial property to prevent z-fighting artifacts.app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (7)
248-270: LGTM! Camera controls properly disabled during transformations.The logic correctly disables camera controls when the user is actively transforming or segmenting, preventing conflicting interactions, and re-enables them otherwise.
272-277: LGTM! Effect properly responds to transform state changes.The effect ensures camera controls configuration updates whenever the transforming or segmenting state changes, maintaining consistent interaction behavior.
297-310: LGTM! Effective bounding box with defensive fallback.The fallback to
DEFAULT_BOUNDING_BOXwhensceneBoundingBoxis null ensures robust behavior, and the automaticlookAtinitialization from the bounding box center is a good UX enhancement.
430-450: LGTM! Defensive guards in resetActiveNode.The callback correctly:
- Ignores right-click events (contextmenu) to avoid interfering with camera panning
- Skips reset during active segmentation to preserve the user's workflow
- Properly clears all transform and selection state
676-742: LGTM! Comprehensive camera initialization logic.The effect correctly:
- Guards execution until bounding box computation completes via
isComputingSceneBoundingBox- Attempts to restore saved camera state from localStorage with proper error handling
- Falls back through multiple camera position sources (foScene props, saved state, computed from bounds)
- Includes all relevant dependencies
769-781: LGTM! Multi-panel view decision and bounds recomputation.The
shouldRenderMultiPanelViewcomputation correctly considers annotation mode, group media visibility, and scene initialization. The accompanying effect appropriately triggers bounds recomputation when switching to multi-panel view.
808-850: LGTM! Clean dual-path rendering strategy.The conditional rendering properly switches between:
MultiPanelViewfor annotation mode with multiple synchronized camera viewsMainContainerwithFo3dSceneContentfor standard viewingThe
AnnotationToolbaris correctly rendered conditionally for annotate mode.app/packages/looker-3d/src/hooks/use-mesh-material-controls.ts (1)
14-14: LGTM! Z-fighting state properly integrated.The hook correctly:
- Reads
avoidZFightingfrom Recoil state- Passes it to
getThreeMaterialFromFo3dMaterialfor material creation- Includes it in the
useMemodependency array to trigger material updates when the setting changesAlso applies to: 137-161
app/packages/looker-3d/src/fo3d/scene-controls/SceneControls.tsx (2)
46-66: LGTM! Efficient camera state persistence with dataset-specific keys.The logic correctly:
- Early-returns during bounding box computation to avoid premature writes
- Uses
getCameraPositionKey(datasetName)for proper per-dataset camera state isolation- Throttles writes to every 250ms to prevent excessive localStorage operations
92-98: LGTM! Z-fighting toggle properly integrated.The "Avoid Z fighting" control is correctly wired to
avoidZFightingAtom, enabling users to dynamically control z-fighting behavior across the 3D scene.app/packages/looker-3d/src/state.ts (7)
55-72: LGTM! Dataset-specific state isolation.The addition of
prependDatasetNameInKey: truetoshadeByAtomandcustomColorMapAtomensures proper state isolation across different datasets, preventing unintended state sharing.
137-145: LGTM! Annotation toolbar position persistence.The new
annotationToolbarPositionAtomcorrectly persists the toolbar's vertical position to browser storage, providing a consistent user experience across sessions.
214-223: LGTM! Hover state atoms properly typed.Both
hoveredLabelAtomandhoveredPolylineInfoAtomuse appropriate types (OverlayLabel | nullandHoveredPolylineInfo | null), providing type safety for annotation hover interactions.
245-290: LGTM! Annotation and transform state atoms well-structured.The new atoms (
selectedLabelForAnnotationAtom,transformModeAtom,transformSpaceAtom,currentArchetypeSelectedForTransformAtom,isCurrentlyTransformingAtom,transformedLabelsAtom,selectedPolylineVertexAtom,polylinePointTransformsAtom) follow Recoil best practices with proper types, keys, and sensible defaults.
292-339: LGTM! Polyline and segmentation state atoms.The atoms (
polylineEffectivePointsAtom,segmentPolylineStateAtom,isActivelySegmentingSelector,isSegmentingPointerDownAtom,tempPolylinesAtom,snapCloseAutomaticallyAtom,editSegmentsModeAtom,sharedCursorPositionAtom) are well-designed with appropriate types and defaults for the polyline annotation workflow.
341-377: LGTM! Annotation plane and Z-fighting atoms with appropriate persistence.Both atoms use proper persistence strategies:
annotationPlaneAtomusessessionStoragefor transient annotation plane stateisSnapToAnnotationPlaneAtomandavoidZFightingAtomuse persistent browser storage for user preferences
379-397: LGTM! Clean transform state reset pattern.The
clearTransformStateSelectorprovides a centralized cleanup mechanism that resets transform UI state while preservingpolylinePointTransforms(user edits), which is the correct behavior for maintaining annotation data across interactions.app/packages/looker-3d/src/hooks/use-bounds.ts (3)
12-21: Finite check helper looks solidCovers all min/max components across axes; good guard against ±Infinity/NaN cases.
31-36: All call sites properly updated; review concern verified as addressedThe verification confirms all
useFo3dBoundscalls have been correctly migrated to the new API shape:
use-pcd-material.tsx(line 68) destructuresboundingBoxMediaTypeFo3d.tsx(line 301) destructuresrecomputeBounds,isComputing, andsceneBoundingBox- Test file properly accesses through hook result wrapper
- No legacy patterns expecting direct
Box3return found
44-63: Add tests for manual recompute behavior and returned box immutabilityThe hook exports
recomputeBoundsfor manual triggering but lacks test coverage for:
- Counter/timer reset when
recomputeBoundsis called directly (currently they are not reset, which could cause state inconsistency on repeated calls)- Timer cancellation (pending
getBoundingBoxtimeouts are not cleared before recompute)- Returned box immutability (Box3 instances are mutable; mutations to
boundingBoxwill affect internal state since it's not cloned)Tests should verify these behaviors work correctly. Note: the current implementation may require fixes to support these test scenarios (e.g., cloning the box before returning, resetting internal counters and canceling timers in
recomputeBounds).
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (34)
app/packages/looker-3d/src/utils.ts (3)
223-230: Normalize thetovector for consistency and safety.Line 227 normalizes
from, but line 228 doesn't normalizeto. SincesetFromUnitVectorsexpects unit vectors, both should be normalized for defensive programming, even if the parameter name suggestsupVectorNormalizedis already normalized.Apply this diff:
export const getGridQuaternionFromUpVector = ( upVectorNormalized: Vector3, targetNormal: Vector3 = new Vector3(0, 1, 0) ) => { const from = targetNormal.clone().normalize(); - const to = upVectorNormalized.clone(); + const to = upVectorNormalized.clone().normalize(); return new Quaternion().setFromUnitVectors(from, to); };
258-270: Avoid theas anytype cast for better type safety.Line 264 uses
as anyto bypass TypeScript's type checking. Consider creating a properVector2instance or using a more specific type.Apply this diff:
+import { Vector2 } from 'three'; export function getPlaneIntersection( raycaster: Raycaster, camera: Camera, ndc: { x: number; y: number }, plane: Plane ): Vector3 | null { - raycaster.setFromCamera(ndc as any, camera); + raycaster.setFromCamera(new Vector2(ndc.x, ndc.y), camera); const point = new Vector3(); if (raycaster.ray.intersectPlane(plane, point)) { return point; } return null; }
272-281: Document the semantics of theconstantparameter.Line 280 negates the
constantwhen creating the Plane. This suggests the input parameter has different semantics than the Three.js plane constant (which appears in the equationnormal · point + constant = 0). Please clarify in the documentation what theconstantparameter represents (e.g., distance from origin) and why it's negated, or consider renaming it to better reflect its meaning.app/packages/state/src/recoil/atoms.ts (1)
411-414: Guard against stale multi-panel state across dataset/modal changesGlobal, non-persistent is fine, but ensure it resets to false when dataset/modal changes to avoid cross-context leakage. Optionally colocate under 3D state for cohesion.
app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx (1)
65-65: Type the ref for better safetyAdd a concrete type to avoid any/unknown escaping.
-import { AnimationMixer, type Quaternion, type Vector3 } from "three"; +import { AnimationMixer, type Object3D, type Quaternion, type Vector3 } from "three"; @@ - const groupRef = useRef(); + const groupRef = useRef<Object3D | null>(null);app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx (1)
65-67: Prevent AnimationMixer leaks on re-load/toggleNew mixers are created when fbx changes; add cleanup to stop/uncache actions.
- const mixer = useMemo(() => new AnimationMixer(fbx), [fbx]); + const mixer = useMemo(() => new AnimationMixer(fbx), [fbx]); + useEffect(() => { + return () => { + try { + mixer.stopAllAction(); + if (fbx) mixer.uncacheRoot(fbx); + } catch {} + }; + }, [mixer, fbx]);app/packages/looker-3d/src/state.ts (5)
9-20: Use type‑only imports from three to avoid bundling runtime typesThese are used as types; import with
import typeto reduce bundle and avoid side effects.-import { Vector3, Vector3Tuple } from "three"; +import type { Vector3, Vector3Tuple } from "three";
292-297: Avoid sharing a single default array instance across atomFamily keysUse a factory default so each key starts with its own array instance.
-export const polylineEffectivePointsAtom = atomFamily<Vector3Tuple[][], string>({ - key: "fo3d-polylineEffectivePoints", - default: [], -}); +export const polylineEffectivePointsAtom = atomFamily<Vector3Tuple[][], string>( + { + key: "fo3d-polylineEffectivePoints", + default: () => [], + } +);
363-379: Scope persisted annotation plane per dataset to prevent cross‑dataset bleedPersisting in sessionStorage is good; also prepend dataset name for isolation.
effects: [ - getBrowserStorageEffectForKey("fo3d-annotationPlane", { + getBrowserStorageEffectForKey("fo3d-annotationPlane", { useJsonSerialization: true, sessionStorage: true, + prependDatasetNameInKey: true, }), ],
381-389: Likewise, dataset‑scope snap‑to‑plane toggleAligns with plane state scoping.
effects: [ - getBrowserStorageEffectForKey("fo3d-isSnapToAnnotationPlane", { + getBrowserStorageEffectForKey("fo3d-isSnapToAnnotationPlane", { valueClass: "boolean", + prependDatasetNameInKey: true, }), ],
401-419: Reset segmenting pointer state when clearing transform statePrevents “stuck mouse down” after global reset.
set: ({ set }) => { set(transformModeAtom, "translate"); set(transformSpaceAtom, "world"); set(selectedPolylineVertexAtom, null); set(currentArchetypeSelectedForTransformAtom, null); set(isCurrentlyTransformingAtom, false); + set(isSegmentingPointerDownAtom, false); // Note: We don't clear polylinePointTransforms here as it should persist set(segmentPolylineStateAtom, { isActive: false, vertices: [], currentMousePosition: null, isClosed: false, }); },app/packages/looker-3d/src/fo3d/mesh/Stl.tsx (3)
53-53: Type the mesh stateAvoid
anyby annotating state asMesh | null.- const [mesh, setMesh] = useState(null); + const [mesh, setMesh] = useState<Mesh | null>(null);
57-69: Dispose old geometry/mesh on updates to prevent leaksEach points/material change creates a new Mesh; add cleanup to dispose the previous geometry.
- useEffect(() => { - if (!material) { - return; - } - - if (points) { - points.computeVertexNormals(); - - const newMesh = new Mesh(points, material); - setMesh(newMesh); - } - }, [points, stlUrl, material]); + useEffect(() => { + if (!material || !points) return; + points.computeVertexNormals(); + const newMesh = new Mesh(points, material); + setMesh(newMesh); + return () => { + try { + newMesh.geometry?.dispose?.(); + } catch {} + }; + }, [points, material]);
45-52: Cloning geometry per panel is fine; consider sharing attributes for memoryIf memory spikes in multi‑panel, derive clones that share BufferAttributes (keep index/attributes, vary normals if needed) or move to InstancedMesh.
app/packages/looker-3d/src/fo3d/mesh/Obj.tsx (2)
42-49: Avoid repeated deep clones; cache per-source and dispose on unmount.Deep cloning on each
isInMultiPanelViewtoggle reallocates geometries/materials. Cache the clone perobjUrl(ormesh_) and dispose when unmounted to prevent leaks.@@ - const mesh = useMemo(() => { - if (isInMultiPanelView && mesh_) { - return mesh_.clone(true); - } - return mesh_; - }, [mesh_, isInMultiPanelView]); + const cloneRef = useRef<THREE.Object3D | null>(null); + const mesh = useMemo(() => { + if (!mesh_) return null; + if (!isInMultiPanelView) return mesh_; + if (!cloneRef.current) cloneRef.current = mesh_.clone(true); + return cloneRef.current; + }, [mesh_, isInMultiPanelView]); + useEffect(() => { + return () => { + cloneRef.current?.traverse((o: any) => { + o.geometry?.dispose?.(); + o.material?.dispose?.(); + }); + cloneRef.current = null; + }; + }, []);Also applies to: 122-130
40-41: Tighten types for loader outputs.
OBJLoaderreturns aTHREE.Group. Typing clarifies intent and helps tooltips.- const mesh_ = useFoLoader(OBJLoader, objUrl); + const mesh_ = useFoLoader(OBJLoader, objUrl) as THREE.Group | undefined;Repeat in the custom-material path.
Also applies to: 115-116
app/packages/looker-3d/src/fo3d/mesh/Ply.tsx (2)
190-201: Cache geometry clones to avoid churn on view toggles.Cloning on every
isInMultiPanelViewtransition reallocates large buffers. Cache pergeometry_and dispose on unmount.- const geometry = useMemo(() => { - if (isInMultiPanelView && geometry_) { - return geometry_.clone(); - } - return geometry_; - }, [geometry_, isInMultiPanelView]); + const cloneRef = useRef<THREE.BufferGeometry | null>(null); + const geometry = useMemo(() => { + if (!geometry_) return null; + if (!isInMultiPanelView) return geometry_; + if (!cloneRef.current) cloneRef.current = geometry_.clone(); + return cloneRef.current; + }, [geometry_, isInMultiPanelView]); + useEffect(() => { + return () => { + cloneRef.current?.dispose?.(); + cloneRef.current = null; + }; + }, []);
60-67: Type refs for clarity.Add explicit ref types to improve IntelliSense and prevent accidental misuse.
- const pointsContainerRef = useRef(); + const pointsContainerRef = useRef<THREE.Points>(null); @@ - const mesh = useMemo(() => new Points(geometry), [geometry]); + const mesh = useMemo(() => new Points(geometry), [geometry]);(Repeat similar typing for mesh refs in material override/no-override components if needed.)
Also applies to: 128-131, 150-153
app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx (2)
108-121: Use epsilon-based keys for dedup to avoid float fragility.String-equality on raw floats can miss duplicates due to tiny deltas. Use a quantized key.
- const visitedPoints = new Set<string>(); + const visitedPoints = new Set<string>(); + const posKey = (p: Vector3Tuple, places = 4) => + p.map((n) => Number(n.toFixed(places))).join("|"); @@ - const key = `${point[0]}-${point[1]}-${point[2]}`; + const key = posKey(point);If you prefer tolerance-based equality, reuse
positionsEqualto build keys or filter duplicates before mapping.Also applies to: 122-151
163-191: Don’t render centroid marker when there are no points.Avoid showing a floating centroid at [0,0,0].
- const centroidMarker = useMemo(() => { - if (!isAnnotateMode || !isSelectedForAnnotation) return null; + const centroidMarker = useMemo(() => { + if (!isAnnotateMode || !isSelectedForAnnotation) return null; + const hasPoints = + effectivePoints3d.length > 0 && + effectivePoints3d.some((seg) => seg.length > 0); + if (!hasPoints) return null; @@ - }, [ + }, [ isAnnotateMode, isSelectedForAnnotation, + effectivePoints3d, centroid, strokeAndFillColor, label._id, ]);app/packages/looker-3d/src/annotation/AnnotationPlane.tsx (2)
58-86: Plane sizing: avoid rounding down; just pad max orthogonal extents.
Math.round(dimensions[i])can shrink the plane. Use raw sizes and pad.- orthogonalSizes.push(Math.round(dimensions[i])); + orthogonalSizes.push(dimensions[i]); @@ - return Math.max(size.x, size.y, size.z, 10) * 2; + return Math.max(size.x, size.y, size.z, 10) * 2;(Second line unchanged; included for context.)
Also applies to: 89-91
101-132: Axis gating: show all axes when upVector isn’t strongly axis-aligned.If
upVectoris diagonal (e.g., normalized [1,1,0]), current< 0.5/max checks can disable translation unintentionally. Add a tolerance and fall back to all axes.- const maxComponent = Math.max(upAbs.x, upAbs.y, upAbs.z); + const maxComponent = Math.max(upAbs.x, upAbs.y, upAbs.z); + const STRONG_ALIGN = 0.9; @@ - // For translate mode, only show the axis aligned with upVector - return { - showX: isXUp, - showY: isYUp, - showZ: isZUp, - }; + // If not strongly axis-aligned, expose all axes to avoid locking the user out + if (maxComponent < STRONG_ALIGN) { + return { showX: true, showY: true, showZ: true }; + } + return { showX: isXUp, showY: isYUp, showZ: isZUp };app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx (2)
22-87: Tighten prop types forFo3dSceneContentProps.Several props are
anyand optional values are unused (e.g.,zoom). Consider refining to concrete types (FoScene, PointCloudSettings, Sample) and removing unused props to reduce surface area.
151-156: Consider lazy boundary for annotation UI.Wrap
AnnotationControlsinSuspensewithfallback={null}to keep the main scene responsive if annotation modules lazy-load.- {mode === "annotate" && <AnnotationControls />} + {mode === "annotate" && ( + <Suspense fallback={null}> + <AnnotationControls /> + </Suspense> + )}app/packages/looker-3d/src/labels/polyline.tsx (4)
80-95: Skip degenerate segments when rendering linesGuard against segments with fewer than 2 points to avoid Three.js warnings and invisible Line geometries during early/partial edits.
- const lines = useMemo( - () => - effectivePoints3d.map((pts, i) => ( + const lines = useMemo( + () => + effectivePoints3d + .filter((pts) => pts.length >= 2) + .map((pts, i) => ( <LineDrei key={`polyline-${label._id}-${i}`} lineWidth={lineWidth} segments points={pts} color={strokeAndFillColor} rotation={rotation} - transparent={opacity < 0.2} + transparent={opacity < 1} opacity={opacity} onPointerOver={() => handleSegmentPointerOver(i)} onPointerOut={handleSegmentPointerOut} onClick={handleSegmentClick} /> )), [ effectivePoints3d, strokeAndFillColor, lineWidth, rotation, label._id, handleSegmentPointerOver, handleSegmentPointerOut, handleSegmentClick, ] );
88-90: Transparency threshold should match opacityUse transparent when opacity < 1 to ensure proper blending; current 0.2 threshold can produce hard edges at 0.2–0.99.
- transparent={opacity < 0.2} + transparent={opacity < 1}
169-177: Anchor transform gizmo to the temp-transformed contentWhen tempLabelTransforms are applied, keep the TransformControls positioned at that temp position so handles don’t drift.
- <Transformable + <Transformable archetype="polyline" isSelectedForTransform={isSelectedForAnnotation} - transformControlsPosition={centroid as THREE.Vector3Tuple} + transformControlsPosition={ + (tempTransforms?.position as THREE.Vector3Tuple) ?? + (centroid as THREE.Vector3Tuple) + } transformControlsRef={transformControlsRef} onTransformEnd={handleTransformEnd} onTransformChange={handleTransformChange} explicitObjectRef={contentRef} > <group ref={contentRef} position={tempTransforms?.position} quaternion={tempTransforms?.quaternion}Please verify handleTransformEnd resets contentRef.position before further edits so the gizmo doesn’t accumulate offsets. Based on relevant hook behavior in app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx.
Also applies to: 178-200
141-149: Clear mesh ref array on unmountYou dispose geometry, but keeping references in meshesRef can retain objects longer than necessary.
useEffect(() => { return () => { meshesRef.current.forEach((mesh) => { if (mesh.geometry) { mesh.geometry.dispose(); } }); + meshesRef.current = []; }; }, []);app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx (4)
152-176: Scale marker correctly for orthographic camerasDistance-based scaling doesn’t work in ortho views; use inverse zoom for consistent screen-space size (as done in Crosshair3D).
- useFrame(({ clock, camera }) => { - const distance = camera.position.distanceTo(position); - - const rawScale = distance; - const minScale = 2; - const maxScale = 10.0; - const screenSpaceScale = Math.max(minScale, Math.min(maxScale, rawScale)); + useFrame(({ clock, camera }) => { + const isOrtho = camera instanceof THREE.OrthographicCamera; + const baseScale = isOrtho ? 1 / (camera as THREE.OrthographicCamera).zoom + : camera.position.distanceTo(position); + const minScale = 2; + const maxScale = 10.0; + const screenSpaceScale = Math.max(minScale, Math.min(maxScale, baseScale)); let finalScale = screenSpaceScale;Test in side panels (orthographic) by zooming; the marker should keep a near-constant on-screen size.
198-199: Keep transform handles aligned with temp transformsWhen tempVertexTransforms are active, use that position for transformControlsPosition to avoid a visual offset.
- transformControlsPosition={position.toArray()} + transformControlsPosition={ + (tempVertexTransforms?.position as THREE.Vector3Tuple) ?? + position.toArray() + }
184-189: Cleanup hovered state on unmountPrevent stale hover HUD by clearing hoveredPolylineInfo on unmount.
useEffect(() => { return () => { setTempVertexTransforms(null); + setHoveredPolylineInfo(null); }; - }, []); + }, [setHoveredPolylineInfo]);
200-222: Optional: prefer onPointerDown for selectionUsing onClick can fire after a drag, leading to unintended reselection. onPointerDown avoids that ambiguity.
- <mesh + <mesh ref={meshRef} position={position} - onPointerOver={() => { + onPointerOver={() => { ... }} - onClick={handlePointClick} + onPointerDown={handlePointClick} >Confirm no regressions with click-to-select on stationary clicks.
app/packages/looker-3d/src/annotation/MultiPanelView.tsx (1)
88-166: Direction basis can be simplified and made robustConstruct an orthonormal basis from upVector (u, r, f) and pick directions from it; reduces special-cases and handles arbitrary up vectors.
const calculateCameraPositionForSidePanel = ( viewType: ViewType, upVector: Vector3, lookAt: Vector3, sceneBoundingBox: Box3 | null ): Vector3 => { @@ - const upDir = upVector.clone().normalize(); - const center = lookAt.clone(); - // Create orthogonal vectors for different views - let direction: Vector3; - switch (viewType) { - case "Top": - direction = upDir.clone(); - break; - case "Bottom": - direction = upDir.clone().negate(); - break; - case "Left": - if (Math.abs(upDir.y) > 0.9) { - direction = new Vector3(-1, 0, 0); - } else { - const right = new Vector3(0, 1, 0).cross(upDir).normalize(); - direction = right.negate(); - } - break; - case "Right": - if (Math.abs(upDir.y) > 0.9) { - direction = new Vector3(1, 0, 0); - } else { - direction = new Vector3(0, 1, 0).cross(upDir).normalize(); - } - break; - case "Front": - if (Math.abs(upDir.y) > 0.9) { - direction = new Vector3(0, 0, 1); - } else { - const left = new Vector3(0, -1, 0).cross(upDir).normalize(); - direction = upDir.clone().cross(left).normalize(); - } - break; - case "Back": - if (Math.abs(upDir.y) > 0.9) { - direction = new Vector3(0, 0, -1); - } else { - const left = new Vector3(0, 1, 0).cross(upDir).normalize(); - direction = upDir.clone().cross(left).normalize(); - } - break; - default: - direction = upDir.clone(); - } + const u = upVector.clone().normalize(); + const ref = Math.abs(u.y) < 0.9 ? new Vector3(0, 1, 0) : new Vector3(1, 0, 0); + const r = ref.clone().cross(u).normalize(); // right + const f = u.clone().cross(r).normalize(); // forward + + const center = lookAt.clone(); + const direction = + viewType === "Top" ? u : + viewType === "Bottom" ? u.clone().negate() : + viewType === "Left" ? r.clone().negate() : + viewType === "Right" ? r : + viewType === "Front" ? f : + f.clone().negate(); // Backapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1)
397-409: Minor: use actual center for POV when bounds are knownCurrent POV default uses center (0,0,0); consider passing the computed center to keep framing consistent across scenes.
- return calculateCameraPositionForUpVector( - new Vector3(0, 0, 0), - size, - upVector, - 1.5, - "pov" - ); + const center = effectiveSceneBoundingBox.getCenter(new Vector3()); + return calculateCameraPositionForUpVector(center, size, upVector, 1.5, "pov");Validate on non-origin scenes that initial POV looks natural.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/packages/looker-3d/src/annotation/AnnotationPlane.tsx(1 hunks)app/packages/looker-3d/src/annotation/Crosshair3D.tsx(1 hunks)app/packages/looker-3d/src/annotation/MultiPanelView.tsx(1 hunks)app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx(1 hunks)app/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsx(1 hunks)app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx(1 hunks)app/packages/looker-3d/src/constants.ts(4 hunks)app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx(1 hunks)app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(21 hunks)app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx(3 hunks)app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx(3 hunks)app/packages/looker-3d/src/fo3d/mesh/Obj.tsx(7 hunks)app/packages/looker-3d/src/fo3d/mesh/Ply.tsx(4 hunks)app/packages/looker-3d/src/fo3d/mesh/Stl.tsx(3 hunks)app/packages/looker-3d/src/labels/polyline.tsx(3 hunks)app/packages/looker-3d/src/state.ts(8 hunks)app/packages/looker-3d/src/utils.ts(6 hunks)app/packages/state/src/recoil/atoms.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/packages/looker-3d/src/constants.ts
- app/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsx
- app/packages/looker-3d/src/annotation/Crosshair3D.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/state/src/recoil/atoms.tsapp/packages/looker-3d/src/fo3d/mesh/Obj.tsxapp/packages/looker-3d/src/fo3d/mesh/Gltf.tsxapp/packages/looker-3d/src/labels/polyline.tsxapp/packages/looker-3d/src/fo3d/mesh/Fbx.tsxapp/packages/looker-3d/src/annotation/PolylinePointMarker.tsxapp/packages/looker-3d/src/annotation/MultiPanelView.tsxapp/packages/looker-3d/src/annotation/AnnotationPlane.tsxapp/packages/looker-3d/src/fo3d/mesh/Ply.tsxapp/packages/looker-3d/src/utils.tsapp/packages/looker-3d/src/fo3d/Fo3dCanvas.tsxapp/packages/looker-3d/src/fo3d/mesh/Stl.tsxapp/packages/looker-3d/src/annotation/usePolylineAnnotation.tsxapp/packages/looker-3d/src/state.tsapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx
🧬 Code graph analysis (13)
app/packages/looker-3d/src/fo3d/mesh/Obj.tsx (2)
app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)app/packages/looker-3d/src/hooks/use-fo-loaders.ts (1)
useFoLoader(14-39)
app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx (1)
app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)
app/packages/looker-3d/src/labels/polyline.tsx (6)
app/packages/looker-3d/src/labels/shared.tsx (1)
OverlayProps(3-5)app/packages/looker-3d/src/labels/shared/hooks.ts (3)
useHoverState(12-27)useEventHandlers(32-44)useLabelColor(49-69)app/packages/looker-3d/src/state.ts (2)
selectedLabelForAnnotationAtom(245-248)tempLabelTransformsAtom(341-350)app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx (1)
usePolylineAnnotation(32-386)app/packages/looker-3d/src/labels/polygon-fill-utils.ts (1)
createFilledPolygonMeshes(303-326)app/packages/looker-3d/src/labels/shared/TransformControls.tsx (1)
Transformable(38-115)
app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx (2)
app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)app/packages/looker-3d/src/hooks/use-fo-loaders.ts (1)
useFoLoader(14-39)
app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx (4)
app/packages/looker-3d/src/state.ts (7)
hoveredPolylineInfoAtom(220-223)transformModeAtom(250-253)selectedPolylineVertexAtom(280-283)currentArchetypeSelectedForTransformAtom(260-264)segmentPolylineStateAtom(299-307)editSegmentsModeAtom(331-334)tempVertexTransformsAtom(352-361)app/packages/looker-3d/src/annotation/types.ts (1)
SelectedPoint(44-48)app/packages/looker-3d/src/labels/shared/TransformControls.tsx (1)
Transformable(38-115)app/packages/looker-3d/src/constants.ts (1)
LABEL_3D_ANNOTATION_POINT_SELECTED_FOR_TRANSFORMATION_COLOR(94-95)
app/packages/looker-3d/src/annotation/MultiPanelView.tsx (10)
app/packages/looker-3d/src/fo3d/context.tsx (1)
useFo3dContext(54-56)app/packages/state/src/hooks/useBrowserStorage.ts (1)
useBrowserStorage(4-67)app/packages/looker-3d/src/state.ts (4)
activeNodeAtom(204-207)currentHoveredPointAtom(209-212)selectedPolylineVertexAtom(280-283)currentArchetypeSelectedForTransformAtom(260-264)app/packages/looker-3d/src/components/PcdColormapModal.tsx (1)
PcdColorMapTunnel(37-37)app/packages/looker-3d/src/StatusBar.tsx (1)
StatusBar(371-431)app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx (1)
Fo3dSceneContent(89-158)app/packages/looker-3d/src/fo3d/Gizmos.tsx (1)
Gizmos(83-183)app/packages/looker-3d/src/annotation/AnnotationPlane.tsx (1)
AnnotationPlane(21-347)app/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsx (1)
SegmentPolylineRenderer(34-573)app/packages/looker-3d/src/annotation/Crosshair3D.tsx (1)
Crosshair3D(55-123)
app/packages/looker-3d/src/annotation/AnnotationPlane.tsx (3)
app/packages/looker-3d/src/state.ts (4)
annotationPlaneAtom(363-379)segmentPolylineStateAtom(299-307)transformModeAtom(250-253)currentArchetypeSelectedForTransformAtom(260-264)app/packages/looker-3d/src/fo3d/context.tsx (1)
useFo3dContext(54-56)app/packages/looker-3d/src/labels/shared/TransformControls.tsx (1)
Transformable(38-115)
app/packages/looker-3d/src/fo3d/mesh/Ply.tsx (2)
app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)app/packages/looker-3d/src/hooks/use-fo-loaders.ts (1)
useFoLoader(14-39)
app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx (7)
app/packages/looker-3d/src/StatusBar.tsx (1)
StatusTunnel(146-146)app/packages/looker-3d/src/fo3d/scene-controls/SceneControls.tsx (1)
SceneControls(15-139)app/packages/looker-3d/src/fo3d/Gizmos.tsx (1)
Gizmos(83-183)app/packages/looker-3d/src/fo3d/FoScene.tsx (1)
FoSceneComponent(248-299)app/packages/looker-3d/src/labels/index.tsx (1)
ThreeDLabels(48-399)app/packages/looker-3d/src/annotation/AnnotationPlane.tsx (1)
AnnotationPlane(21-347)app/packages/looker-3d/src/annotation/SegmentPolylineRenderer.tsx (1)
SegmentPolylineRenderer(34-573)
app/packages/looker-3d/src/fo3d/mesh/Stl.tsx (2)
app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)app/packages/looker-3d/src/hooks/use-fo-loaders.ts (1)
useFoLoader(14-39)
app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx (3)
app/packages/looker-3d/src/state.ts (6)
polylinePointTransformsAtom(285-290)editSegmentsModeAtom(331-334)hoveredLabelAtom(215-218)hoveredPolylineInfoAtom(220-223)polylineEffectivePointsAtom(292-297)tempLabelTransformsAtom(341-350)app/packages/looker-3d/src/annotation/utils/polyline-utils.ts (5)
applyTransformsToPolyline(182-208)updateDuplicateVertices(268-314)applyDeltaToAllPoints(219-256)findClickedSegment(492-536)insertVertexInSegment(369-482)app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx (1)
PolylinePointMarker(31-240)
app/packages/looker-3d/src/state.ts (3)
app/packages/looker-3d/src/labels/loader.ts (1)
OverlayLabel(8-27)app/packages/looker-3d/src/annotation/types.ts (9)
HoveredPolylineInfo(28-33)TransformMode(36-36)TransformSpace(37-37)TransformedLabelData(79-84)SelectedPoint(44-48)PolylinePointTransform(87-91)SegmentPolylineState(93-98)TempPolyline(100-106)AnnotationPlaneState(108-115)app/packages/looker-3d/src/types.ts (1)
TransformArchetype(79-83)
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (9)
app/packages/state/src/recoil/groups.ts (1)
isGroup(135-140)app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)app/packages/looker-3d/src/state.ts (9)
isActivelySegmentingSelector(309-314)isSegmentingPointerDownAtom(316-319)isCurrentlyTransformingAtom(267-270)cameraPositionAtom(50-53)activeNodeAtom(204-207)currentHoveredPointAtom(209-212)clearTransformStateSelector(402-419)selectedPolylineVertexAtom(280-283)annotationPlaneAtom(363-379)app/packages/looker-3d/src/hooks/use-bounds.ts (1)
useFo3dBounds(31-154)app/packages/looker-3d/src/constants.ts (2)
DEFAULT_BOUNDING_BOX(27-32)DEFAULT_CAMERA_POSITION(24-24)app/packages/looker-3d/src/fo3d/utils.ts (1)
getCameraPositionKey(24-25)app/packages/looker-3d/src/annotation/MultiPanelView.tsx (1)
MultiPanelView(189-306)app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx (1)
Fo3dSceneContent(89-158)app/packages/looker-3d/src/annotation/annotation-toolbar/AnnotationToolbar.tsx (1)
AnnotationToolbar(130-254)
🪛 Gitleaks (8.28.0)
app/packages/looker-3d/src/state.ts
[high] 382-382: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 392-392: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
🔇 Additional comments (8)
app/packages/looker-3d/src/utils.ts (4)
232-247: LGTM!The NDC conversion formulas are correct and follow the standard WebGL convention with y-axis inverted.
283-292: LGTM!Simple and correct button matching implementation with clear documentation.
294-316: LGTM!The implementation correctly extracts the plane normal by rotating the Z-axis with the quaternion and creates the plane using Three.js's standard method. The approach is well-documented and logically sound.
318-343: LGTM!Well-implemented bounding box expansion with good defensive programming (empty box check), clear logic, and proper immutability (returns new box).
app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx (1)
45-52: Double-check useGLTF argument positions vs installed drei versionUsing four params assumes signature (url, useDraco, useMeshOpt, extendLoader). Please confirm your drei version matches this.
app/packages/looker-3d/src/labels/polyline.tsx (1)
119-129: LGTM: geometry-only disposal avoids shared material invalidationGood fix to dispose geometries while keeping the shared fill material alive. This prevents the “vanishing fill after first edit” issue noted earlier.
app/packages/looker-3d/src/annotation/MultiPanelView.tsx (1)
404-411: LGTM: bounds observe handoffObserving Bounds briefly then disabling is a nice UX to stabilize cameras without fighting user input.
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1)
273-279: LGTM: camera controls react to transform/segment statesThe effect that reapplies controls config on state changes eliminates latency and prevents accidental camera moves during transforms.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx (1)
73-81: Prevent shared-asset disposal across panels: add dispose={null}Without this, when one panel unmounts R3F may dispose geometries/materials still referenced by the other panel (clones share resources), causing render breakage.
- <primitive - object={fbx} + <primitive + dispose={null} + object={fbx} position={position} quaternion={quaternion} scale={scale} >
♻️ Duplicate comments (4)
app/packages/looker-3d/src/hooks/use-bounds.ts (1)
61-63: ResetretryCounton successful recompute to avoid permanent short‑circuitIf retries previously hit the cap, future polls can short-circuit even after a successful manual recompute. Reset on success here as well.
- setBoundingBox(box); + retryCount.current = 0; + setBoundingBox(box); setIsComputing(false);app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx (1)
55-63: Good fix: SkeletonUtils.clone for multi-panel scenesCorrectly avoids de-binding skeletons/animations vs Object3D.clone. Thanks for addressing the prior feedback.
app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx (1)
53-59: Correct: SkeletonUtils.clone for FBXPreserves bones/animations across panels.
app/packages/looker-3d/src/fo3d/mesh/Obj.tsx (1)
73-75: LGTM: null guards before renderingPrevents crashes while loaders resolve.
Also applies to: 141-144
🧹 Nitpick comments (14)
app/packages/looker-3d/src/hooks/use-bounds.ts (5)
73-75: Use tolerance when comparing boxes to avoid float jitterExact equality on Vector3 can flap; use a small epsilon.
- const boxesAreEqual = (box1: Box3, box2: Box3) => { - return box1.min.equals(box2.min) && box1.max.equals(box2.max); - }; + const BOUNDS_EPSILON = 1e-6; + const boxesAreNearEqual = (box1: Box3, box2: Box3) => { + return ( + box1.min.distanceTo(box2.min) <= BOUNDS_EPSILON && + box1.max.distanceTo(box2.max) <= BOUNDS_EPSILON + ); + };Also update usages:
- if (previousBox.current && boxesAreEqual(box, previousBox.current)) { + if (previousBox.current && boxesAreNearEqual(box, previousBox.current)) {
145-154: Reset stability trackers in cleanup to avoid stale state across effect re-runsWhen dependencies change, leftover
unchangedCount/previousBoxcan skew the next cycle. Reset them in cleanup.return () => { isMounted = false; - retryCount.current = 0; + retryCount.current = 0; + unchangedCount.current = 0; + previousBox.current = null; setIsComputing(false);
84-91: De-duplicate bailout logic into a small helperSame bailout block appears twice; extract a local
bail()to reduce duplication and future drift.+ const bail = () => { + retryCount.current = 0; + unchangedCount.current = 0; + previousBox.current = null; + setBoundingBox(null); + setIsComputing(false); + }; ... - if (retryCount.current >= MAX_BOUNDING_BOX_RETRIES) { - retryCount.current = 0; - unchangedCount.current = 0; - previousBox.current = null; - setBoundingBox(null); - setIsComputing(false); - return; - } + if (retryCount.current >= MAX_BOUNDING_BOX_RETRIES) { + bail(); + return; + } ... - if (retryCount.current >= MAX_BOUNDING_BOX_RETRIES) { - retryCount.current = 0; - unchangedCount.current = 0; - previousBox.current = null; - setBoundingBox(null); - setIsComputing(false); - return; - } + if (retryCount.current >= MAX_BOUNDING_BOX_RETRIES) { + bail(); + return; + }Also applies to: 103-110
65-69: ConsideruseEffectinstead ofuseLayoutEffectunless you need pre-paint guaranteesThe effect doesn’t read layout/DOM;
useLayoutEffectcan block paint. If no synchronous pre-paint work is required, preferuseEffect.Would switching to
useEffectkeep camera/bounds behavior acceptable in your scenarios?
1-1: Memoize the returned object to stabilize referential identitySome consumers may rely on referential equality; memoizing avoids needless re-renders. Low cost.
-import { useCallback, useLayoutEffect, useRef, useState } from "react"; +import { useCallback, useLayoutEffect, useRef, useState, useMemo } from "react"; ... - return { boundingBox, recomputeBounds, isComputing }; + const result = useMemo( + () => ({ boundingBox, recomputeBounds, isComputing }), + [boundingBox, recomputeBounds, isComputing] + ); + return result;Also applies to: 157-157
app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx (2)
69-71: Guard AnimationMixer creation and dispose itInstantiate the mixer only when scene is ready and stop actions on unmount to avoid runtime risks and leaks.
Apply:
- const mixer = useMemo(() => new AnimationMixer(scene), [scene]); + const mixer = useMemo(() => (scene ? new AnimationMixer(scene) : null), [scene]); + useEffect(() => { + return () => { + mixer?.stopAllAction(); + }; + }, [mixer]);Ensure useAnimationSelect tolerates a null mixer (noop internally). If not, adapt it to accept AnimationMixer | null. As per coding guidelines.
67-67: Type the refAdd an explicit type to avoid any-refs.
- const groupRef = useRef(); + const groupRef = useRef<THREE.Object3D | null>(null);As per coding guidelines.
app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx (1)
67-68: Guard AnimationMixer creation and clean upAvoid constructing with undefined and stop actions on unmount.
- const mixer = useMemo(() => new AnimationMixer(fbx), [fbx]); + const mixer = useMemo(() => (fbx ? new AnimationMixer(fbx) : null), [fbx]); + useEffect(() => () => mixer?.stopAllAction(), [mixer]);Ensure downstream logic handles a null mixer. As per coding guidelines.
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (2)
737-743: Include datasetName in restore-camera effect depsEnsures per-dataset camera restore runs after dataset switches, even if foScene identity doesn’t change.
- }, [ - foScene, - onChangeView, - cameraControlsRef, - cameraRef, - isComputingSceneBoundingBox, - ]); + }, [ + foScene, + onChangeView, + cameraControlsRef, + cameraRef, + isComputingSceneBoundingBox, + datasetName, + ]);
287-294: Reset document cursor on unmountSegment hover toggles body cursor; add a cleanup so the cursor returns to default if the component unmounts while active.
+ useEffect(() => { + return () => { + document.body.style.cursor = ""; + }; + }, []);Also applies to: 304-312
app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx (3)
109-117: Normalize dedupe key to resist float jitterExact stringifying of floats can produce duplicate markers for the “same” vertex due to minor rounding. Normalize the coordinates before keying.
- const visitedPoints = new Set<string>(); + const visitedPoints = new Set<string>(); + const dedupeKey = (p: Vector3Tuple) => + [p[0], p[1], p[2]].map((n) => Number(n.toFixed(6))).join("|"); @@ - const key = `${point[0]}-${point[1]}-${point[2]}`; + const key = dedupeKey(point);Also applies to: 120-127
227-267: Transform semantics: ensure translate-only or handle rotations/scaleshandleTransformEnd extracts only positional delta; rotations/scales from the deltaMatrix are ignored, then group position is reset. Either constrain TransformControls to “translate” for polylines or extend logic to apply rotation/scale to points.
Would you like a follow-up patch to lock TransformControls mode for this archetype?
56-59: Type the refsAdd explicit types for better safety and IntelliSense.
- const transformControlsRef = useRef(null); - const contentRef = useRef<THREE.Group>(null); + const transformControlsRef = useRef<import("three-stdlib").TransformControls | null>(null); + const contentRef = useRef<THREE.Group | null>(null);As per coding guidelines.
app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx (1)
104-201: Consider reordering variable declarations for better readability.The callbacks at lines 104-152 reference
groupRef(declared at line 197) andsetTempVertexTransforms(declared at line 199) before their declarations. While this works correctly in JavaScript due to closure behavior and execution timing, it makes the code harder to follow.Consider moving the declarations of
groupRefandtempVertexTransforms/setTempVertexTransformsto before the callbacks that use them (around line 60) for improved code clarity:const setSegmentPolylineState = useSetRecoilState(segmentPolylineStateAtom); const setEditSegmentsMode = useSetRecoilState(editSegmentsModeAtom); + const groupRef = useRef(null); + + const [tempVertexTransforms, setTempVertexTransforms] = useRecoilState( + tempVertexTransformsAtom(`${labelId}-${segmentIndex}-${pointIndex}`) + ); + const isSelected = selectedPoint?.labelId === labelId && selectedPoint?.segmentIndex === segmentIndex &&Then remove the declarations from their current locations at lines 197 and 199-201.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
e2e-pw/src/oss/specs/3d/fo3d-ply.spec.ts-snapshots/ply-scene-top-view-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/3d/fo3d-ply.spec.ts-snapshots/ply-scene-top-view-chromium-linux.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (8)
app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx(1 hunks)app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx(1 hunks)app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(21 hunks)app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx(3 hunks)app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx(3 hunks)app/packages/looker-3d/src/fo3d/mesh/Obj.tsx(7 hunks)app/packages/looker-3d/src/hooks/use-bounds.ts(6 hunks)app/packages/looker-3d/src/labels/polyline.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/looker-3d/src/fo3d/mesh/Gltf.tsxapp/packages/looker-3d/src/hooks/use-bounds.tsapp/packages/looker-3d/src/fo3d/mesh/Fbx.tsxapp/packages/looker-3d/src/fo3d/mesh/Obj.tsxapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsxapp/packages/looker-3d/src/labels/polyline.tsxapp/packages/looker-3d/src/annotation/PolylinePointMarker.tsxapp/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx
🧬 Code graph analysis (7)
app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx (1)
app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)
app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx (2)
app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)app/packages/looker-3d/src/hooks/use-fo-loaders.ts (1)
useFoLoader(14-39)
app/packages/looker-3d/src/fo3d/mesh/Obj.tsx (2)
app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)app/packages/looker-3d/src/hooks/use-fo-loaders.ts (1)
useFoLoader(14-39)
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (10)
app/packages/state/src/recoil/groups.ts (1)
isGroup(135-140)app/packages/state/src/recoil/atoms.ts (1)
isInMultiPanelViewAtom(411-414)app/packages/looker-3d/src/state.ts (9)
isActivelySegmentingSelector(309-314)isSegmentingPointerDownAtom(316-319)isCurrentlyTransformingAtom(267-270)cameraPositionAtom(50-53)activeNodeAtom(204-207)currentHoveredPointAtom(209-212)clearTransformStateSelector(402-419)selectedPolylineVertexAtom(280-283)annotationPlaneAtom(363-379)app/packages/looker-3d/src/hooks/use-bounds.ts (1)
useFo3dBounds(31-158)app/packages/looker-3d/src/constants.ts (2)
DEFAULT_BOUNDING_BOX(27-32)DEFAULT_CAMERA_POSITION(24-24)app/packages/looker-3d/src/fo3d/utils.ts (1)
getCameraPositionKey(24-25)app/packages/looker-3d/src/annotation/MultiPanelView.tsx (1)
MultiPanelView(189-306)app/packages/looker-3d/src/components/PcdColormapModal.tsx (1)
PcdColorMapTunnel(37-37)app/packages/looker-3d/src/fo3d/Fo3dCanvas.tsx (1)
Fo3dSceneContent(89-158)app/packages/looker-3d/src/annotation/annotation-toolbar/AnnotationToolbar.tsx (1)
AnnotationToolbar(130-254)
app/packages/looker-3d/src/labels/polyline.tsx (6)
app/packages/looker-3d/src/labels/shared.tsx (1)
OverlayProps(3-5)app/packages/looker-3d/src/labels/shared/hooks.ts (3)
useHoverState(12-27)useEventHandlers(32-44)useLabelColor(49-69)app/packages/looker-3d/src/state.ts (2)
selectedLabelForAnnotationAtom(245-248)tempLabelTransformsAtom(341-350)app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx (1)
usePolylineAnnotation(32-411)app/packages/looker-3d/src/labels/polygon-fill-utils.ts (1)
createFilledPolygonMeshes(303-326)app/packages/looker-3d/src/labels/shared/TransformControls.tsx (1)
Transformable(38-115)
app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx (4)
app/packages/looker-3d/src/state.ts (7)
hoveredPolylineInfoAtom(220-223)transformModeAtom(250-253)selectedPolylineVertexAtom(280-283)currentArchetypeSelectedForTransformAtom(260-264)segmentPolylineStateAtom(299-307)editSegmentsModeAtom(331-334)tempVertexTransformsAtom(352-361)app/packages/looker-3d/src/annotation/types.ts (1)
SelectedPoint(44-48)app/packages/looker-3d/src/labels/shared/TransformControls.tsx (1)
Transformable(38-115)app/packages/looker-3d/src/constants.ts (1)
LABEL_3D_ANNOTATION_POINT_SELECTED_FOR_TRANSFORMATION_COLOR(94-95)
app/packages/looker-3d/src/annotation/usePolylineAnnotation.tsx (3)
app/packages/looker-3d/src/state.ts (6)
polylinePointTransformsAtom(285-290)editSegmentsModeAtom(331-334)hoveredLabelAtom(215-218)hoveredPolylineInfoAtom(220-223)polylineEffectivePointsAtom(292-297)tempLabelTransformsAtom(341-350)app/packages/looker-3d/src/annotation/utils/polyline-utils.ts (5)
applyTransformsToPolyline(182-208)updateDuplicateVertices(268-314)applyDeltaToAllPoints(219-256)findClickedSegment(492-536)insertVertexInSegment(369-482)app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx (1)
PolylinePointMarker(31-260)
⏰ 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). (7)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
🔇 Additional comments (10)
app/packages/looker-3d/src/hooks/use-bounds.ts (2)
12-21: Finite-value guard looks solidCovers all min/max components; this closes the Infinity/NaN gaps. Nice.
39-39: Review comment is incorrect: strict null checks are disabledThe review comment claims
useRef<Box3>(null)"is invalid under strict null checks and will fail type-checking," but the looker-3d package has"strict": falsein its tsconfig.json, which disables strict null checks. The code will not fail type-checking under the current configuration.While
useRef<Box3 | null>(null)would be a better self-documenting type annotation given the actual usage pattern (where previousBox is assigned both null and Box3 values), the current code is valid and will compile without errors.Likely an incorrect or invalid review comment.
app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1)
249-271: Solid: camera controls gating during transforms/segmentingDisabling input while editing prevents accidental camera moves; the update effect keeps bindings in sync.
Also applies to: 273-279
app/packages/looker-3d/src/labels/polyline.tsx (1)
120-130: LGTM: safe resource cleanup for filled meshesGeometry-only disposal avoids breaking shared materials; material is disposed in a dedicated cleanup. Solid lifecycle management.
Also applies to: 142-150, 152-159
app/packages/looker-3d/src/annotation/PolylinePointMarker.tsx (6)
1-17: LGTM! Imports are well-organized.All necessary dependencies are imported correctly from React, Recoil, Three.js, and local modules.
19-29: LGTM! Props interface is well-defined.The interface clearly defines all necessary props with appropriate types for the polyline point marker functionality.
171-195: LGTM! Distance-based scaling and transform controls update logic is sound.The frame-by-frame updates correctly handle:
- Distance-based scaling with reasonable bounds
- Optional pulsate animation
- Transform controls positioning relative to the group
220-227: Verify the nested position transform produces correct visual behavior.The component uses a layered transform structure:
- Line 218:
transformControlsPosition={position.toArray()}positions the controls- Line 222:
position={tempVertexTransforms?.position}on the parent group for temporary drag transform- Line 227:
position={position}on the mesh itselfThis pattern appears intentional for computing transform deltas in
handleTransformEnd(lines 124-139), where the delta is extracted and applied back to the data. However, the interaction between these three position values is complex.Please verify that:
- The marker renders at the correct position in both normal and dragging states
- The transform controls attach to the correct location
- The visual position matches the logical position throughout the interaction
243-255: LGTM! Material properties create effective visual feedback.The material correctly uses color and emissive properties to highlight selected points, with appropriate intensity values for clear visual distinction.
154-168: Verify that overlapping Escape key conditions are handled correctly.The codebase has multiple Escape key handlers in the annotation system:
- TransformControls.tsx (parent): Clears
isCurrentlyTransformingwhen Escape pressed- PolylinePointMarker.tsx (child): Clears
selectedPointwhen Escape pressed- AnnotationPlane.tsx (child): Clears selection when Escape pressed
- SegmentPolylineRenderer.tsx: Cancels active segmentation when Escape pressed
All call
stopImmediatePropagation(), which means whichever handler fires first prevents the others from executing. Since these components can have overlapping conditions—for example, bothselectedPointandisCurrentlyTransformingcan be true simultaneously—the first-registered handler will prevent the others from clearing their state.Verify:
- Whether having both
selectedPointandisCurrentlyTransformingtrue at the same time is an expected scenario- Whether the current order of event listener registration produces the intended behavior
- Whether handlers should coordinate to clear both states, or if the current "first-wins" behavior is acceptable
1 participant
Overview
This PR introduces 3D polyline annotation capabilities to FiftyOne.
Key Features
Implementation Details
SegmentPolylineRenderer: Core polyline rendering and interaction logicPolylinePointMarker: Individual vertex markers with transform controlsMultiPanelView: Multi-camera view support for annotation workflowsAnnotationPlane: Z-drift protection and plane-based editingAnnotationToolbar: Annotation controlspolyline-utils.ts: Core vertex and segment editing / merging logicTesting
Unit tests for dense logics like vertex manipulation and segment merging
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests