improve 3d bbox projection logic #4420
Conversation
|
Important Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes introduce support for 3D datasets and projections in the Looker package. Key updates include adding a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
fiftyone/utils/utils3d.py (1)
904-904: Clarify default behavior fornormalparameter.The assignment
normal=projection_normal or [0, 0, 1]uses a logical OR to provide a default value for thenormalparameter. While this is a common pattern in Python, it might be clearer to use an explicit check likeif projection_normal is Nonefor setting default values, especially for readability and explicitness in critical parameters like this.fiftyone/zoo/datasets/base.py (1)
3209-3219: Ensure properties are documented for clarity.While the properties
name,tags, andsupported_splitsare implemented correctly, adding a brief docstring to each can enhance the readability and maintainability of the code. This helps new developers understand the purpose of these properties at a glance.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
app/packages/looker/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/*.lock,!**/*.lock
Files selected for processing (9)
- app/packages/looker/src/overlays/detection.ts (2 hunks)
- app/packages/looker/src/state.ts (1 hunks)
- app/packages/looker/src/worker/label-3d-projection-utils.test.ts (1 hunks)
- app/packages/looker/src/worker/label-3d-projection-utils.ts (1 hunks)
- app/packages/looker/src/worker/threed-label-processor.ts (6 hunks)
- docs/source/user_guide/dataset_zoo/datasets.rst (1 hunks)
- fiftyone/utils/utils3d.py (3 hunks)
- fiftyone/zoo/datasets/base.py (2 hunks)
- tests/unittests/utils3d_tests.py (2 hunks)
Files skipped from review due to trivial changes (1)
- docs/source/user_guide/dataset_zoo/datasets.rst
Additional Context Used
Ruff (6)
fiftyone/zoo/datasets/base.py (6)
636-636: Do not use bare
except
643-643: Do not use bare
except
3359-3359: Do not use bare
except
3367-3367: Do not use bare
except
3375-3375: Do not use bare
except
3382-3382: Do not use bare
except
Biome (37)
app/packages/looker/src/overlays/detection.ts (8)
196-196: The assignment should not be in an expression.
205-205: The assignment should not be in an expression.
293-302: Prefer for...of instead of forEach.
8-8: All these imports are only used as types.
9-9: All these imports are only used as types.
11-11: Some named imports are only used as types.
195-195: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
202-202: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
app/packages/looker/src/state.ts (10)
121-121: Unexpected any. Specify a different type.
130-130: This enum declaration contains members that are implicitly initialized.
188-188: Unexpected any. Specify a different type.
323-323: Unexpected any. Specify a different type.
412-412: Unexpected any. Specify a different type.
494-494: Unexpected any. Specify a different type.
6-6: All these imports are only used as types.
7-7: All these imports are only used as types.
8-8: All these imports are only used as types.
10-10: All these imports are only used as types.
app/packages/looker/src/worker/label-3d-projection-utils.test.ts (1)
3-3: Some named imports are only used as types.
app/packages/looker/src/worker/threed-label-processor.ts (18)
220-220: Declare variables separately
220-220: This variable implicitly has the any type.
274-276: Prefer for...of instead of forEach.
2-2: Some named imports are only used as types.
5-5: All these imports are only used as types.
6-6: All these imports are only used as types.
7-10: Some named imports are only used as types.
110-110: Use Number.Infinity instead of the equivalent global.
111-111: Use Number.Infinity instead of the equivalent global.
112-112: Use Number.Infinity instead of the equivalent global.
113-113: Use Number.Infinity instead of the equivalent global.
143-143: Use Number.Infinity instead of the equivalent global.
144-144: Use Number.Infinity instead of the equivalent global.
147-147: Use Number.Infinity instead of the equivalent global.
150-150: Use Number.Infinity instead of the equivalent global.
156-156: Use Number.Infinity instead of the equivalent global.
159-159: Use Number.Infinity instead of the equivalent global.
Import statements could be sorted:
Path-based Instructions (5)
app/packages/looker/src/worker/label-3d-projection-utils.test.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/looker/src/worker/label-3d-projection-utils.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/looker/src/overlays/detection.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/looker/src/worker/threed-label-processor.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/looker/src/state.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (14)
app/packages/looker/src/worker/label-3d-projection-utils.test.ts (1)
4-56: The test suite forrotatePointis well-structured and covers rotations around different axes effectively.app/packages/looker/src/worker/label-3d-projection-utils.ts (5)
1-2: The type definitions forVec3andVec2are correctly specified.
4-15: The type definitions forBoundingBox3DandBoundingBox2Dare well-documented and correctly structured.
17-44: TherotatePointfunction correctly implements the 3D rotation using rotation matrices. Good use of mathematical functions for clarity.
46-55: TheprojectTo2Dfunction is well-implemented with clear handling of different projection planes.
57-103: ThegetProjectedCornersfunction correctly computes the 2D projections of the corners of a 3D bounding box, following the appropriate transformations and projections.app/packages/looker/src/overlays/detection.ts (2)
24-24: The addition of theconvexHullproperty to theDetectionLabelinterface is correctly typed and well-documented.
233-254: ThefillRectFor3dmethod in theDetectionOverlayclass effectively uses the convex hull to render the 3D bounding box. Good handling of canvas state changes.app/packages/looker/src/worker/threed-label-processor.ts (2)
Line range hint
34-86: ThegetScalingFactorForLabelfunction is well-implemented with efficient use of caching and correct logic for determining scaling factors based on the projection plane.
183-243: ThePainterFactory3Dfunction correctly handles the projection and drawing of 3D labels, using the convex hull to determine the outline of the projected label.app/packages/looker/src/state.ts (1)
68-70: The updates to theOrthographicProjectionMetadatatype are correctly implemented, adding necessary fields for 3D projection handling.tests/unittests/utils3d_tests.py (2)
288-288: Ensure thenormalattribute is correctly initialized and used.This change aligns with the PR's objective to enhance 3D data handling by including a normal vector in the projection metadata, which is crucial for correct 3D to 2D projection.
308-308: Validate thenormalfield in unit tests.The addition of the
normalfield to theOrthographicProjectionMetadataclass is correctly tested here, ensuring that the field is properly serialized and deserialized. This is crucial for maintaining data integrity across operations.fiftyone/zoo/datasets/base.py (1)
3221-3237: Refactor exception handling in_download_and_preparemethod to specify exception types.- except: + except Exception as e: + logger.error("An error occurred: %s", str(e))Using a bare
except:clause can catch unexpected events such as keyboard interrupts, and system exits, besides the regular exceptions. This is generally not advisable unless you have a specific reason to catch all exceptions. Specifying the exception type can help in handling only those exceptions that are expected and relevant to the context.Likely invalid or redundant comment.
What changes are proposed in this pull request?
Project 3d bounding box to 2d plane, and fill the convex hull of the resulting point set. Still needs refinement.
Summary by CodeRabbit
New Features
Quickstart3DDatasetclass for handling 3D datasets with meshes, point clouds, and oriented bounding boxes.Enhancements
normalfield.Bug Fixes
Documentation