feat(viewer): toggleable ground grid and feature-edge outlines - #262
philippecottier wants to merge 2 commits into
Conversation
Re-enable the drei <Grid> ground plane (already present in ThreeScene but commented out) and add a switch in the viewer overlay to show/hide it, next to the orthographic/perspective toggle. A ground grid makes it much easier to read scale and see where the Z=0 level sits under a model. - New GridToggle component (Switch + tooltip) mirroring the existing OrthographicPerspectiveToggle style. - Grid shown by default; state lives in ThreeScene.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| coloredGroup, | ||
| }: ThreeSceneProps) { | ||
| const [isOrthographic, setIsOrthographic] = useState(true); | ||
| const [showGrid, setShowGrid] = useState(true); |
There was a problem hiding this comment.
The PR specifies that both overlays are off by default, but showGrid starts as true. Every newly mounted viewer therefore displays the grid without the user enabling it. Initialize this state to false to match the stated behavior.
| const [showGrid, setShowGrid] = useState(true); | |
| const [showGrid, setShowGrid] = useState(false); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/viewer/ThreeScene.tsx">
<violation number="1" location="src/components/viewer/ThreeScene.tsx:35">
P2: The ground grid is visible immediately for every viewer, contradicting the stated off-by-default behavior. Initialize `showGrid` to `false`.</violation>
</file>
<file name="src/components/viewer/EdgesToggle.tsx">
<violation number="1" location="src/components/viewer/EdgesToggle.tsx:16">
P3: EdgesToggle duplicates the entire GridToggle component added in this same PR (42 lines each, identical structure and classes, differing only in icon, prop names, and label text). Extract a single parametrized toggle (icon, label, ariaLabel, checked, onToggle) and render both overlays from it, or the two copies will drift.</violation>
<violation number="2" location="src/components/viewer/EdgesToggle.tsx:22">
P3: The tooltip trigger is a plain `<div>` with no `tabIndex` or role, so it cannot receive keyboard focus and Radix never opens the tooltip for keyboard users. Make the trigger focusable (e.g. `tabIndex={0}`) or wrap the `Switch` itself in the `TooltipTrigger`, since the switch has the accessible label and is focusable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| coloredGroup, | ||
| }: ThreeSceneProps) { | ||
| const [isOrthographic, setIsOrthographic] = useState(true); | ||
| const [showGrid, setShowGrid] = useState(true); |
There was a problem hiding this comment.
P2: The ground grid is visible immediately for every viewer, contradicting the stated off-by-default behavior. Initialize showGrid to false.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/viewer/ThreeScene.tsx, line 35:
<comment>The ground grid is visible immediately for every viewer, contradicting the stated off-by-default behavior. Initialize `showGrid` to `false`.</comment>
<file context>
@@ -28,6 +32,8 @@ export function ThreeScene({
coloredGroup,
}: ThreeSceneProps) {
const [isOrthographic, setIsOrthographic] = useState(true);
+ const [showGrid, setShowGrid] = useState(true);
+ const [showEdges, setShowEdges] = useState(false);
</file context>
| const [showGrid, setShowGrid] = useState(true); | |
| const [showGrid, setShowGrid] = useState(false); |
| <TooltipProvider delayDuration={300}> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <div className="cursor-help"> |
There was a problem hiding this comment.
P3: The tooltip trigger is a plain <div> with no tabIndex or role, so it cannot receive keyboard focus and Radix never opens the tooltip for keyboard users. Make the trigger focusable (e.g. tabIndex={0}) or wrap the Switch itself in the TooltipTrigger, since the switch has the accessible label and is focusable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/viewer/EdgesToggle.tsx, line 22:
<comment>The tooltip trigger is a plain `<div>` with no `tabIndex` or role, so it cannot receive keyboard focus and Radix never opens the tooltip for keyboard users. Make the trigger focusable (e.g. `tabIndex={0}`) or wrap the `Switch` itself in the `TooltipTrigger`, since the switch has the accessible label and is focusable.</comment>
<file context>
@@ -0,0 +1,42 @@
+ <TooltipProvider delayDuration={300}>
+ <Tooltip>
+ <TooltipTrigger asChild>
+ <div className="cursor-help">
+ <Box className="h-4 w-4 text-adam-text-primary" />
+ </div>
</file context>
| <div className="cursor-help"> | |
| <div className="cursor-help" tabIndex={0}> |
| onToggle: (value: boolean) => void; | ||
| } | ||
|
|
||
| export function EdgesToggle({ showEdges, onToggle }: EdgesToggleProps) { |
There was a problem hiding this comment.
P3: EdgesToggle duplicates the entire GridToggle component added in this same PR (42 lines each, identical structure and classes, differing only in icon, prop names, and label text). Extract a single parametrized toggle (icon, label, ariaLabel, checked, onToggle) and render both overlays from it, or the two copies will drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/viewer/EdgesToggle.tsx, line 16:
<comment>EdgesToggle duplicates the entire GridToggle component added in this same PR (42 lines each, identical structure and classes, differing only in icon, prop names, and label text). Extract a single parametrized toggle (icon, label, ariaLabel, checked, onToggle) and render both overlays from it, or the two copies will drift.</comment>
<file context>
@@ -0,0 +1,42 @@
+ onToggle: (value: boolean) => void;
+}
+
+export function EdgesToggle({ showEdges, onToggle }: EdgesToggleProps) {
+ return (
+ <div className="flex items-center gap-2">
</file context>
Add a switch (next to the grid and orthographic toggles) that overlays feature edges on the model for a crisp technical-CAD look. Edges use EdgesGeometry at a 15 degree threshold, so only sharp/feature edges are drawn, not tessellation on curved surfaces. - Works on both render paths: drei <Edges> on the single-geometry mesh, and a matching LineSegments overlay per child mesh for the multi-mesh (coloredGroup) path, kept aligned with the model's transforms. - New EdgesToggle component mirroring the existing toggles. Off by default.
f28b0c5 to
2fb6b4a
Compare
|
@philippecottier is attempting to deploy a commit to the Adam Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks — addressed the review. The feature-edge overlay's geometries and shared material are now disposed on replacement/unmount (a useEffect keyed on the overlay), and EdgesGeometry is only built when the overlay is enabled (deferred via showEdges in the memo). groundY now checks the coloredGroup bounds before the geometry bounds, matching the render branch. Correction to the description: the ground grid is on by default and the edge overlay is off — the "both off" wording was wrong. tsc, eslint and prettier all pass. |
| if (!showEdges || !coloredGroup) return null; | ||
| coloredGroup.updateMatrixWorld(true); |
There was a problem hiding this comment.
With construction deferred until the toggle is enabled, the colored group is already mounted when child.matrixWorld is copied into each line segment. That matrix includes the viewer’s centering and Z-up-to-Y-up rotation, but the overlay is rendered under the same centering and rotation again. Enabling outlines on a displayed colored model therefore produces displaced or rotated edges instead of tracing its surface. Convert each child’s transform into colored-group-local space before applying it to the segments.
| } from '@react-three/drei'; | ||
| import * as THREE from 'three'; | ||
| import { Suspense, useMemo, useState } from 'react'; | ||
| import { Suspense, useEffect, useMemo, useState } from 'react'; |
There was a problem hiding this comment.
The modified React import remains below the external-library imports. The repository requires React first, then external libraries, then components, with blank lines between groups. Move the React import to the top and separate these groups; this explicit repository requirement must be satisfied before merging.
Context Used: .cursor/rules/code-style.mdc (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/viewer/ThreeScene.tsx">
<violation number="1" location="src/components/viewer/ThreeScene.tsx:86">
P1: When the user turns edges on after the colored model is rendered, the overlay is transformed twice and appears displaced or rotated. Build segments from transforms relative to `coloredGroup` before mounting the overlay under the same wrapper.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| }); | ||
| return group; | ||
| }, [coloredGroup, showEdges]); |
There was a problem hiding this comment.
P1: When the user turns edges on after the colored model is rendered, the overlay is transformed twice and appears displaced or rotated. Build segments from transforms relative to coloredGroup before mounting the overlay under the same wrapper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/viewer/ThreeScene.tsx, line 86:
<comment>When the user turns edges on after the colored model is rendered, the overlay is transformed twice and appears displaced or rotated. Build segments from transforms relative to `coloredGroup` before mounting the overlay under the same wrapper.</comment>
<file context>
@@ -83,7 +83,20 @@ export function ThreeScene({
});
return group;
- }, [coloredGroup]);
+ }, [coloredGroup, showEdges]);
+
+ useEffect(() => {
</file context>
1 participant
What
Two independent, user-toggleable viewer overlays.
Both are off by default and toggled from the viewer controls.
Notes
tsc -b,eslint, andprettierall pass.Summary by cubic
Adds two independent viewer overlays: a ground grid on the Z=0 plane to gauge scale and placement, and feature-edge outlines for a clearer read of geometry. The grid is on by default; edges are off, and both toggle from the viewer controls. No new dependencies are introduced.
Written for commit 2fb6b4a. Summary will update on new commits.