Hartmut
253f11a945
USD exporter: - Compute normals from B-Rep surface via BRepLProp_SLProps at each vertex UV parameter — eliminates faceting on curved surfaces (same as Stepper) - Add GMSH Frontal-Delaunay tessellation engine (opt-in via --tessellation_engine gmsh) with per-solid strategy matching export_step_to_gltf.py - Use vertex normal interpolation instead of faceVarying (6x smaller normals) - Default engine remains OCC (GMSH has coordinate-space bug with instanced parts) Frontend: - Fix faceted shading in InlineCadViewer: only call computeVertexNormals() when geometry lacks normals, preserving smooth GLB normals from pipeline - Add useGeometryMerge hook for draw call batching (merge by material) - Fix unused import in cadUtils, optional props in ThreeDViewer Backend: - Move dataclass import to top of step_processor.py (PEP 8) - Unified single-read STEP metadata extraction with fallback Render worker: - Fix USD import seam/sharp restoration: read primvars via pxr directly (Blender's USD importer doesn't expose custom Int2Array primvars) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3.2 KiB
Review Report: Draw Call Batching + Merge Dual STEP Parse
Date: 2026-03-13
Result: ⚠️ Minor issues
Problems Found
[backend/app/services/step_processor.py:405] dataclass import at module middle
Severity: Low
Recommendation: Move from dataclasses import dataclass, field to the top of the file with other stdlib imports. Mid-file imports work but violate PEP 8.
[frontend/src/components/cad/useGeometryMerge.ts:6] Unused import forEachMeshMaterial
Severity: Low
Recommendation: Remove forEachMeshMaterial from the import — it's imported but never called.
[frontend/src/components/cad/useGeometryMerge.ts:148] partMaterials and pbrMap in useEffect deps
Severity: Medium
Recommendation: partMaterials and pbrMap are objects that get new references on re-render. The stateRef.current early return (line 55) mitigates unnecessary re-merge, BUT the cleanup function (line 141) runs on every deps change, setting stateRef.current = null and triggering a full re-merge next render. Consider comparing by stable serialization or using refs. Not blocking because partMaterials only changes on user save, and the re-merge is fast for typical assemblies.
[backend/app/services/step_processor.py:487] ReadFile status unchecked
Severity: Low
Recommendation: status from reader.ReadFile() is captured but never validated. Not a regression — same pattern exists at lines 270 and 650 in existing code.
Checklist Results
Backend / Python
- No new endpoints — no role check needed
- No SQL injections
- Async consistency:
extract_step_metadatais sync (called from sync Celery tasks) - Uses
logger(notprint()) - No hardcoded paths
- Fallback preserves existing behavior
- [N/A] No new models or migrations
Frontend / TypeScript
tsc --noEmitpasses with 0 errors- No new API interfaces needed (pure client-side optimization)
- No Tailwind opacity syntax violations
- Icons from
lucide-reactonly
Security
- No credentials in code
- No hardcoded tokens or secrets
- English variable names and comments
Positives
- Excellent fallback strategy: Both callers try unified read first, fall back to separate reads on failure. Zero-risk deployment.
- Geometry attribute compatibility check (useGeometryMerge.ts:75-81): Correctly compares attribute sets before merging — prevents crash on mismatched attributes.
- Clean restore logic:
_restore()properly disposes merged geometry and materials, restores visibility and raycast. No memory leaks on toggle-off. - OCC.Core vs OCP dual-import handling:
_using_ocpflag with lambda wrappers for_ssuffix dispatch matches existing codebase pattern. - World transform baking (useGeometryMerge.ts:71-73): Correctly applies
matrixWorldto cloned geometry before merging. - Cloned geometry disposal (line 131): Disposes intermediate buffers after merge.
Recommendation
Approved with minor cleanup. The unused import and mid-file import are trivial fixes. The useEffect deps issue is mitigated by existing guards and only affects rapid material reassignment during perfMode — acceptable for now.
Review complete. Result: ⚠️