Add 3d media type / mesh support #3985
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3985 +/- ##
===========================================
- Coverage 28.85% 27.99% -0.86%
===========================================
Files 768 814 +46
Lines 97048 101631 +4583
Branches 1120 1178 +58
===========================================
+ Hits 28004 28453 +449
- Misses 69044 73178 +4134
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
sashankaryal
force-pushed
the
feat/3d-v2
branch
3 times, most recently
from
1a5f93c
to
312d45b
Compare
sashankaryal
force-pushed
the
feat/3d-v2
branch
from
e425585
to
72c2010
Compare
sashankaryal
force-pushed
the
feat/3d-v2
branch
from
4d6830f
to
c948e08
Compare
sashankaryal
force-pushed
the
feat/3d-v2
branch
2 times, most recently
from
a2c02a5
to
4588c3e
Compare
sashankaryal
force-pushed
the
feat/3d-v2
branch
4 times, most recently
from
33fcc57
to
ba88ac3
Compare
sashankaryal
force-pushed
the
feat/3d-v2
branch
7 times, most recently
from
c614bb6
to
dc43c3f
Compare
sashankaryal
force-pushed
the
feat/3d-v2
branch
from
034bb42
to
66ccadc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files ignored due to path filters (2)
app/packages/utilities/package.jsonis excluded by:!**/*.jsonapp/yarn.lockis excluded by:!**/*.lock
Files selected for processing (8)
- app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/tests/get-fo3d-root.test.ts (1 hunks)
- app/packages/looker-3d/src/fo3d/utils.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-fo3d-fetcher.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-fo3d.ts (1 hunks)
- app/packages/state/src/recoil/utils.ts (1 hunks)
- app/packages/utilities/src/paths.test.ts (1 hunks)
- app/packages/utilities/src/paths.ts (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx
- app/packages/looker-3d/src/fo3d/tests/get-fo3d-root.test.ts
- app/packages/looker-3d/src/fo3d/utils.ts
- app/packages/looker-3d/src/hooks/use-fo3d.ts
- app/packages/state/src/recoil/utils.ts
Additional comments: 3
app/packages/looker-3d/src/hooks/use-fo3d-fetcher.ts (1)
- 6-9: Consider adding error handling for the fetch operation within the
useCallbackhook. This could involve wrapping the fetch call in a try-catch block and handling or logging the error appropriately. This will improve the robustness of the fetch operation and provide better feedback in case of failures.app/packages/utilities/src/paths.ts (1)
- 55-56: The change to remove search parameters from the URL in the
resolveParentfunction is a sensible enhancement. It ensures that the function's output is focused on path resolution without being affected by irrelevant URL parameters.app/packages/utilities/src/paths.test.ts (1)
- 94-96: The new test case for
resolveParentcorrectly verifies the functionality of resolving parent paths for URLs with search parameters. Consider adding additional test cases to cover edge cases, such as URLs without paths or with only search parameters, to ensure comprehensive test coverage.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files ignored due to path filters (15)
app/packages/app/package.jsonis excluded by:!**/*.jsonapp/packages/components/package.jsonis excluded by:!**/*.jsonapp/packages/core/package.jsonis excluded by:!**/*.jsonapp/packages/embeddings/package.jsonis excluded by:!**/*.jsonapp/packages/looker-3d/package.jsonis excluded by:!**/*.jsonapp/packages/looker-3d/src/favicon.svgis excluded by:!**/*.svgapp/packages/looker-3d/src/logo.svgis excluded by:!**/*.svgapp/packages/map/package.jsonis excluded by:!**/*.jsonapp/packages/relay/src/fragments/__generated__/datasetFragment.graphql.tsis excluded by:!**/__generated__/**app/packages/relay/src/fragments/__generated__/mediaTypeFragment.graphql.tsis excluded by:!**/__generated__/**app/packages/relay/src/queries/__generated__/mainSampleQuery.graphql.tsis excluded by:!**/__generated__/**app/packages/relay/src/queries/__generated__/paginateSamplesQuery.graphql.tsis excluded by:!**/__generated__/**app/packages/utilities/package.jsonis excluded by:!**/*.jsonapp/yarn.lockis excluded by:!**/*.lockdocs/source/images/app/ app-new-3d-visualizer.gifis excluded by:!**/*.gif
Files selected for processing (107)
- .coderabbit.yaml (1 hunks)
- .github/workflows/build-docs.yml (1 hunks)
- app/packages/app/vite.config.ts (1 hunks)
- app/packages/core/src/components/Actions/GroupMediaVisibilityContainer.tsx (4 hunks)
- app/packages/core/src/components/ColorModal/ShareStyledDiv.ts (1 hunks)
- app/packages/core/src/components/ColorModal/colorPalette/ColorPalette.tsx (1 hunks)
- app/packages/core/src/components/Modal/Group/DynamicGroup/NonNestedGroup/index.tsx (1 hunks)
- app/packages/core/src/components/Modal/Group/GroupCarousel.tsx (1 hunks)
- app/packages/core/src/components/Modal/Group/GroupSample3d.tsx (3 hunks)
- app/packages/core/src/components/Modal/Modal.tsx (2 hunks)
- app/packages/looker-3d/src/Environment.tsx (2 hunks)
- app/packages/looker-3d/src/ErrorBoundary.tsx (1 hunks)
- app/packages/looker-3d/src/Looker3d.tsx (4 hunks)
- app/packages/looker-3d/src/Looker3dPlugin.tsx (2 hunks)
- app/packages/looker-3d/src/MediaTypePcd.tsx (1 hunks)
- app/packages/looker-3d/src/SpinningCube.tsx (1 hunks)
- app/packages/looker-3d/src/StatusBar.tsx (1 hunks)
- app/packages/looker-3d/src/action-bar/ColorSpace.tsx (2 hunks)
- app/packages/looker-3d/src/action-bar/PointSize.tsx (1 hunks)
- app/packages/looker-3d/src/action-bar/SliceSelector.tsx (1 hunks)
- app/packages/looker-3d/src/action-bar/ToggleBackground.tsx (1 hunks)
- app/packages/looker-3d/src/action-bar/ViewHelp.tsx (1 hunks)
- app/packages/looker-3d/src/action-bar/ViewJson.tsx (1 hunks)
- app/packages/looker-3d/src/action-bar/index.tsx (1 hunks)
- app/packages/looker-3d/src/constants.ts (1 hunks)
- app/packages/looker-3d/src/containers.ts (2 hunks)
- app/packages/looker-3d/src/fo3d/AssetErrorBoundary.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/AssetWrapper.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/Background.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/FoScene.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/Gizmos.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/SelectToZoom.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/Stl.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/context.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/BooleanButton.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/boolean-button-style.module.css (1 hunks)
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/index.ts (1 hunks)
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/plugin.ts (1 hunks)
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/types.ts (1 hunks)
- app/packages/looker-3d/src/fo3d/lights/DefaultLights.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/lights/Lights.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/mesh/Obj.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/mesh/Ply.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/node-info/MeshNodeInfo.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/node-info/NodeInfoCard.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/node-info/PcdNodeInfo.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/node-info/index.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/node-info/node-info-containers.ts (1 hunks)
- app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/shape/Box.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/shape/Cylinder.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/shape/Plane.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/shape/Sphere.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/tests/get-fo3d-root.test.ts (1 hunks)
- app/packages/looker-3d/src/fo3d/utils.ts (1 hunks)
- app/packages/looker-3d/src/hooks/index.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-animation-select.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-bounds.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-fo3d-fetcher.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-fo3d.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-hot-key.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-light-controls.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-mesh-material-controls.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-path-filter.ts (1 hunks)
- app/packages/looker-3d/src/hooks/use-pcd-material-controls.ts (1 hunks)
- app/packages/looker-3d/src/labels/index.tsx (1 hunks)
- app/packages/looker-3d/src/renderables/index.ts (1 hunks)
- app/packages/looker-3d/src/renderables/pcd/index.tsx (6 hunks)
- app/packages/looker-3d/src/renderables/pcd/shaders.tsx (11 hunks)
- app/packages/looker-3d/src/state.ts (3 hunks)
- app/packages/looker-3d/src/types.ts (1 hunks)
- app/packages/looker-3d/src/utils.ts (1 hunks)
- app/packages/looker-3d/vite.config.ts (1 hunks)
- app/packages/looker/src/elements/index.ts (5 hunks)
- app/packages/looker/src/elements/three-d.ts (1 hunks)
- app/packages/looker/src/lookers/abstract.ts (1 hunks)
- app/packages/looker/src/lookers/index.ts (1 hunks)
- app/packages/looker/src/lookers/three-d.ts (2 hunks)
- app/packages/looker/src/state.ts (5 hunks)
- app/packages/looker/src/worker/index.ts (1 hunks)
- app/packages/looker/src/worker/threed-label-processor.ts (1 hunks)
- app/packages/relay/src/queries/mainSample.ts (1 hunks)
- app/packages/relay/src/queries/paginateSamples.ts (1 hunks)
- app/packages/state/src/hooks/useCreateLooker.ts (5 hunks)
- app/packages/state/src/recoil/groups.ts (6 hunks)
- app/packages/state/src/recoil/selectors.ts (1 hunks)
- app/packages/state/src/recoil/types.ts (1 hunks)
- app/packages/state/src/recoil/utils.ts (1 hunks)
- app/packages/utilities/src/paths.test.ts (1 hunks)
- app/packages/utilities/src/paths.ts (1 hunks)
- app/schema.graphql (3 hunks)
- app/vite.base.config.ts (1 hunks)
- docs/source/user_guide/app.rst (3 hunks)
- docs/source/user_guide/using_datasets.rst (2 hunks)
- e2e-pw/src/oss/fixtures/loader.ts (1 hunks)
- e2e-pw/src/oss/poms/modal/index.ts (1 hunks)
- e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts (2 hunks)
- e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts (1 hunks)
- fiftyone/public.py (1 hunks)
- fiftyone/core/media.py (2 hunks)
- fiftyone/core/threed/init.py (1 hunks)
- fiftyone/core/threed/camera.py (1 hunks)
- fiftyone/core/threed/lights.py (1 hunks)
- fiftyone/core/threed/material_3d.py (1 hunks)
Files not processed due to max files limit (16)
- fiftyone/core/threed/mesh.py
- fiftyone/core/threed/object_3d.py
- fiftyone/core/threed/pointcloud.py
- fiftyone/core/threed/scene_3d.py
- fiftyone/core/threed/shape_3d.py
- fiftyone/core/threed/transformation.py
- fiftyone/core/threed/utils.py
- fiftyone/core/threed/validators.py
- fiftyone/server/metadata.py
- fiftyone/server/samples.py
- fiftyone/utils/data/exporters.py
- fiftyone/utils/utils3d.py
- requirements/common.txt
- setup.py
- tests/unittests/threed/object_3d_tests.py
- tests/unittests/threed/scene_3d_tests.py
Files skipped from review as they are similar to previous changes (103)
- .coderabbit.yaml
- .github/workflows/build-docs.yml
- app/packages/app/vite.config.ts
- app/packages/core/src/components/Actions/GroupMediaVisibilityContainer.tsx
- app/packages/core/src/components/ColorModal/ShareStyledDiv.ts
- app/packages/core/src/components/Modal/Group/DynamicGroup/NonNestedGroup/index.tsx
- app/packages/core/src/components/Modal/Group/GroupCarousel.tsx
- app/packages/core/src/components/Modal/Group/GroupSample3d.tsx
- app/packages/core/src/components/Modal/Modal.tsx
- app/packages/looker-3d/src/Environment.tsx
- app/packages/looker-3d/src/ErrorBoundary.tsx
- app/packages/looker-3d/src/Looker3dPlugin.tsx
- app/packages/looker-3d/src/MediaTypePcd.tsx
- app/packages/looker-3d/src/SpinningCube.tsx
- app/packages/looker-3d/src/StatusBar.tsx
- app/packages/looker-3d/src/action-bar/ColorSpace.tsx
- app/packages/looker-3d/src/action-bar/PointSize.tsx
- app/packages/looker-3d/src/action-bar/SliceSelector.tsx
- app/packages/looker-3d/src/action-bar/ToggleBackground.tsx
- app/packages/looker-3d/src/action-bar/ViewHelp.tsx
- app/packages/looker-3d/src/action-bar/ViewJson.tsx
- app/packages/looker-3d/src/constants.ts
- app/packages/looker-3d/src/containers.ts
- app/packages/looker-3d/src/fo3d/AssetErrorBoundary.tsx
- app/packages/looker-3d/src/fo3d/AssetWrapper.tsx
- app/packages/looker-3d/src/fo3d/Background.tsx
- app/packages/looker-3d/src/fo3d/FoScene.tsx
- app/packages/looker-3d/src/fo3d/Gizmos.tsx
- app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx
- app/packages/looker-3d/src/fo3d/SelectToZoom.tsx
- app/packages/looker-3d/src/fo3d/Stl.tsx
- app/packages/looker-3d/src/fo3d/context.tsx
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/BooleanButton.tsx
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/boolean-button-style.module.css
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/index.ts
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/plugin.ts
- app/packages/looker-3d/src/fo3d/leva-plugins/boolean-button/types.ts
- app/packages/looker-3d/src/fo3d/lights/DefaultLights.tsx
- app/packages/looker-3d/src/fo3d/lights/Lights.tsx
- app/packages/looker-3d/src/fo3d/mesh/Fbx.tsx
- app/packages/looker-3d/src/fo3d/mesh/Gltf.tsx
- app/packages/looker-3d/src/fo3d/mesh/Obj.tsx
- app/packages/looker-3d/src/fo3d/mesh/Ply.tsx
- app/packages/looker-3d/src/fo3d/node-info/MeshNodeInfo.tsx
- app/packages/looker-3d/src/fo3d/node-info/NodeInfoCard.tsx
- app/packages/looker-3d/src/fo3d/node-info/PcdNodeInfo.tsx
- app/packages/looker-3d/src/fo3d/node-info/index.tsx
- app/packages/looker-3d/src/fo3d/node-info/node-info-containers.ts
- app/packages/looker-3d/src/fo3d/point-cloud/Pcd.tsx
- app/packages/looker-3d/src/fo3d/shape/Box.tsx
- app/packages/looker-3d/src/fo3d/shape/Cylinder.tsx
- app/packages/looker-3d/src/fo3d/shape/Plane.tsx
- app/packages/looker-3d/src/fo3d/shape/Sphere.tsx
- app/packages/looker-3d/src/fo3d/tests/get-fo3d-root.test.ts
- app/packages/looker-3d/src/fo3d/utils.ts
- app/packages/looker-3d/src/hooks/index.ts
- app/packages/looker-3d/src/hooks/use-animation-select.ts
- app/packages/looker-3d/src/hooks/use-bounds.ts
- app/packages/looker-3d/src/hooks/use-fo3d-fetcher.ts
- app/packages/looker-3d/src/hooks/use-fo3d.ts
- app/packages/looker-3d/src/hooks/use-hot-key.ts
- app/packages/looker-3d/src/hooks/use-light-controls.ts
- app/packages/looker-3d/src/hooks/use-mesh-material-controls.ts
- app/packages/looker-3d/src/hooks/use-path-filter.ts
- app/packages/looker-3d/src/hooks/use-pcd-material-controls.ts
- app/packages/looker-3d/src/labels/index.tsx
- app/packages/looker-3d/src/renderables/index.ts
- app/packages/looker-3d/src/renderables/pcd/index.tsx
- app/packages/looker-3d/src/renderables/pcd/shaders.tsx
- app/packages/looker-3d/src/types.ts
- app/packages/looker-3d/src/utils.ts
- app/packages/looker-3d/vite.config.ts
- app/packages/looker/src/elements/index.ts
- app/packages/looker/src/elements/three-d.ts
- app/packages/looker/src/lookers/abstract.ts
- app/packages/looker/src/lookers/index.ts
- app/packages/looker/src/lookers/three-d.ts
- app/packages/looker/src/state.ts
- app/packages/looker/src/worker/index.ts
- app/packages/looker/src/worker/threed-label-processor.ts
- app/packages/relay/src/queries/mainSample.ts
- app/packages/relay/src/queries/paginateSamples.ts
- app/packages/state/src/hooks/useCreateLooker.ts
- app/packages/state/src/recoil/groups.ts
- app/packages/state/src/recoil/selectors.ts
- app/packages/state/src/recoil/types.ts
- app/packages/state/src/recoil/utils.ts
- app/packages/utilities/src/paths.test.ts
- app/packages/utilities/src/paths.ts
- app/schema.graphql
- app/vite.base.config.ts
- docs/source/user_guide/app.rst
- docs/source/user_guide/using_datasets.rst
- e2e-pw/src/oss/fixtures/loader.ts
- e2e-pw/src/oss/poms/modal/index.ts
- e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts
- e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts
- fiftyone/public.py
- fiftyone/core/media.py
- fiftyone/core/threed/init.py
- fiftyone/core/threed/camera.py
- fiftyone/core/threed/lights.py
- fiftyone/core/threed/material_3d.py
Additional comments: 33
app/packages/looker-3d/src/state.ts (16)
- 3-3: The import of
atomFamilyfrom "recoil" is added, which is good for creating parameterized atoms. Ensure that its usage inactionRenderListAtomFamilyis correctly implemented and serves the intended purpose.- 4-6: Constants
SHADE_BY_HEIGHT,Actions,ShadeBy, andVisibilityMapare added. Verify that these constants are correctly used throughout the file and align with the application's state management strategy.- 8-11: The
worldBoundsAtomis correctly defined with a default value ofnull. Ensure that the key is unique across the application to avoid conflicts in the global state.- 14-14: The key for
shadeByAtomhas been updated. Confirm that all references to this atom throughout the application have been updated to reflect the new key.- 20-20: The
customColorMapAtomincludes JSON serialization for browser storage. Verify that the serialization and deserialization logic correctly handles all edge cases, especially concerning null values and empty objects.- 29-35: The
actionRenderListAtomFamilyis introduced to manage render lists based on media type. Ensure that the parameterization by media type ("pcd"or"fo3d") is correctly handled in all parts of the application where this atom family is used.- 38-38: The
currentActionAtomhas a default value ofnull. Confirm that the application correctly handles this null state wherevercurrentActionAtomis consumed.- 43-43: The default point size is set to "2". Ensure that this default value is appropriate for all use cases and that the application allows users to adjust the point size as needed.
- 49-49: The
pointSizeRangeAtomdefines a range for point sizes. Verify that the UI components interacting with this atom provide a user-friendly way to select values within this range.- 54-54: The
isPointSizeAttenuatedAtomincludes a boolean value with browser storage effects. Ensure that the boolean value is correctly interpreted in the UI and that any changes to this state are persisted across sessions.- 64-64: The
isGridOnAtomhas a default value oftrue. Confirm that this default state aligns with the user's expectations and that users can easily toggle this setting.- 73-73: The
isFo3dBackgroundOnAtomcontrols the visibility of the background in 3D views. Ensure that toggling this atom's state results in the expected changes in the UI.- 83-83: The
isStatusBarOnAtomhas a default value offalse. Verify that this default aligns with the application's design and that the status bar's visibility can be toggled by the user if needed.- 88-88: The
panelPositionAtomallows for storing the position of a UI panel. Ensure that the application correctly updates and uses this position state, especially in scenarios where the panel position is reset tonull.- 93-93: The
activeNodeAtomis set tonullby default. Confirm that the application handles this null state appropriately, especially in contexts where an active node is expected.- 98-98: The
currentVisibilityMapAtomstarts with an empty object. Ensure that the application correctly populates and utilizes this map for controlling the visibility of various elements in the 3D view.app/packages/looker-3d/src/action-bar/index.tsx (7)
- 1-14: The import of new dependencies and constants is correctly done. Ensure that all imported entities are used appropriately within the component and that there are no unused imports.
- 26-32: The
ActionBarcomponent's props are correctly defined. Verify thatonMouseEnterandonMouseLeavehandlers are properly used to manage hover states in the parent component.- 33-38: The determination of
isFo3dbased onisFo3dSliceormediaTypeis a good use ofuseMemofor optimization. Confirm that this logic correctly identifies all scenarios where theActionBarshould render for 3D media.- 39-46: The dynamic determination of
actionBarRenderListusingactionRenderListAtomFamilyis well-implemented. Ensure that the parameterization by media type ("fo3d"or"pcd") is correctly handled in all parts of the application where this atom family is used.- 47-50: The logic to determine if the
SliceSelectorshould be rendered based on the presence ofACTION_SET_PCDSinactionNamesis clear and concise. Verify that theSliceSelectorcomponent behaves as expected when toggled.- 51-127: The dynamic rendering of components based on
actionNamesis a robust approach. Ensure that each conditional block correctly checks for the presence of an action and renders the corresponding component. Also, verify that thekeyprops are unique and meaningful, and that any callbacks passed to components are correctly implemented.- 129-138: The final rendering of the
ActionBarwith conditional rendering of theSliceSelectorand dynamic components is well-structured. Confirm that theonMouseEnterandonMouseLeavehandlers are correctly used to manage hover states and that thedata-cyattribute is used consistently for testing purposes.app/packages/looker-3d/src/Looker3d.tsx (9)
- 12-16: The component documentation clearly explains the responsibility of the
Looker3dcomponent and its support for both "3d" and "point_cloud" media types. Ensure that this documentation is kept up-to-date as the component evolves.- 18-22: The use of Recoil state to determine the media type and slice information is appropriate. Verify that these state values are correctly managed elsewhere in the application to ensure accurate rendering logic in
Looker3d.- 24-26: The state management for hover effects using
useStateanduseRefis correctly implemented. Confirm that the hover state is correctly used throughout the component to manage UI interactions.- 30-42: The conditional rendering logic for
shouldRenderPcdComponentandshouldRenderFo3dComponentis well-structured. Ensure that all conditions accurately reflect the scenarios in which each component should be rendered.- 47-52: The use of
useHotkeyfor toggling the grid is a good application of custom hooks. Confirm that the hotkey functionality works as expected and that theisGridOnAtomstate is correctly toggled.- 1-64: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [57-96]
The escape key handling logic to close panels and clear selections is comprehensive. Ensure that all scenarios are correctly handled, including the special case where the hovered sample is not in Looker3d.
- 99-114: The
clearandupdatecallbacks for managing hover states and timeouts are correctly implemented. Verify that these functions are called at appropriate times to manage the hover state accurately.- 116-121: The error message for having both fo3d and pcd slices in a group is clear. Ensure that this message is displayed in a user-friendly manner and that users are guided on how to resolve this configuration issue.
- 125-148: The final rendering logic, including conditional rendering of
MediaTypePcdComponentorMediaTypeFo3dComponentand theActionBar, is well-structured. Confirm that all components are rendered as expected based on the state and that theErrorBoundaryis correctly used to catch and handle errors.app/packages/core/src/components/ColorModal/colorPalette/ColorPalette.tsx (1)
- 2-3: The update of import paths for
AddIconandDeleteIconfrom@mui/icons-materialis correctly done. Ensure that these icons are used appropriately within the component and that there are no issues with their rendering or functionality.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 13
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- fiftyone/core/threed/scene_3d.py (1 hunks)
- fiftyone/utils/data/exporters.py (3 hunks)
Additional comments: 5
fiftyone/core/threed/scene_3d.py (5)
- 105-111: The constructor of the
Sceneclass is well-implemented, leveraging optional parameters and providing sensible defaults. This approach enhances the flexibility and usability of the class.- 203-213: The
traversemethod effectively implements scene graph traversal using a generator. This method is a good example of Python's generator functionality for efficient traversal of potentially large scene graphs.- 251-279: The
get_asset_pathsmethod efficiently collects all asset paths in the scene. This method demonstrates good use of polymorphism by checking the instance type before accessing specific attributes. However, consider adding a comment to clarify the purpose of appending material paths separately forObjMeshinstances, as this might not be immediately clear to readers.Consider adding a comment explaining why material paths are handled separately for
ObjMeshinstances.
- 290-301: The
_to_dict_extramethod effectively serializes additional scene properties. It's a good practice to separate serialization logic for clarity and maintainability. This method is well-implemented and follows the established pattern.- 303-335: The static method
_from_fo3d_dictand the class methodfrom_fo3dare crucial for deserializing scene data from.fo3dfiles. These methods are well-documented and implemented, ensuring that scenes can be easily loaded from disk. However, consider adding error handling for file operations infrom_fo3dto gracefully handle cases where the specified file does not exist or is inaccessible.Consider adding error handling for file operations in the
from_fo3dmethod to improve robustness.
Comment on lines
+1158
to
+1237
| def _handle_fo3d_file(self, fo3d_path, fo3d_output_path, export_mode): | ||
| if export_mode in (False, "manifest"): | ||
| return | ||
|
|
||
| scene = fo3d.Scene.from_fo3d(fo3d_path) | ||
| asset_paths = scene.get_asset_paths() | ||
|
|
||
| for asset_path in asset_paths: | ||
| if not os.path.isabs(asset_path): | ||
| absolute_asset_path = os.path.join( | ||
| os.path.dirname(fo3d_path), asset_path | ||
| ) | ||
| else: | ||
| absolute_asset_path = asset_path | ||
|
|
||
| seen = self._filename_maker.seen_input_path(absolute_asset_path) | ||
|
|
||
| if seen: | ||
| continue | ||
|
|
||
| asset_output_path = self._filename_maker.get_output_path( | ||
| absolute_asset_path | ||
| ) | ||
|
|
||
| if export_mode is True: | ||
| etau.copy_file(absolute_asset_path, asset_output_path) | ||
| elif export_mode == "move": | ||
| etau.move_file(absolute_asset_path, asset_output_path) | ||
| elif export_mode == "symlink": | ||
| etau.symlink_file(absolute_asset_path, asset_output_path) | ||
|
|
||
| is_scene_modified = False | ||
|
|
||
| for node in scene.traverse(): | ||
| path_attribute = next( | ||
| ( | ||
| attr | ||
| for attr in fo3d.fo3d_path_attributes | ||
| if hasattr(node, attr) | ||
| ), | ||
| None, | ||
| ) | ||
|
|
||
| if path_attribute is not None: | ||
| asset_path = getattr(node, path_attribute) | ||
|
|
||
| is_nested_path = os.path.split(asset_path)[0] != "" | ||
|
|
||
| if asset_path is not None and is_nested_path: | ||
| setattr(node, path_attribute, os.path.basename(asset_path)) | ||
| is_scene_modified = True | ||
|
|
||
| # modify scene background paths, if any | ||
| if scene.background is not None: | ||
| if scene.background.image is not None: | ||
| scene.background.image = os.path.basename( | ||
| scene.background.image | ||
| ) | ||
| is_scene_modified = True | ||
|
|
||
| if scene.background.cube is not None: | ||
| scene.background.cube = [ | ||
| os.path.basename(face_path) | ||
| for face_path in scene.background.cube | ||
| ] | ||
| is_scene_modified = True | ||
|
|
||
| if is_scene_modified: | ||
| # note: we can't have different behavior for "symlink" because | ||
| # scene is modified, so we just copy the file regardless | ||
| scene.write(fo3d_output_path) | ||
| else: | ||
| if export_mode == "symlink": | ||
| etau.symlink_file(fo3d_path, fo3d_output_path) | ||
| else: | ||
| etau.copy_file(fo3d_path, fo3d_output_path) | ||
|
|
||
| if export_mode == "move": | ||
| etau.delete_file(fo3d_path) | ||
|
|
There was a problem hiding this comment.
The method _handle_fo3d_file in FiftyOneImageClassificationDatasetExporter is designed to handle fo3d files specifically, which seems out of place given the class's purpose of exporting image classification datasets. This method appears to be more suited for a 3D data exporter class.
Consider moving _handle_fo3d_file to a more appropriate class that deals with 3D data export, ensuring that each exporter class remains focused on its specific data type.
The method _handle_fo3d_file appears in ImageDirectoryExporter, which is intended for exporting a directory of images. This method is specific to handling fo3d files, which is unrelated to the primary function of this class.
To maintain class responsibility integrity, consider removing _handle_fo3d_file from ImageDirectoryExporter or refactoring the codebase to ensure that 3D file handling is performed in a class dedicated to 3D data.
The presence of _handle_fo3d_file in VideoDirectoryExporter is inconsistent with the class's purpose of exporting video directories. This method deals with 3D file formats, which is not relevant to video data.
It's recommended to remove _handle_fo3d_file from VideoDirectoryExporter or to ensure that 3D data handling is allocated to a class specifically designed for that purpose, keeping the class responsibilities clear and focused.
Including _handle_fo3d_file in MediaDirectoryExporter introduces a specific 3D data handling capability into a class designed for exporting generic media files. This method's functionality seems misaligned with the class's general purpose.
To keep the class's responsibilities well-defined, consider removing _handle_fo3d_file from MediaDirectoryExporter or creating a dedicated exporter class for 3D data that encapsulates this functionality.
The method _handle_fo3d_file found in FiftyOneImageDetectionDatasetExporter is specifically for handling fo3d files, which is not relevant to the primary function of exporting image detection datasets.
To ensure that each exporter class focuses on a specific dataset type, consider removing _handle_fo3d_file from FiftyOneImageDetectionDatasetExporter or integrating it into a class that explicitly deals with 3D data exports.
The inclusion of _handle_fo3d_file in FiftyOneTemporalDetectionDatasetExporter introduces 3D file handling into a class designed for exporting temporal video detection datasets. This method's functionality does not align with the class's intended purpose.
Consider removing _handle_fo3d_file from FiftyOneTemporalDetectionDatasetExporter to maintain the class's focus on temporal video detection data. Alternatively, introduce a dedicated class for handling 3D data exports that includes this method.
The method _handle_fo3d_file in ImageSegmentationDirectoryExporter is designed for handling fo3d files, which is not relevant to the class's primary function of exporting image segmentation datasets.
To keep the class responsibilities clear, consider removing _handle_fo3d_file from ImageSegmentationDirectoryExporter or ensuring that 3D data handling is performed by a class specifically designed for that purpose.
Including _handle_fo3d_file in FiftyOneImageLabelsDatasetExporter introduces specific 3D data handling functionality into a class designed for exporting labeled image datasets in ETA ImageLabels format.
Consider removing _handle_fo3d_file from FiftyOneImageLabelsDatasetExporter to maintain the class's focus on exporting labeled image data. If 3D data handling is necessary, it should be encapsulated within a class dedicated to 3D data exports.
The presence of _handle_fo3d_file in FiftyOneVideoLabelsDatasetExporter is inconsistent with the class's purpose of exporting labeled video datasets in ETA VideoLabels format. This method deals with 3D file formats, which is not relevant to video data.
It's recommended to remove _handle_fo3d_file from FiftyOneVideoLabelsDatasetExporter or to ensure that 3D data handling is allocated to a class specifically designed for that purpose, keeping the class responsibilities clear and focused.
* linting * minor lints * linting * linting * linting * update slider thumb css * label tag count bug * doc string pass * update class * theme leva * add feat/ to docs trigger * lint tests * restrict SDK to one fo3d group slice * lint components * update pydantic * add feat/ to pr checks * 3d or point-cloud * update gif and fix filename * add fullscreen to Looker3d * typo * validate with set * Update fiftyone/core/dataset.py typo Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update app/packages/looker-3d/src/fo3d/Background.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * final docs tweaks --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files ignored due to path filters (3)
app/packages/looker-3d/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/*.lockdocs/source/images/app/app-new-3d-visualizer.gifis excluded by!**/*.gif
Files selected for processing (38)
- .github/workflows/build-docs.yml (2 hunks)
- .github/workflows/pr.yml (1 hunks)
- app/packages/app/src/index.css (1 hunks)
- app/packages/components/src/components/ThemeProvider/index.tsx (1 hunks)
- app/packages/components/src/components/index.ts (1 hunks)
- app/packages/core/src/components/Common/RangeSlider.tsx (1 hunks)
- app/packages/looker-3d/src/Looker3d.tsx (4 hunks)
- app/packages/looker-3d/src/MediaTypePcd.tsx (1 hunks)
- app/packages/looker-3d/src/constants.ts (1 hunks)
- app/packages/looker-3d/src/containers.ts (2 hunks)
- app/packages/looker-3d/src/fo3d/Background.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/FoScene.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/Gizmos.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/Leva.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx (1 hunks)
- app/packages/looker-3d/src/fo3d/node-info/PcdNodeInfo.tsx (1 hunks)
- app/packages/looker-3d/src/hooks/use-light-controls.ts (1 hunks)
- app/packages/state/src/recoil/pathData/labels.ts (1 hunks)
- docs/source/user_guide/app.rst (3 hunks)
- docs/source/user_guide/using_datasets.rst (2 hunks)
- fiftyone/public.py (2 hunks)
- fiftyone/core/dataset.py (1 hunks)
- fiftyone/core/threed/init.py (1 hunks)
- fiftyone/core/threed/camera.py (1 hunks)
- fiftyone/core/threed/lights.py (1 hunks)
- fiftyone/core/threed/material_3d.py (1 hunks)
- fiftyone/core/threed/mesh.py (1 hunks)
- fiftyone/core/threed/object_3d.py (1 hunks)
- fiftyone/core/threed/pointcloud.py (1 hunks)
- fiftyone/core/threed/scene_3d.py (1 hunks)
- fiftyone/core/threed/shape_3d.py (1 hunks)
- fiftyone/core/threed/transformation.py (1 hunks)
- fiftyone/core/threed/utils.py (1 hunks)
- fiftyone/utils/utils3d.py (7 hunks)
- setup.py (1 hunks)
- tests/unittests/group_tests.py (1 hunks)
- tests/unittests/threed/object_3d_tests.py (1 hunks)
- tests/unittests/threed/scene_3d_tests.py (1 hunks)
Path instructions used (6)
app/packages/state/src/recoil/pathData/labels.ts (1)
**/*.{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-3d/src/fo3d/Leva.tsx (1)
**/*.{ts,tsx}
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/components/src/components/index.ts (1)
**/*.{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-3d/src/Looker3d.tsx (1)
**/*.{ts,tsx}
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Common/RangeSlider.tsx (1)
**/*.{ts,tsx}
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/components/src/components/ThemeProvider/index.tsx (1)
**/*.{ts,tsx}
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files skipped from review due to trivial changes (1)
- app/packages/app/src/index.css
Files skipped from review as they are similar to previous changes (18)
- .github/workflows/build-docs.yml
- app/packages/looker-3d/src/MediaTypePcd.tsx
- app/packages/looker-3d/src/constants.ts
- app/packages/looker-3d/src/containers.ts
- app/packages/looker-3d/src/fo3d/Background.tsx
- app/packages/looker-3d/src/fo3d/FoScene.tsx
- app/packages/looker-3d/src/fo3d/Gizmos.tsx
- app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx
- app/packages/looker-3d/src/fo3d/node-info/PcdNodeInfo.tsx
- app/packages/looker-3d/src/hooks/use-light-controls.ts
- docs/source/user_guide/app.rst
- docs/source/user_guide/using_datasets.rst
- fiftyone/public.py
- fiftyone/core/threed/init.py
- fiftyone/core/threed/camera.py
- fiftyone/core/threed/lights.py
- fiftyone/core/threed/material_3d.py
- fiftyone/core/threed/scene_3d.py
Additional comments (29)
app/packages/state/src/recoil/pathData/labels.ts (1)
- 15-17: The addition of a check for the length of
pathsbefore proceeding with the sum calculation is a good practice. It improves the robustness of thelabelCountselector function by avoiding unnecessary operations on empty arrays and implicitly handles potential errors.fiftyone/core/threed/utils.py (1)
- 12-44: The utility functions for converting camelCase to snake_case are implemented efficiently using regular expressions and handle various data structures (dictionaries, lists, and nested structures) properly. The special handling of
FO3D_VERSION_KEYis a thoughtful detail. These functions are well-suited for data serialization/deserialization purposes in the context of 3D visualization support.app/packages/looker-3d/src/fo3d/Leva.tsx (1)
- 6-48: The implementation of the
Levacomponent demonstrates good React practices, including the use of hooks (useThemeanduseFont) for consistent theming and fonts, and the use ofcreatePortalfor rendering the component in a modal. This approach enhances accessibility and styling isolation.app/packages/components/src/components/index.ts (1)
- 25-25: Exporting
useFontalongsideuseThemefromThemeProviderimproves theming consistency and developer experience by making both hooks easily available for import and use throughout the application.fiftyone/core/threed/transformation.py (1)
- 8-63: The classes
Vector3,Euler, andQuaternionare well-implemented, with clear documentation and efficient use ofnumpyandscipyfor mathematical operations. The conversion methods between Euler angles and quaternions are correctly implemented, making these classes fundamental for handling 3D transformations and rotations.tests/unittests/threed/scene_3d_tests.py (1)
- 17-75: The unit tests for the
Sceneclass are well-implemented, covering important functionalities such as error handling, serialization/deserialization, and data structure manipulation. The use ofmock_openandpatchfor testing file operations and the thorough tests for camelCase to snake_case conversion ensure the reliability of the 3D visualization features.fiftyone/core/threed/pointcloud.py (1)
- 15-76: The
Pointcloudclass is well-implemented, with clear documentation, appropriate error handling for file path validation, and thoughtful design for handling materials and serialization. This class represents a solid foundation for handling point clouds in the context of 3D visualization.setup.py (1)
- 62-62: Adding
pydantic>=2as a dependency is approved. Ensure to verify its compatibility with other project dependencies to avoid potential conflicts.app/packages/looker-3d/src/Looker3d.tsx (1)
- 1-74: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [12-161]
Refactoring of the
Looker3dcomponent to support both "3d" and "point_cloud" media types is well-implemented. The use of React hooks, Recoil state management, and conditional rendering based on media types is in line with best practices. Great job on maintaining readability and efficiency in the component's logic.app/packages/components/src/components/ThemeProvider/index.tsx (1)
- 235-237: The addition of the
useFontfunction is approved. It provides a clean and straightforward way to access the font family from the theme, aligning with best practices in React and TypeScript.fiftyone/core/threed/shape_3d.py (1)
- 16-228: The implementation of the 3D shape classes (
Shape3D,BoxGeometry,CylinderGeometry,SphereGeometry,PlaneGeometry) is well-done. The structure, use of type annotations, and consistent serialization logic via_to_dict_extramethods are commendable. The design is flexible, allowing for default materials to be overridden, and follows Python best practices.tests/unittests/threed/object_3d_tests.py (1)
- 16-253: The unit tests for the
Object3Dclass and its related geometry classes are thorough and well-implemented. They cover a wide range of scenarios, including initialization, matrix transformations, and serialization/deserialization, adhering to best practices for unit testing in Python. The use ofnumpyfor precise array comparisons is commendable.fiftyone/core/threed/mesh.py (1)
- 15-251: The implementation of the 3D mesh classes (
Mesh,ObjMesh,FbxMesh,GltfMesh,PlyMesh,StlMesh) is well-done. The structure, use of type annotations, and consistent serialization logic via_to_dict_extramethods are commendable. The validation of file extensions for each mesh type enhances robustness. The design follows Python best practices.fiftyone/core/threed/object_3d.py (10)
- 34-40: The constructor of
Object3Dclass initializes several properties. Consider adding type hints forvisible,position,scale, andquaternionparameters in the docstring to improve code readability and documentation.- 85-88: The setter for
positionusesnormalize_to_vec3for validation and normalization. Ensure thatnormalize_to_vec3handles all expected input types correctly and throws meaningful errors for invalid inputs.- 95-105: The setter for
rotationacceptsEuler,List[float], orTuple[float, ...]. This flexibility is good, but ensure that the conversion from list or tuple toEuleris robust and handles edge cases or invalid inputs gracefully.- 112-124: The setter for
quaternionis similar torotationin handling multiple input types. Verify that the conversion logic and error handling inQuaternionare robust, especially for edge cases.- 131-134: The setter for
scaleusesnormalize_to_vec3for normalization. It's important to ensure that this function can handle all expected types and values for scale, including negative values or zeros, which might have special meanings in 3D contexts.- 174-205: The
_update_matrixmethod combines translation, rotation, and scale matrices to update the local transform matrix. Ensure that the order of matrix multiplication is correct for the intended transformation hierarchy (i.e., scale, then rotate, then translate).- 215-221: The
traversemethod yields all objects in the scene graph. Verify that this method, combined with theaddmethod's logic, correctly handles complex scene graphs without causing infinite loops or missing objects.- 223-239: The
as_dictmethod serializes the object to a dictionary. Ensure that all relevant properties are included and that the serialization logic is consistent across different object types. Also, consider handling potential serialization issues with custom types likeVector3andQuaternion.- 246-298: The
_from_dictstatic method deserializes an object from a dictionary. Verify that this method correctly handles all expected dictionary formats and gracefully handles missing or invalid data. Additionally, ensure that the recursive handling of children does not introduce vulnerabilities, such as infinite recursion from circular references.- 23-298: Overall, the
Object3Dclass provides a solid foundation for representing 3D objects in a scene. Consider the following design improvements:
- Ensure that all methods and properties are necessary and serve a clear purpose, adhering to the Single Responsibility Principle.
- Review the class for opportunities to increase modularity and extensibility, such as abstracting transformation logic or serialization/deserialization mechanisms.
- Verify that the class design allows for easy extension and customization for specific types of 3D objects without requiring modifications to the base class.
fiftyone/utils/utils3d.py (4)
- 19-30: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [8-27]
The organization and grouping of import statements follow Python's PEP 8 guidelines, which is good practice for readability and maintainability.
- 439-455: The function
_get_pcd_filepath_from_fo3d_scenecorrectly implements a depth-first search to find the PCD file path within a 3D scene. Ensure that theflag_for_projectionattribute used to determine the correct PCD file is well-documented.- 539-570: The modifications to
compute_orthographic_projection_imagesto support theTHREE_Dmedia type and handle.fo3dfiles are well-integrated. Ensure thorough testing, especially for edge cases where a scene might not contain a PCD file or the file path is incorrect.- 790-802: The renaming of
_get_point_cloud_sliceto_get_3d_slicealigns with the broader support for 3D data. Ensure that all references to the old function name are updated across the codebase.Verification successful
The renaming of
_get_point_cloud_sliceto_get_3d_sliceappears to have been thoroughly applied across the codebase, as no references to the old function name were found. It's recommended to manually verify any indirect references or comments that might not have been caught by the script.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old function name. rg --type py "_get_point_cloud_slice"Length of output: 37
tests/unittests/group_tests.py (2)
- 256-272: The new test method
test_one_fo3d_group_sliceis well-structured and focuses on testing the group slice functionality with the new3dmedia type. It's important to ensure comprehensive test coverage for all relevant scenarios. Consider adding comments within the test to explain the rationale behind each assertion for future maintainability.- 253-275: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-272]
Overall, the file
group_tests.pyappears to be well-organized and follows consistent patterns for testing various aspects of group functionality in datasets. Ensure that all tests are adequately isolating the functionality they aim to test and that setup and teardown operations are correctly managed. It's also beneficial to periodically review test coverage to ensure that new features and edge cases are adequately tested.
Comment on lines
+207
to
+209
| def add(self, *objs: "Object3D") -> None: | ||
| """Add one or more objects as children of this one.""" | ||
| self.children.extend(objs) |
There was a problem hiding this comment.
The add method allows adding children to the object. Ensure that there's logic in place to prevent adding an object as a child of itself or creating circular references, which could lead to infinite loops during traversal.
swheaton
requested changes
Mar 29, 2024
sashankaryal
requested review from
swheaton
and removed request for
ritch and
imanjra
swheaton
approved these changes
Mar 29, 2024
|
Nominal approval - we decided we will merge and further changes requested may be made in a follow up PR |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Scene,Material,Mesh,Shape,PointCloud, andLight, along with their respective serialization and deserialization logic.Testing
Summary by CodeRabbit