Fix 3D bounding box alignment for orthographic projections #4422
Conversation
|
Warning Rate Limit Exceeded@brimoor has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 54 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates introduce significant enhancements to the Looker package, particularly in 3D data handling. Key changes 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (1)
app/packages/looker/src/worker/threed-label-processor.ts (1)
Line range hint
218-220: Prefer usingfor...ofinstead offorEachfor better performance and readability.- label[cls.toLocaleLowerCase()].forEach((label: ThreeDLabel) => { + for (const labelItem of label[cls.toLocaleLowerCase()]) {
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 (5 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)
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 (36)
app/packages/looker/src/overlays/detection.ts (8)
195-195: The assignment should not be in an expression.
204-204: The assignment should not be in an expression.
292-301: Prefer for...of instead of forEach.
6-7: All these imports are only used as types.
7-8: All these imports are only used as types.
9-10: Some named imports are only used as types.
194-194: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
201-201: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
app/packages/looker/src/state.ts (10)
120-120: Unexpected any. Specify a different type.
129-129: This enum declaration contains members that are implicitly initialized.
187-187: Unexpected any. Specify a different type.
322-322: Unexpected any. Specify a different type.
411-411: Unexpected any. Specify a different type.
493-493: Unexpected any. Specify a different type.
4-5: All these imports are only used as types.
5-6: All these imports are only used as types.
6-7: All these imports are only used as types.
8-9: All these imports are only used as types.
app/packages/looker/src/worker/label-3d-projection-utils.test.ts (1)
1-2: Some named imports are only used as types.
app/packages/looker/src/worker/threed-label-processor.ts (17)
161-161: Declare variables separately
161-161: This variable implicitly has the any type.
218-220: Prefer for...of instead of forEach.
1-1: Some named imports are only used as types.
3-4: All these imports are only used as types.
4-5: All these imports are only used as types.
5-9: Some named imports are only used as types.
51-51: Use Number.Infinity instead of the equivalent global.
52-52: Use Number.Infinity instead of the equivalent global.
53-53: Use Number.Infinity instead of the equivalent global.
54-54: Use Number.Infinity instead of the equivalent global.
84-84: Use Number.Infinity instead of the equivalent global.
85-85: Use Number.Infinity instead of the equivalent global.
88-88: Use Number.Infinity instead of the equivalent global.
91-91: Use Number.Infinity instead of the equivalent global.
97-97: Use Number.Infinity instead of the equivalent global.
100-100: Use Number.Infinity instead of the equivalent global.
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/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/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/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 (8)
app/packages/looker/src/worker/label-3d-projection-utils.ts (2)
26-35: TheprojectTo2Dfunction is well-implemented for projecting points onto specified planes.
37-85: ThegetProjectedCornersfunction correctly calculates the transformed and projected corners of a 3D bounding box.app/packages/looker/src/worker/threed-label-processor.ts (1)
26-38: Theclampandremapfunctions are correctly implemented for their respective purposes.app/packages/looker/src/overlays/detection.ts (2)
24-24: AddingconvexHullas an optional property toDetectionLabelaligns with the PR's focus on 3D bounding box enhancements.
233-254: The methodfillRectFor3dcorrectly handles drawing the convex hull for 3D bounding boxes. Ensure the alpha transparency settings are as intended.Verification successful
The alpha transparency settings in the
fillRectFor3dmethod appear to be consistent with the intended behavior. The method setsctx.globalAlphatostate.options.alpha * 0.3for filling the convex hull with some transparency.
app/packages/looker/src/overlays/detection.ts: Verified the alpha transparency settings in thefillRectFor3dmethod.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the alpha transparency settings in the `fillRectFor3d` method. # Test: Search for the usage of `fillRectFor3d`. Expect: Consistent alpha settings across different uses. rg --type typescript $'fillRectFor3d'Length of output: 71
Script:
#!/bin/bash # Description: Verify the alpha transparency settings in the `fillRectFor3d` method. # Test: Search for the usage of `fillRectFor3d`. Expect: Consistent alpha settings across different uses. rg --type ts 'fillRectFor3d'Length of output: 205
Script:
#!/bin/bash # Description: Extract lines around the usage of `fillRectFor3d` to verify alpha transparency settings. # Extract lines around the usage of `fillRectFor3d` in `app/packages/looker/src/overlays/detection.ts`. rg --type ts -A 5 -B 5 'fillRectFor3d' app/packages/looker/src/overlays/detection.tsLength of output: 751
app/packages/looker/src/state.ts (1)
68-70: The updatedOrthographicProjectionMetadatatype now includes 3D bounds and a normal vector, enhancing its utility for 3D projections.tests/unittests/utils3d_tests.py (1)
Line range hint
288-308: The test casetest_orthographic_projection_metadata_fieldcorrectly asserts the fields ofOrthographicProjectionMetadata.docs/source/user_guide/dataset_zoo/datasets.rst (1)
3453-3502: The addition of the "Quickstart 3D" dataset section provides a concise overview of the dataset's contents, including 3D meshes and point clouds. The details are well-organized, offering clear information on the dataset's size, tags, and the absence of splits, which is typical for quickstart datasets. The example usage is correctly formatted and provides straightforward instructions for loading and viewing the dataset in FiftyOne, enhancing the usability of the documentation for users.
8ed2213
to
f62cf0f
Compare
|
Note: I think there will still be issues with different projection normals. Other projections should be tested before merging this. |
3b890a9
to
a2f8936
Compare
a2f8936
to
4be6314
Compare
5b351ec
to
f7f1ed4
Compare
f7f1ed4
to
2c04e79
Compare
There was a problem hiding this comment.
@ritch I tested after your latest force push and I'm not seeing any improvement over #4406.
Example 1 works on both branches, but the boxes in the grid for examples 2 and 3 are misaligned on both branches.
import fiftyone as fo
import fiftyone.zoo as foz
import fiftyone.utils.utils3d as fou3d
dataset = foz.load_zoo_dataset("quickstart-3d")
# Example 1: boxes are aligned
fou3d.compute_orthographic_projection_images(
dataset,
(-1, 512),
bounds=((-50,-50,-50), (50,50,50)),
projection_normal=(0,-1,0),
output_dir="/tmp/quickstart-3d-proj",
shading_mode="height",
)
session = fo.launch_app(dataset)
# Example 2: boxes are NOT aligned
fou3d.compute_orthographic_projection_images(
dataset,
(-1, 512),
projection_normal=(0,-1,0),
output_dir="/tmp/quickstart-3d-proj",
shading_mode="height",
)
session = fo.launch_app(dataset)
# Example 3: boxes are NOT aligned
fou3d.compute_orthographic_projection_images(
dataset,
(-1, 512),
padding=0.25,
projection_normal=(0,-1,0),
output_dir="/tmp/quickstart-3d-proj",
shading_mode="height",
)
session = fo.launch_app(dataset)| }; | ||
|
|
||
| export const getProjectionPlaneForNormal = (normal: Vec3): ProjectionPlane => { | ||
| const [nx, ny, nz] = normal ?? [0, 0, 1]; |
There was a problem hiding this comment.
FYI- I added ?? [0, 0, 1] here for backwards compatibility. OrthographicProjectionMetadata did not have a normal field in prior releases so we need to default here.
Yes I refactored the code to add more tests and its back to the way it was before (as you noticed). I think there is some issue outside of these changes. I tried a simpler set of bounds (min_z=0, max_z=20) and the bottom of the image didn't align with the bottom of the model, which makes me think there's an issue in the image creation. |
| const py = remap(y, ymin, ymax, 0, 1); | ||
| const pz = remap(z, zmin, zmax, 0, 1); | ||
|
|
||
| console.log({ z, zmin, zmax, pz }); |
To reproduce the issue use the following:
Prior to this PR you should see the following:
After the changes in this PR you should see the projected bounding boxes align properly for the same example.
Summary by CodeRabbit
New Features
Quickstart3DDatasetfor handling 3D datasets, including methods for downloading and preparing the dataset.Enhancements
DetectionLabelinterface to includeconvexHullproperty.OrthographicProjectionMetadatawith anormalfield for better 3D projection handling.Bug Fixes
normalfield.Documentation