Feature/establish bounding box #6423
Conversation
WalkthroughAdded overlay-aware interaction support and a SETTING move state for bounding boxes; introduced BaseOverlay.validBounds and guards in Scene2D for relative-bounds updates and coordinate-update signaling. Minor formatting changes in annotate hooks and a guard on overlay selection assignment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Pointer
participant IM as InteractionManager
participant O as BoundingBoxOverlay
participant S as Scene2D
note over IM,O #dbeafe: Overlay-aware interactions & SETTING state
U->>IM: pointerdown / move / up
alt overlay present
IM->>O: dispatch pointer events
O->>O: enter SETTING / DRAGGING / RESIZE states
O->>O: update absoluteBounds (guard with BaseOverlay.validBounds)
alt bounds valid
O->>S: request relative bounds update
S->>S: compute relative bounds
S->>O: apply relative bounds
S->>O: markCoordinateUpdateComplete()
else bounds invalid
O-->>IM: skip applying relative bounds
end
opt drag end with overlay
IM->>IM: swap overlay back into handler list
end
else
IM->>IM: fallback to non-overlay handler flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/packages/lighter/src/overlay/BaseOverlay.ts (1)
36-40: Consider usingisFinitefor explicit NaN/Infinity checks.While
bounds[prop] >= 0correctly rejects NaN (sinceNaN >= 0is false), usingNumber.isFinite()would make the intent clearer and also catchInfinityvalues.static validBounds(bounds: Rect): boolean { return ["x", "y", "width", "height"].every( - (prop) => typeof bounds[prop] === "number" && bounds[prop] >= 0 + (prop) => Number.isFinite(bounds[prop]) && bounds[prop] >= 0 ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/useLabels.ts(1 hunks)app/packages/lighter/src/core/Scene2D.ts(2 hunks)app/packages/lighter/src/interaction/InteractionManager.ts(6 hunks)app/packages/lighter/src/overlay/BaseOverlay.ts(2 hunks)app/packages/lighter/src/overlay/BoundingBoxOverlay.ts(14 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/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsxapp/packages/lighter/src/core/Scene2D.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/useLabels.tsapp/packages/lighter/src/overlay/BaseOverlay.tsapp/packages/lighter/src/interaction/InteractionManager.tsapp/packages/lighter/src/overlay/BoundingBoxOverlay.ts
🔇 Additional comments (8)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (1)
123-136: LGTM! Formatting update with no functional changes.The reformatting of the
useHandleChangeshook to a more compact arrow function style maintains the same logic, error handling, and type conversions. The change improves code consistency across the codebase.app/packages/core/src/components/Modal/Sidebar/Annotate/useLabels.ts (1)
117-131: LGTM! Formatting update with no functional changes.The reformatting of the
getFieldTypecallback to a more compact arrow function style maintains identical logic, error handling, and type assertions. This change aligns with the formatting updates in AnnotationSchema.tsx for code consistency.app/packages/lighter/src/core/Scene2D.ts (1)
1440-1447: LGTM! Defensive bounds validation added.The validity check prevents invalid bounds (NaN, negative values) from being propagated through the coordinate system. The placement of
markCoordinateUpdateComplete()inside the validity check ensures coordinate updates are only marked complete when bounds are actually valid.app/packages/lighter/src/interaction/InteractionManager.ts (1)
226-227: LGTM! Overlay-aware interaction flow implemented correctly.The changes enable overlays to be used during interactive drawing/setting workflows. The pattern is consistent:
- Prefer
interactiveHandler.overlaywhen present- Fall back to
interactiveHandlerotherwise- After pointer up, swap the interactive handler with the overlay to transition from drawing mode to regular interaction mode
This design cleanly separates the interactive creation workflow from normal overlay interactions.
Also applies to: 284-293, 327-346
app/packages/lighter/src/overlay/BoundingBoxOverlay.ts (4)
57-57: LGTM! SETTING state enables initial bounds establishment.The addition of the SETTING state,
settingBoundsflag, and NaN initialization work together to support a workflow where bounding box bounds are established during user interaction. Initializing with NaN appropriately marks the bounds as initially invalid until set by the user.Also applies to: 75-75, 89-89
103-107: Bounds validation added to setters.Both
setAbsoluteBoundsandsetRelativeBoundsnow validate bounds before applying them. This defensive approach prevents invalid bounds (NaN, negative values) from being stored, ensuring state consistency throughout the overlay lifecycle.Also applies to: 111-115
463-465: LGTM! Division-by-zero prevention added.The fallback to
aspectRatio = 1whenmoveStartBoundshas zero or undefined dimensions prevents division-by-zero errors during resize operations. This is particularly important when transitioning from the SETTING state where bounds may be initially invalid.
295-297: LGTM! SETTING state fully integrated into interaction lifecycle.The SETTING state is comprehensively handled throughout the interaction flow:
- Cursor changes to
crosshairwhen bounds are invalidonPointerDowninitializes position and sets dimensions to 0onMoveandonResizetreat SETTING as a resize operationsettingBoundsflag is properly reset on pointer uphasValidBounds()provides a clean public API for validity checksThe implementation enables a smooth workflow for establishing bounding boxes through user interaction.
Also applies to: 344-344, 379-394, 417-417, 492-509, 565-565, 586-592
| */ | ||
| export abstract class BaseOverlay implements InteractionHandler { | ||
| readonly id: string; | ||
| readonly field: string; |
There was a problem hiding this comment.
Duplicate field declaration.
Line 18 declares readonly field: string but Line 33 still declares protected field: string. TypeScript will reject this duplicate declaration.
Apply this diff to remove the duplicate:
protected eventBus?: EventBus;
protected resourceLoader?: ResourceLoader;
protected currentStyle?: DrawStyle;
- protected field: string;
protected label: RawLookerLabel;Also applies to: 33-33
🤖 Prompt for AI Agents
In app/packages/lighter/src/overlay/BaseOverlay.ts around lines 18 and 33,
there's a duplicate declaration of `field` (`readonly field: string` at line 18
and `protected field: string` at line 33); remove the duplicate at line 18 so
only the `protected field: string` declaration remains (or alternatively make
both consistent by replacing line 33 with `protected readonly field: string` if
immutability is intended), ensuring a single declaration to satisfy TypeScript.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/core/src/components/Modal/Sidebar/Annotate/useFocus.ts (1)
14-17: Good defensive guard added.The guard prevents setting
editingto an undefined value, which is a solid defensive programming practice.Consider whether the case where a label is not found represents an error condition. If overlay IDs can legitimately be out of sync with
labelMap, the current silent handling is appropriate. However, if this indicates a bug, you might want to add development-mode logging or explicitly clear theeditingstate to avoid leaving stale values.Example with logging:
const label = store.get(labelMap)[event.detail.id]; if (label) { store.set(editing, label); +} else if (process.env.NODE_ENV === 'development') { + console.warn(`Label not found for overlay ID: ${event.detail.id}`); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/core/src/components/Modal/Sidebar/Annotate/useFocus.ts(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/core/src/components/Modal/Sidebar/Annotate/useFocus.ts
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 (1)
app/packages/lighter/src/renderer/PixiRenderer2D.ts(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/lighter/src/renderer/PixiRenderer2D.ts
| const x = Math.max(bounds.x - halfWidth, 0); | ||
| const y = Math.max(bounds.y - halfWidth, 0); | ||
| const w = | ||
| Math.min(bounds.width + borderWidth, sceneDimensions.width - bounds.x) + | ||
| Math.min(bounds.x, 0); | ||
| const h = | ||
| Math.min(bounds.height + borderWidth, sceneDimensions.height - bounds.y) + | ||
| Math.min(bounds.y, 0); | ||
|
|
||
| mask.rect(x, y, w, h); |
There was a problem hiding this comment.
Incorrect width/height calculation for scrim cutout.
The formulas for w and h contain logic errors that produce incorrect dimensions when the bounding box extends beyond scene boundaries.
Issue Analysis:
When bounds.x < 0 (left edge outside scene):
- Current formula:
w = Math.min(110, 210) + (-10) = 100 - Expected:
95(the visible portion from x=0 to x=95)
When the bounding box extends past the right edge:
- Current formula:
w = Math.min(110, 50) + 0 = 50 - Expected:
55(from x=145 to x=200)
The issue is that the formula attempts to compute width from bounds.x rather than from the clamped x coordinate, and the + Math.min(bounds.x, 0) adjustment doesn't fully compensate.
Apply this diff to fix the calculation:
const halfWidth = borderWidth / 2;
const x = Math.max(bounds.x - halfWidth, 0);
const y = Math.max(bounds.y - halfWidth, 0);
-const w =
- Math.min(bounds.width + borderWidth, sceneDimensions.width - bounds.x) +
- Math.min(bounds.x, 0);
-const h =
- Math.min(bounds.height + borderWidth, sceneDimensions.height - bounds.y) +
- Math.min(bounds.y, 0);
+const w = Math.min(bounds.x + bounds.width + halfWidth, sceneDimensions.width) - x;
+const h = Math.min(bounds.y + bounds.height + halfWidth, sceneDimensions.height) - y;
mask.rect(x, y, w, h);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const x = Math.max(bounds.x - halfWidth, 0); | |
| const y = Math.max(bounds.y - halfWidth, 0); | |
| const w = | |
| Math.min(bounds.width + borderWidth, sceneDimensions.width - bounds.x) + | |
| Math.min(bounds.x, 0); | |
| const h = | |
| Math.min(bounds.height + borderWidth, sceneDimensions.height - bounds.y) + | |
| Math.min(bounds.y, 0); | |
| mask.rect(x, y, w, h); | |
| const halfWidth = borderWidth / 2; | |
| const x = Math.max(bounds.x - halfWidth, 0); | |
| const y = Math.max(bounds.y - halfWidth, 0); | |
| const w = Math.min(bounds.x + bounds.width + halfWidth, sceneDimensions.width) - x; | |
| const h = Math.min(bounds.y + bounds.height + halfWidth, sceneDimensions.height) - y; | |
| mask.rect(x, y, w, h); |
🤖 Prompt for AI Agents
In app/packages/lighter/src/renderer/PixiRenderer2D.ts around lines 207 to 216,
the width/height for the scrim cutout are computed from the original bounds.x/y
causing incorrect sizes when the box is outside scene bounds; fix by computing
clamped left/top (x,y) as you already do, then compute clamped right =
Math.min(bounds.x + bounds.width + borderWidth, sceneDimensions.width) and
bottom = Math.min(bounds.y + bounds.height + borderWidth,
sceneDimensions.height), and set w = Math.max(0, right - x) and h = Math.max(0,
bottom - y) so the cutout dimensions are derived from the clamped coordinates
and never negative.
2 participants
What changes are proposed in this pull request?
(Please fill in changes proposed in this fix)
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Chores