Add ability to dynamically alter 3D labels line width #4590
Conversation
WalkthroughThe recent updates enhance the Looker 3D application's functionality by introducing new constants and significantly improving the rendering of line segments and cuboids using advanced Three.js techniques. State management with Recoil is implemented for dynamic control of line widths, enriching the user experience and visual fidelity. Collectively, these changes streamline the management of visual elements, making it easier to customize and interact with 3D components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Recoil
participant Cuboid
participant Line
User->>Cuboid: Adjust line width
Cuboid->>Recoil: Update cuboidLineWidth
Recoil-->>Cuboid: State updated
Cuboid->>Line: Render with new width
Line-->>User: Display updated line
Poem
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 Configuration 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: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/packages/looker-3d/src/constants.ts (1 hunks)
- app/packages/looker-3d/src/labels/cuboid.tsx (4 hunks)
- app/packages/looker-3d/src/labels/index.tsx (3 hunks)
- app/packages/looker-3d/src/labels/line.tsx (3 hunks)
- app/packages/looker-3d/src/state.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/packages/looker-3d/src/constants.ts
Additional context used
Path-based instructions (4)
app/packages/looker-3d/src/labels/line.tsx (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-3d/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.app/packages/looker-3d/src/labels/cuboid.tsx (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-3d/src/labels/index.tsx (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 (22)
app/packages/looker-3d/src/labels/line.tsx (7)
4-10: Imports look good.The new imports for
useRecoilState,polylineLabelLineWidthAtom,Line2,LineMaterial,LineGeometry, andextendare necessary and correctly used in the file.
11-11: Extend function usage looks good.The
extendfunction is correctly used to registerLine2,LineMaterial, andLineGeometryglobally.
13-20: JSX namespace extension looks good.The global JSX namespace extension for
line2andlineGeometryis correctly implemented.
35-35: Recoil state usage looks good.The
useRecoilStatehook is correctly used to manage the line width dynamically.
50-54: Geometry creation looks good.The
LineGeometryis correctly created using the positions from thegeoattributes.
55-60: Material creation looks good.The
LineMaterialis correctly created using the color and line width from the Recoil state.
63-65: Component rendering looks good.The
Linecomponent correctly renders theline2element with the created geometry and material.app/packages/looker-3d/src/state.ts (2)
131-134: Cuboid label line width atom looks good.The
cuboidLabelLineWidthAtomis correctly implemented with a default value of 3.
136-139: Polyline label line width atom looks good.The
polylineLabelLineWidthAtomis correctly implemented with a default value of 3.app/packages/looker-3d/src/labels/cuboid.tsx (7)
3-10: Imports look good.The new imports for
LineSegments2,LineMaterial,LineSegmentsGeometry,useRecoilState, andcuboidLabelLineWidthAtomare necessary and correctly used in the file.
12-12: Extend function usage looks good.The
extendfunction is correctly used to registerLineSegments2,LineMaterial, andLineSegmentsGeometryglobally.
14-23: JSX namespace extension looks good.The global JSX namespace extension for
lineSegments2andlineSegmentsGeometryis correctly implemented.
45-45: Recoil state usage looks good.The
useRecoilStatehook is correctly used to manage the line width dynamically.
75-79: Geometry creation looks good.The
LineSegmentsGeometryis correctly created using the edges of the box geometry.
80-89: Material creation looks good.The
LineMaterialis correctly created using the opacity, color, and line width from the Recoil state.
100-100: Component rendering looks good.The
Cuboidcomponent correctly renders thelineSegments2element with the created geometry and material.app/packages/looker-3d/src/labels/index.tsx (6)
10-10: Import statement updated correctly.The import statement now includes
useRecoilState, which is necessary for managing state with Recoil.
20-21: New imports are appropriate.The imports from
levaand the state file are necessary for the new functionality.
26-32: Global namespace declaration is correct.The global namespace declaration for
lineMaterialis necessary for using it as a JSX intrinsic element.
50-55: State management for line widths is implemented correctly.The state for
cuboidLineWidthandpolylineWidthis managed using Recoil atoms.
60-81: Configuration for line width controls is well-defined.The configuration object
constLabelLevaContralsdefines properties for the line width controls, facilitating their integration into the UI usingleva.
83-91: Controls integrated into the UI correctly.The
useControlshook fromlevaintegrates the line width controls into the UI, allowing dynamic adjustments.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/looker-3d/src/labels/line.tsx (3 hunks)
Additional context used
Path-based instructions (1)
app/packages/looker-3d/src/labels/line.tsx (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 (6)
app/packages/looker-3d/src/labels/line.tsx (6)
4-10: Imports look good.The new imports are necessary for the updated functionality and are correctly used in the code.
11-11: Extend call is correctly used.The
extendfunction is correctly used to addLine2,LineMaterial, andLineGeometryto the JSX namespace.
13-20: Global declarations are correctly added.The
declare globalblock correctly addsline2andlineGeometryto the JSX intrinsic elements.
35-35: Recoil state integration looks good.The
Linecomponent correctly uses the Recoil state for line width.
50-59: Memoized geometry and material look good.The
geometryandmaterialare correctly memoized to optimize performance.
62-64: Return statement integration looks good.The return statement correctly integrates the new
line2andlineGeometrycomponents.
|
whoa code rabbit is making seq diagrams now lol |
| }, [geo]); | ||
| const material = useMemo( |
| const selectedLabels = useRecoilValue(fos.selectedLabelMap); | ||
| const tooltip = fos.useTooltip(); | ||
| const labelAlpha = colorScheme.opacity; | ||
|
|
||
| const constLabelLevaContrals = { |
There was a problem hiding this comment.
nit: typo. Contrals -> Controls
| declare global { | ||
| namespace JSX { | ||
| interface IntrinsicElements { | ||
| line2: ReactThreeFiber.Node<Line2, typeof Line2>; | ||
| lineGeometry: ReactThreeFiber.Node<LineGeometry, typeof LineGeometry>; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
can we put these declaration merging / global augmentation code in a separate file? maybe three-d-labels.d.ts, or even better index.d.ts in looker-3d package root?
| }, [geo]); | ||
| const material = React.useMemo(() => { |
sashankaryal
changed the title
|
@CamronStaley great work tracking down what turned into a bigger task than initially expected! Couple feature questions before looking at the code:
Will this impact performance or number of lines we can support? I'm imagining number of points quadrupling - 2 points in a line to 8 points for a long cuboid. Where does the line width setting live? Is it for the current scene only or the whole session? Stored in the fo3d file? |
I think this could impact performance in cases where there are tons of lines / cuboids, but I can't say for certain how much it will hurt it. Since it's an extension of mesh I would assume it's similar efficiency to that, which was already used for cuboids. One potential solution could be if we are seeing performance issues with >X amount of lines we could disable the option to increase line thickness and default back to the old implementation to save performance in those cases.
Currently it's only stored in state so it won't persist between sessions, but it will persist between samples. We can discuss if we think it should persist between sessions in some way since I agree users who want it to always remain at X size should have that option. This could be a bigger discussion about settings as a whole since there are other settings which could probably live in the user's settings such as lighting for 3d scenes. |
I'm not sure exactly how to measure performance of a web drawing tool but you could try this? Add a zillion lines to one of the scenes until the approach in this PR slows down. Then try same scene with the old lines approach and note any differences.
lighting settings can be persisted in the FO3D file |
|
We can persist line width settings in the browser storage for now. Add |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (3)
e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts (1)
86-87: Add a comment to explain the purpose of the new assertion.The new assertion
modal.assert.verifyLevaFolders(["stl", "pcd"]);verifies the presence of specific folders in the modal context. Adding a comment to explain its purpose would improve code readability.+ // Verify that the modal contains the expected folders for STL and PCD assets. await modal.assert.verifyLevaFolders(["stl", "pcd"]);app/packages/looker-3d/src/labels/cuboid.tsx (1)
33-33: Add a default value forlineWidth.To prevent potential issues with undefined values, consider adding a default value for
lineWidth.- const [lineWidth] = useRecoilState(cuboidLabelLineWidthAtom); + const [lineWidth] = useRecoilState(cuboidLabelLineWidthAtom) || 1;e2e-pw/src/oss/poms/modal/index.ts (1)
38-38: Add a comment to explain the purpose of thelevaproperty.Adding a comment to explain the purpose of the
levaproperty would improve code readability.+ // Initialize the Leva property for managing Leva controls in the modal. this.leva = new ModalLevaPom(page, this);
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- app/packages/looker-3d/index.d.ts (1 hunks)
- app/packages/looker-3d/src/labels/cuboid.tsx (4 hunks)
- app/packages/looker-3d/src/labels/index.tsx (3 hunks)
- app/packages/looker-3d/src/labels/line.tsx (3 hunks)
- app/packages/looker-3d/src/state.ts (1 hunks)
- e2e-pw/src/oss/poms/modal/index.ts (4 hunks)
- e2e-pw/src/oss/poms/modal/leva.ts (1 hunks)
- e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/packages/looker-3d/src/labels/index.tsx
- app/packages/looker-3d/src/state.ts
Additional context used
Path-based instructions (6)
app/packages/looker-3d/index.d.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.e2e-pw/src/oss/poms/modal/leva.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-3d/src/labels/line.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.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-3d/src/labels/cuboid.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/poms/modal/index.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 (16)
app/packages/looker-3d/index.d.ts (2)
1-6: Imports are correctly referenced.The necessary modules from three.js and react-three-fiber are correctly imported.
8-21: Global JSX declarations are correctly defined.The custom JSX elements for Line2, LineGeometry, LineSegments2, LineSegmentsGeometry, and LineMaterial are correctly declared.
e2e-pw/src/oss/poms/modal/leva.ts (2)
4-13: ClassModalLevaPomis correctly defined.The properties and constructor are correctly defined and initialized.
16-35: ClassLevaAsserteris correctly defined.The methods for verifying folders are correctly defined and perform the intended assertions.
app/packages/looker-3d/src/labels/line.tsx (4)
4-9: Imports are correctly referenced.The necessary modules from three.js, recoil, and react-three-fiber are correctly imported.
11-11: Extend statement is correctly defined.The custom elements are correctly registered with react-three-fiber.
26-26: State management is correctly implemented.The line width state is correctly managed using Recoil.
41-56: Rendering logic is correctly implemented.The component correctly uses
Line2,LineGeometry, andLineMaterialfor rendering.app/packages/looker-3d/src/labels/cuboid.tsx (5)
12-12: Ensure the extended components are registered correctly.The
extendfunction registers the new components globally. Verify that the components are correctly registered and used in the JSX syntax.
90-90: Ensure thelineSegments2component is correctly rendered.The
lineSegments2component should be correctly rendered within themesh. Verify its rendering in different scenarios.
69-78: Ensure the material properties are correctly applied.The
LineMaterialproperties should be correctly applied, especially theopacityandlinewidth. Verify that these properties are working as expected.Verification successful
Ensure the material properties are correctly applied.
The
LineMaterialpropertiesopacityandlinewidthare correctly applied in the following locations:
app/packages/looker-3d/src/labels/line.tsx:return new LineMaterial({ color: color, linewidth: lineWidth, opacity: opacity, });app/packages/looker-3d/src/labels/cuboid.tsx:new LineMaterial({ opacity: opacity, transparent: false, color: selected ? "orange" : color, linewidth: lineWidth, }),These properties are correctly set and used within the
LineMaterialinstances.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct application of `LineMaterial` properties. # Test: Search for the usage of `LineMaterial` and verify the properties. rg --type tsx --pattern 'LineMaterial'Length of output: 450
Script:
#!/bin/bash # Description: Verify the correct application of `LineMaterial` properties. # Test: Search for the usage of `LineMaterial` and verify the properties. rg 'LineMaterial' --type tsx -A 5Length of output: 61
Script:
#!/bin/bash # Description: Verify the correct application of `LineMaterial` properties. # Test: Search for the usage of `LineMaterial` and verify the properties. rg 'LineMaterial' --type ts -A 5Length of output: 3347
3-5: Ensure compatibility with Three.js version.The new imports from
three/examples/jsm/linesshould be compatible with the current version of Three.js used in the project. Verify that the project dependencies are updated if necessary.Verification successful
Verified Three.js version compatibility
The project uses Three.js version
^0.165.0, which supports theLineSegments2,LineMaterial, andLineSegmentsGeometryclasses fromthree/examples/jsm/lines. The imports are compatible with the specified version.
- Three.js version:
^0.165.0found inapp/packages/looker-3d/package.jsonScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the compatibility of new imports with the current version of Three.js. # Test: Check the version of Three.js used in the project. rg --type js --pattern 'three' package.jsonLength of output: 455
Script:
#!/bin/bash # Description: Verify the compatibility of new imports with the current version of Three.js. # Test: Check the version of Three.js used in the project. rg 'three' package.jsonLength of output: 76
Script:
#!/bin/bash # Description: Locate the package.json file and verify the compatibility of new imports with the current version of Three.js. # Step 1: Locate the package.json file. fd package.json # Step 2: Check the version of Three.js used in the located package.json file. fd package.json --exec rg 'three'Length of output: 806
8-9: Ensure the Recoil atom is defined and used correctly.The
cuboidLabelLineWidthAtomshould be defined in the Recoil state and used correctly. Verify its definition and usage in the state management.Verification successful
The
cuboidLabelLineWidthAtomis defined and used correctly.The
cuboidLabelLineWidthAtomis defined inapp/packages/looker-3d/src/state.tsand is used in multiple files, includingindex.tsxandcuboid.tsx. Here are the relevant locations:
Definition:
app/packages/looker-3d/src/state.ts:1export const cuboidLabelLineWidthAtom = atom<number>({ key: "fo3d-cuboidLabelLineWidth", default: 3, effects: [getBrowserStorageEffectForKey("fo3d-cuboidLabelLineWidth")], });Usages:
app/packages/looker-3d/src/labels/index.tsx:1import { cuboidLabelLineWidthAtom, polylineLabelLineWidthAtom } from "../state";app/packages/looker-3d/src/labels/index.tsx:5const [lineWidth] = useRecoilState(cuboidLabelLineWidthAtom);app/packages/looker-3d/src/labels/cuboid.tsx:1import { cuboidLabelLineWidthAtom } from "../state";These findings confirm that the atom is properly defined and utilized within the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `cuboidLabelLineWidthAtom` in the Recoil state. # Test: Search for the definition and usage of `cuboidLabelLineWidthAtom`. rg --type tsx --pattern 'cuboidLabelLineWidthAtom'Length of output: 462
Script:
#!/bin/bash # Description: Verify the definition and usage of `cuboidLabelLineWidthAtom` in the Recoil state. # Test: Search for the definition and usage of `cuboidLabelLineWidthAtom`. rg 'cuboidLabelLineWidthAtom' --type ts --type tsx -A 3Length of output: 83
Script:
#!/bin/bash # Description: Verify the definition and usage of `cuboidLabelLineWidthAtom` in the Recoil state. # Test: Search for the definition and usage of `cuboidLabelLineWidthAtom`. rg 'cuboidLabelLineWidthAtom' --glob '*.ts' --glob '*.tsx' -A 3Length of output: 1732
e2e-pw/src/oss/poms/modal/index.ts (3)
8-8: Ensure the new dependency is correctly imported.The
ModalLevaPomclass should be correctly imported and used in theModalPomclass.
21-21: Ensure the new property is correctly initialized.The
levaproperty should be correctly initialized in the constructor of theModalPomclass.
264-267: Ensure the new method is correctly defined and used.The
verifyLevaFoldersmethod should be correctly defined and used in theModalAsserterclass. Verify its usage in the test cases.
| const geometry = useMemo(() => { | ||
| return new LineSegmentsGeometry().fromLineSegments( | ||
| new THREE.LineSegments(new THREE.EdgesGeometry(geo)) | ||
| ); | ||
| }, [geo]); |
There was a problem hiding this comment.
Optimize the geometry creation.
The geometry creation can be optimized by memoizing the EdgesGeometry instance.
- const geometry = useMemo(() => {
- return new LineSegmentsGeometry().fromLineSegments(
- new THREE.LineSegments(new THREE.EdgesGeometry(geo))
- );
- }, [geo]);
+ const edgesGeo = useMemo(() => new THREE.EdgesGeometry(geo), [geo]);
+ const geometry = useMemo(() => new LineSegmentsGeometry().fromLineSegments(new THREE.LineSegments(edgesGeo)), [edgesGeo]);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 geometry = useMemo(() => { | |
| return new LineSegmentsGeometry().fromLineSegments( | |
| new THREE.LineSegments(new THREE.EdgesGeometry(geo)) | |
| ); | |
| }, [geo]); | |
| const edgesGeo = useMemo(() => new THREE.EdgesGeometry(geo), [geo]); | |
| const geometry = useMemo(() => new LineSegmentsGeometry().fromLineSegments(new THREE.LineSegments(edgesGeo)), [edgesGeo]); |
app/packages/looker-3d/src/state.ts
Outdated
| @@ -127,3 +127,15 @@ export const activeNodeAtom = atom<FoSceneNode>({ | |||
| key: "fo3d-activeNode", | |||
| default: null, | |||
| }); | |||
|
|
|||
| export const cuboidLabelLineWidthAtom = atom<number>({ | |||
There was a problem hiding this comment.
Type is implicit via the default value :)
| export const cuboidLabelLineWidthAtom = atom<number>({ | |
| export const cuboidLabelLineWidthAtom = atom({ |
app/packages/looker-3d/src/state.ts
Outdated
| export const polylineLabelLineWidthAtom = atom<number>({ | ||
| key: "fo3d-polylineLabelLineWidth", | ||
| default: 3, | ||
| effects: [getBrowserStorageEffectForKey("fo3d-polylineLabelLineWidth")], |
| @@ -22,6 +30,7 @@ export const Cuboid = ({ | |||
| color, | |||
| useLegacyCoordinates, | |||
| }: CuboidProps) => { | |||
| const [lineWidth] = useRecoilState(cuboidLabelLineWidthAtom); | |||
There was a problem hiding this comment.
nit: useRecoilValue can be used when read only
| const [lineWidth] = useRecoilState(cuboidLabelLineWidthAtom); | |
| const lineWidth = useRecoilValue(cuboidLabelLineWidthAtom); |
| readonly locator: Locator; | ||
| readonly assert: LevaAsserter; | ||
|
|
||
| constructor(page: Page, private readonly modal: ModalPom) { |
There was a problem hiding this comment.
The Page is only used by the constructor for creating a Locator. Maybe a locator of the ModalPom can be used to find "#fo-leva-container"? Also, the dataCy plus getByTestId pattern is preferred over an HTML #id e.g. here
@sashankaryal is the e2e guru, but this has been my observation :)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (4)
e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/max-line-width-scene-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/min-line-width-scene-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-chromium-linux.pngis excluded by!**/*.png,!**/*.png
Files selected for processing (6)
- app/packages/looker-3d/src/labels/cuboid.tsx (4 hunks)
- app/packages/looker-3d/src/labels/line.tsx (3 hunks)
- app/packages/looker-3d/src/state.ts (1 hunks)
- e2e-pw/src/oss/poms/modal/index.ts (3 hunks)
- e2e-pw/src/oss/poms/modal/leva.ts (1 hunks)
- e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- app/packages/looker-3d/src/state.ts
Files skipped from review as they are similar to previous changes (3)
- app/packages/looker-3d/src/labels/cuboid.tsx
- e2e-pw/src/oss/poms/modal/index.ts
- e2e-pw/src/oss/poms/modal/leva.ts
Additional context used
Path-based instructions (2)
app/packages/looker-3d/src/labels/line.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.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 (12)
app/packages/looker-3d/src/labels/line.tsx (6)
4-10: Ensure proper use of imported modules.The new imports and extension of three.js components are necessary for the new functionality. Verify that these modules are used correctly throughout the file.
26-26: Recoil state management implementation looks good.The
lineWidthis correctly obtained frompolylineLabelLineWidthAtomusinguseRecoilValue.
41-43: Geometry creation usingLineGeometrylooks good.The
geometryis correctly created usingLineGeometryandTHREE.Line.
45-51: Material creation usingLineMateriallooks good.The
materialis correctly created usingLineMaterialwithcolor,lineWidth, andopacity.
54-56: JSX return statement looks good.The return statement correctly includes a
<mesh>with a<line2>element.
11-11: JSX namespace extension looks good.The JSX namespace is correctly extended to include custom elements for
line2andlineGeometry.e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts (6)
58-60: Creation ofPolylineobject looks good.The
Polylineobject is correctly created with predefined 3D points and added to the sample under the key "polylines".
62-66: Creation ofDetectionobject looks good.The
Detectionobject is correctly created with specified location, rotation, and dimensions and added to the sample under the key "bounding_box".
67-67: Addition of sample to dataset looks good.The sample with
PolylineandDetectionobjects is correctly added to the dataset.
69-69: Computation of orthographic projection images looks good.The orthographic projection images are correctly computed for the dataset.
97-99: Assertions for default and asset folders look good.The assertions correctly verify the default and asset folders in the modal interface.
101-119: Assertions for minimum and maximum line widths look good.The assertions correctly verify the minimum and maximum line widths for the polyline and cuboid.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (4)
e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/max-line-width-scene-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/min-line-width-scene-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-chromium-linux.pngis excluded by!**/*.png,!**/*.png
Files selected for processing (2)
- e2e-pw/src/oss/poms/modal/leva.ts (1 hunks)
- e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e-pw/src/oss/poms/modal/leva.ts
Additional context used
Path-based instructions (1)
e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.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.
| await modal.leva.minSlider("Polyline Line Width"); | ||
| await modal.leva.minSlider("Cuboid Line Width"); | ||
| await expect(modal.modalContainer).toHaveScreenshot( | ||
| "min-line-width-scene.png", | ||
| { | ||
| mask, | ||
| animations: "allow", | ||
| } | ||
| ); | ||
|
|
||
| await modal.leva.maxSlider("Polyline Line Width"); | ||
| await modal.leva.maxSlider("Cuboid Line Width"); | ||
| await expect(modal.modalContainer).toHaveScreenshot( | ||
| "max-line-width-scene.png", |
There was a problem hiding this comment.
Add assertions to verify slider values.
While the slider interaction logic is correct, it would be beneficial to add assertions to verify the slider values for polyline and cuboid line widths.
+ await expect(modal.leva.getSliderValue("Polyline Line Width")).toBe(minValue);
+ await expect(modal.leva.getSliderValue("Cuboid Line Width")).toBe(minValue);
+ await expect(modal.leva.getSliderValue("Polyline Line Width")).toBe(maxValue);
+ await expect(modal.leva.getSliderValue("Cuboid Line Width")).toBe(maxValue);Committable suggestion was skipped due to low confidence.
|
|
||
| extend({ Line2, LineMaterial, LineGeometry }); | ||
|
|
There was a problem hiding this comment.
I'm assuming THREE.LineBasicMaterial didn't work here?
There was a problem hiding this comment.
Yeah Line2 requires type of "LineMaterial" as its material attribute
| return this.page.locator('div[style*="--leva-colors-folderWidgetColor"]', { | ||
| has: this.page.locator(`text="${folderName}"`), | ||
| }); |
There was a problem hiding this comment.
Substituting this.page.locator with this.locator didn't work? (basically restricting the search space)
There was a problem hiding this comment.
yeah it would just hang for whatever reason I tried a few different combinations and could only get this one working properly.
| const slider = this.page | ||
| .locator("div") |
There was a problem hiding this comment.
Same comment as above. this.locator's search space makes more sense, but if that's somehow not working, all is well.
| expect(this.modalLevaPom.getFolder(assetName)).toHaveCount(2) | ||
| ) |
There was a problem hiding this comment.
this is because the leva component has two divs containing that assets name (the asset which was added to the scene. One is the visibility dropdown and the other is the dropdown named after that assets allowing you to edit it.
| async verifyDefaultFolders() { | ||
| await Promise.all( | ||
| DEFAULT_FOLDER_NAMES.map((folderName) => | ||
| expect(this.modalLevaPom.getFolder(folderName)).toContainText( | ||
| folderName | ||
| ) | ||
| ) | ||
| ); | ||
| } |
| await modal.leva.toggleFolder("Labels"); | ||
| await modal.leva.assert.verifyDefaultFolders(); | ||
| await modal.leva.assert.verifyAssetFolders(["pcd", "stl"]); | ||
|
|
||
| await modal.leva.minSlider("Polyline Line Width"); | ||
| await modal.leva.minSlider("Cuboid Line Width"); | ||
| await expect(modal.modalContainer).toHaveScreenshot( | ||
| "min-line-width-scene.png", | ||
| { | ||
| mask, | ||
| animations: "allow", | ||
| } | ||
| ); | ||
|
|
||
| await modal.leva.maxSlider("Polyline Line Width"); | ||
| await modal.leva.maxSlider("Cuboid Line Width"); | ||
| await expect(modal.modalContainer).toHaveScreenshot( | ||
| "max-line-width-scene.png", |
What changes are proposed in this pull request?
https://voxel51.atlassian.net/browse/TEAMS-3117
Adding sliders in 3d datasets that allow us to increase / decrease the polylines width (both lines and cuboids)
These changes required us to move away from the default implementation of "Lines" offered by three.js because if you look in their notes line width changes will not be reflected in browsers running openGL due to a limitation (this applies to most all browsers). So instead we are using "Lines2" now which uses polygons to allow for lines to be resized.
How is this patch tested? If it is not, please explain why.
Launched local server using dataset quickstart-3d:
Added polylines to the first object in quickstart-3d so that I could test those using this script:
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
When working with 3D datasets you can now change polyline width within the dropdown under labels.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Documentation