feat(viewer): click a 3D part to insert it as an @mention, with exploded view - #261
philippecottier wants to merge 7 commits into
Conversation
…pe (key=uuid) pour que le picking survive aux reconstructions (decompose + retour)
|
@philippecottier is attempting to deploy a commit to the Adam Team on Vercel. A member of the Team first needs to authorize it. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (m && m.index === i && modules.has(m[1])) { | ||
| calls.push(m[1]); |
There was a problem hiding this comment.
For ordinary unannotated OpenSCAD source, this scanner also treats the name inside each top-level module foo(...) declaration as a module call. If declaration order differs from the order of the actual top-level calls, AMF objects receive the wrong names and colors by index. Clicking a visible part then inserts an @mention for a different part.
| if (mountedPartsGroupRef.current) | ||
| disposeColoredGroup(mountedPartsGroupRef.current); | ||
| mountedPartsGroupRef.current = group; | ||
| setPartsGroup(group); |
There was a problem hiding this comment.
A successful regeneration disposes the selected mesh and installs a new parts group without clearing selectedPart. PickableParts also retains its reference to the disposed selection because only the inner primitive is keyed. The rebuilt model therefore has no selected highlight while the viewer continues to show the old part name.
| function emissiveOf(object: THREE.Object3D | null): THREE.Color | null { | ||
| const material = (object as THREE.Mesh | null)?.material; | ||
| if (material && !Array.isArray(material) && 'emissive' in material) { | ||
| return (material as THREE.MeshStandardMaterial).emissive; |
There was a problem hiding this comment.
These helpers cast an arbitrary Object3D to Mesh and then cast its material to MeshStandardMaterial; the same pattern is repeated in materialOf, the parts traversal, and the selection handler. This violates the repository directive to avoid type casting and make types correct from the beginning. The requirement must be satisfied before merging by narrowing the Three.js objects safely or typing the pickable objects as meshes.
Rule Used: Avoid type casting and ensure types are correct from the beginning rather than casting to fix type mismatches. (source)
Learned From
Adam-CAD/desktop-backend#3
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!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 8 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/OpenSCADViewer.tsx">
<violation number="1" location="src/components/viewer/OpenSCADViewer.tsx:265">
P2: When `scadCode` changes before the new worker response arrives, this line maps the retained previous AMF geometry to the new source. Invalidate the old AMF on compile start or associate each AMF blob with the source revision that produced it before building the pickable group.</violation>
<violation number="2" location="src/components/viewer/OpenSCADViewer.tsx:273">
P2: When a new non-empty AMF result replaces a selected model, this branch leaves the old part name in the overlay until the user selects another part. Clear `selectedPart` when installing the replacement group.</violation>
</file>
<file name="src/utils/amfParser.ts">
<violation number="1" location="src/utils/amfParser.ts:40">
P2: When AMF contains an empty `<object>` before a non-empty one, this `continue` shifts `buildPartsGroup`'s `parts[i]` mapping, so names, colors, and @mentions target the wrong geometry. Preserve an empty placeholder for each object, or carry the original object index instead of filtering it here.</violation>
</file>
<file name="src/utils/partsFromAmf.ts">
<violation number="1" location="src/utils/partsFromAmf.ts:72">
P2: When a part is colored by wrapping its top-level module call, `moduleColorMap` misses the source color and the viewer replaces it with the fallback palette. Capture the color wrapper while parsing each top-level part call, not only colors inside module definitions.</violation>
<violation number="2" location="src/utils/partsFromAmf.ts:89">
P1: When SCAD defines helper modules before its top-level geometry, this scanner records the declarations as part calls. That shifts AMF objects onto helper names and can make click-to-mention select the wrong part; skip module declaration headers when collecting calls.</violation>
<violation number="3" location="src/utils/partsFromAmf.ts:135">
P3: Passing `fallbackColor` never affects a mesh because every index selects an existing palette entry first. Use the caller-provided fallback when supplied, or remove this unreachable parameter and branch.</violation>
</file>
<file name="src/worker/openSCAD.ts">
<violation number="1" location="src/worker/openSCAD.ts:247">
P2: This adds a third `-o` output (/out.amf) but the comment directly above still documents multi-output support as "two -o flags (/out.stl + /out.off)" that were verified against the 2025.03.25 vendored WASM, with a warning to re-verify if that build changes. Three simultaneous exports are a new, unverified case; if the vendored build rejects the third output or the AMF format, `callMain` returns non-zero and the whole preview throws `OpenSCADError` ("Adam did not exit correctly"), regressing all previews, not just the new parts feature. Update the comment to describe three outputs and confirm the vendored WASM accepts three `-o` flags / AMF export.</violation>
</file>
<file name="src/components/viewer/ThreeScene.tsx">
<violation number="1" location="src/components/viewer/ThreeScene.tsx:121">
P2: When a regenerated `partsGroup` arrives while the slider is settled above zero, the new meshes remain collapsed even though the slider still shows the exploded value. Apply the settled target to the current `spreadDirections`, or reset `appliedExplodeRef` whenever `group` changes.</violation>
<violation number="2" location="src/components/viewer/ThreeScene.tsx:146">
P3: If the viewer is removed while a part is hovered, the global body cursor can remain `pointer` across the rest of the application because cleanup only happens in `handleOut`. Restore the previous cursor in an effect cleanup, or manage the cursor on the viewer element instead of `document.body`.</violation>
<violation number="3" location="src/components/viewer/ThreeScene.tsx:178">
P2: Starting an OrbitControls rotation or right-clicking a part immediately selects it and inserts an `@part` mention because selection runs on `onPointerDown`. Select on a primary `onClick`, or track movement and the button before committing the selection.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| [...code.matchAll(/module\s+(\w+)\s*\(/g)].map((m) => m[1]), | ||
| ); | ||
| const calls: string[] = []; | ||
| const callRe = /(\w+)\s*\(/g; |
There was a problem hiding this comment.
P1: When SCAD defines helper modules before its top-level geometry, this scanner records the declarations as part calls. That shifts AMF objects onto helper names and can make click-to-mention select the wrong part; skip module declaration headers when collecting calls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/partsFromAmf.ts, line 89:
<comment>When SCAD defines helper modules before its top-level geometry, this scanner records the declarations as part calls. That shifts AMF objects onto helper names and can make click-to-mention select the wrong part; skip module declaration headers when collecting calls.</comment>
<file context>
@@ -0,0 +1,153 @@
+ [...code.matchAll(/module\s+(\w+)\s*\(/g)].map((m) => m[1]),
+ );
+ const calls: string[] = [];
+ const callRe = /(\w+)\s*\(/g;
+ let depth = 0;
+ let i = 0;
</file context>
| if (mountedPartsGroupRef.current) | ||
| disposeColoredGroup(mountedPartsGroupRef.current); | ||
| mountedPartsGroupRef.current = group; | ||
| setPartsGroup(group); |
There was a problem hiding this comment.
P2: When a new non-empty AMF result replaces a selected model, this branch leaves the old part name in the overlay until the user selects another part. Clear selectedPart when installing the replacement group.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/viewer/OpenSCADViewer.tsx, line 273:
<comment>When a new non-empty AMF result replaces a selected model, this branch leaves the old part name in the overlay until the user selects another part. Clear `selectedPart` when installing the replacement group.</comment>
<file context>
@@ -232,13 +238,61 @@ export function OpenSCADPreview({
+ if (mountedPartsGroupRef.current)
+ disposeColoredGroup(mountedPartsGroupRef.current);
+ mountedPartsGroupRef.current = group;
+ setPartsGroup(group);
+ })
+ .catch((err) => {
</file context>
| setPartsGroup(group); | |
| setSelectedPart(null); | |
| setPartsGroup(group); |
| .text() | ||
| .then((text) => { | ||
| if (cancelled) return; | ||
| const group = buildPartsGroup(text, partsFromScad(scadCode)); |
There was a problem hiding this comment.
P2: When scadCode changes before the new worker response arrives, this line maps the retained previous AMF geometry to the new source. Invalidate the old AMF on compile start or associate each AMF blob with the source revision that produced it before building the pickable group.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/viewer/OpenSCADViewer.tsx, line 265:
<comment>When `scadCode` changes before the new worker response arrives, this line maps the retained previous AMF geometry to the new source. Invalidate the old AMF on compile start or associate each AMF blob with the source revision that produced it before building the pickable group.</comment>
<file context>
@@ -232,13 +238,61 @@ export function OpenSCADPreview({
+ .text()
+ .then((text) => {
+ if (cancelled) return;
+ const group = buildPartsGroup(text, partsFromScad(scadCode));
+ if (group.children.length === 0) {
+ if (!cancelled) clearPartsGroup();
</file context>
| indices.push(Number(t[1]), Number(t[2]), Number(t[3])); | ||
| } | ||
|
|
||
| if (coords.length === 0 || indices.length === 0) continue; |
There was a problem hiding this comment.
P2: When AMF contains an empty <object> before a non-empty one, this continue shifts buildPartsGroup's parts[i] mapping, so names, colors, and @mentions target the wrong geometry. Preserve an empty placeholder for each object, or carry the original object index instead of filtering it here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/amfParser.ts, line 40:
<comment>When AMF contains an empty `<object>` before a non-empty one, this `continue` shifts `buildPartsGroup`'s `parts[i]` mapping, so names, colors, and @mentions target the wrong geometry. Preserve an empty placeholder for each object, or carry the original object index instead of filtering it here.</comment>
<file context>
@@ -0,0 +1,50 @@
+ indices.push(Number(t[1]), Number(t[2]), Number(t[3]));
+ }
+
+ if (coords.length === 0 || indices.length === 0) continue;
+
+ objects.push({
</file context>
| function moduleColorMap(code: string): Map<string, string> { | ||
| const defaults = colorParamDefaults(code); | ||
| const map = new Map<string, string>(); | ||
| for (const m of code.matchAll(/module\s+(\w+)\s*\([^)]*\)\s*\{/g)) { |
There was a problem hiding this comment.
P2: When a part is colored by wrapping its top-level module call, moduleColorMap misses the source color and the viewer replaces it with the fallback palette. Capture the color wrapper while parsing each top-level part call, not only colors inside module definitions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/partsFromAmf.ts, line 72:
<comment>When a part is colored by wrapping its top-level module call, `moduleColorMap` misses the source color and the viewer replaces it with the fallback palette. Capture the color wrapper while parsing each top-level part call, not only colors inside module definitions.</comment>
<file context>
@@ -0,0 +1,153 @@
+function moduleColorMap(code: string): Map<string, string> {
+ const defaults = colorParamDefaults(code);
+ const map = new Map<string, string>();
+ for (const m of code.matchAll(/module\s+(\w+)\s*\([^)]*\)\s*\{/g)) {
+ const openBrace = (m.index ?? 0) + m[0].length - 1;
+ const arg = /color\s*\(\s*(?:"([^"]+)"|(\w+))/.exec(
</file context>
| [{ path: '/out.off', key: 'off' }], | ||
| [ | ||
| { path: '/out.off', key: 'off' }, | ||
| { path: '/out.amf', key: 'amf' }, |
There was a problem hiding this comment.
P2: This adds a third -o output (/out.amf) but the comment directly above still documents multi-output support as "two -o flags (/out.stl + /out.off)" that were verified against the 2025.03.25 vendored WASM, with a warning to re-verify if that build changes. Three simultaneous exports are a new, unverified case; if the vendored build rejects the third output or the AMF format, callMain returns non-zero and the whole preview throws OpenSCADError ("Adam did not exit correctly"), regressing all previews, not just the new parts feature. Update the comment to describe three outputs and confirm the vendored WASM accepts three -o flags / AMF export.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/worker/openSCAD.ts, line 247:
<comment>This adds a third `-o` output (/out.amf) but the comment directly above still documents multi-output support as "two -o flags (/out.stl + /out.off)" that were verified against the 2025.03.25 vendored WASM, with a warning to re-verify if that build changes. Three simultaneous exports are a new, unverified case; if the vendored build rejects the third output or the AMF format, `callMain` returns non-zero and the whole preview throws `OpenSCADError` ("Adam did not exit correctly"), regressing all previews, not just the new parts feature. Update the comment to describe three outputs and confirm the vendored WASM accepts three `-o` flags / AMF export.</comment>
<file context>
@@ -242,7 +242,10 @@ class OpenSCADWrapper {
- [{ path: '/out.off', key: 'off' }],
+ [
+ { path: '/out.off', key: 'off' },
+ { path: '/out.amf', key: 'amf' },
+ ],
);
</file context>
| position={offset.toArray()} | ||
| onPointerMove={handleMove} | ||
| onPointerOut={handleOut} | ||
| onPointerDown={handleDown} |
There was a problem hiding this comment.
P2: Starting an OrbitControls rotation or right-clicking a part immediately selects it and inserts an @part mention because selection runs on onPointerDown. Select on a primary onClick, or track movement and the button before committing the selection.
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 178:
<comment>Starting an OrbitControls rotation or right-clicking a part immediately selects it and inserts an `@part` mention because selection runs on `onPointerDown`. Select on a primary `onClick`, or track movement and the button before committing the selection.</comment>
<file context>
@@ -18,6 +19,166 @@ interface ThreeSceneProps {
+ position={offset.toArray()}
+ onPointerMove={handleMove}
+ onPointerOut={handleOut}
+ onPointerDown={handleDown}
+ />
+ </group>
</file context>
| onPointerDown={handleDown} | |
| onClick={(event) => | |
| handleDown(event as unknown as ThreeEvent<PointerEvent>)} |
| if (Math.abs(diff) < 0.0005) { | ||
| if (current !== explode) { | ||
| appliedExplodeRef.current = explode; | ||
| for (const { mesh, dir } of spreadDirections) { | ||
| mesh.position.copy(dir).multiplyScalar(explode); | ||
| } | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
P2: When a regenerated partsGroup arrives while the slider is settled above zero, the new meshes remain collapsed even though the slider still shows the exploded value. Apply the settled target to the current spreadDirections, or reset appliedExplodeRef whenever group changes.
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 121:
<comment>When a regenerated `partsGroup` arrives while the slider is settled above zero, the new meshes remain collapsed even though the slider still shows the exploded value. Apply the settled target to the current `spreadDirections`, or reset `appliedExplodeRef` whenever `group` changes.</comment>
<file context>
@@ -18,6 +19,166 @@ interface ThreeSceneProps {
+ useFrame((_, delta) => {
+ const current = appliedExplodeRef.current;
+ const diff = explode - current;
+ if (Math.abs(diff) < 0.0005) {
+ if (current !== explode) {
+ appliedExplodeRef.current = explode;
</file context>
| if (Math.abs(diff) < 0.0005) { | |
| if (current !== explode) { | |
| appliedExplodeRef.current = explode; | |
| for (const { mesh, dir } of spreadDirections) { | |
| mesh.position.copy(dir).multiplyScalar(explode); | |
| } | |
| } | |
| return; | |
| } | |
| if (Math.abs(diff) < 0.0005) { | |
| appliedExplodeRef.current = explode; | |
| for (const { mesh, dir } of spreadDirections) { | |
| mesh.position.copy(dir).multiplyScalar(explode); | |
| } | |
| return; | |
| } |
| const info = parts[i]; | ||
| const colorValue: string | number = | ||
| info?.color ?? | ||
| FALLBACK_PALETTE[i % FALLBACK_PALETTE.length] ?? |
There was a problem hiding this comment.
P3: Passing fallbackColor never affects a mesh because every index selects an existing palette entry first. Use the caller-provided fallback when supplied, or remove this unreachable parameter and branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/partsFromAmf.ts, line 135:
<comment>Passing `fallbackColor` never affects a mesh because every index selects an existing palette entry first. Use the caller-provided fallback when supplied, or remove this unreachable parameter and branch.</comment>
<file context>
@@ -0,0 +1,153 @@
+ const info = parts[i];
+ const colorValue: string | number =
+ info?.color ??
+ FALLBACK_PALETTE[i % FALLBACK_PALETTE.length] ??
+ fallbackColor;
+
</file context>
| } | ||
| hovered.current = mesh; | ||
| if (mesh !== selected.current) paint(mesh, HOVER_EMISSIVE); | ||
| document.body.style.cursor = 'pointer'; |
There was a problem hiding this comment.
P3: If the viewer is removed while a part is hovered, the global body cursor can remain pointer across the rest of the application because cleanup only happens in handleOut. Restore the previous cursor in an effect cleanup, or manage the cursor on the viewer element instead of document.body.
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 146:
<comment>If the viewer is removed while a part is hovered, the global body cursor can remain `pointer` across the rest of the application because cleanup only happens in `handleOut`. Restore the previous cursor in an effect cleanup, or manage the cursor on the viewer element instead of `document.body`.</comment>
<file context>
@@ -18,6 +19,166 @@ interface ThreeSceneProps {
+ }
+ hovered.current = mesh;
+ if (mesh !== selected.current) paint(mesh, HOVER_EMISSIVE);
+ document.body.style.cursor = 'pointer';
+ };
+
</file context>
1 participant
What
Viewer features built around the generated model's named parts (from the AMF lazy-union output).
@partmention, so the next instruction can target a specific part. Selected parts get a stronger accent highlight.Notes
tsc -b,eslint, andprettierall pass.Summary by cubic
Adds clickable 3D parts to the viewer so you can target specific parts in the prompt. Hovering highlights a part, clicking inserts an
@partmention, and an exploded-view slider spreads parts apart. No new dependencies.New Features
Written for commit 14c33cb. Summary will update on new commits.