feat: surface-evaluated normals, GMSH tessellation, draw call batching

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>
This commit is contained in:
2026-03-13 15:14:23 +01:00
parent 6c5873d51f
commit 253f11a945
8 changed files with 977 additions and 166 deletions
+31 -89
View File
@@ -1,117 +1,59 @@
# Review Report: PBR Material Extraction for 3D Viewer
# Review Report: Draw Call Batching + Merge Dual STEP Parse
Date: 2026-03-13
## Result: ⚠️ Minor issues
## Changes Reviewed
## Problems Found
### PBR Material Extraction Pipeline
### [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.
#### Task 1: catalog_assets.py — PBR extraction from Principled BSDF
- Extracts `base_color` (linear→sRGB via correct IEC 61966-2-1 formula), `metallic`, `roughness`, `transmission`, `ior`
- Fallback to `mat.diffuse_color` when no Principled BSDF node found
- Handles both Blender 4.0+ (`Transmission Weight`) and older (`Transmission`) input names
- Verified: 35 materials extracted with correct PBR values
### [frontend/src/components/cad/useGeometryMerge.ts:6] Unused import `forEachMeshMaterial`
**Severity**: Low
**Recommendation**: Remove `forEachMeshMaterial` from the import — it's imported but never called.
#### Task 2: Catalog refresh
- Rebuilt render-worker, triggered refresh via API
- DB updated: `SELECT catalog FROM asset_libraries` shows PBR objects
### [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.
#### Task 3: GET /api/asset-libraries/pbr-map endpoint
- Public (no auth) — correct for viewer data
- Placed before `/{lib_id}` to avoid UUID collision
- 1h cache header (`Cache-Control: public, max-age=3600`)
- Handles old string-format catalogs gracefully (skips them)
- Returns empty dict when no active library
#### Task 4: Frontend API types
- `MaterialPBR`, `MaterialPBRMap` types correctly model backend response
- `AssetLibraryCatalog.materials` updated to union type for backwards compat
- `Admin.tsx` and `AssetLibrary.tsx` updated to handle both string and object formats
#### Task 5: cadUtils PBR helpers
- `applyPBRToMaterial()`: sets color, metalness, roughness, approximate transmission via opacity
- `pbrColorHex()`: converts base_color to hex string for UI
- `previewColorForEntry()`: moved from MaterialPanel, now uses dynamic PBR lookup with optional `pbrMap` param
#### Tasks 6-7: ThreeDViewer + InlineCadViewer
- Both fetch PBR map via `useQuery` with 5min staleTime
- Material application clones materials before modifying (prevents shared-instance bugs)
- `_pbrApplied` flag prevents redundant cloning on re-renders
- `pbrMap` added to all dependency arrays
#### Task 8: MaterialPanel
- Hardcoded `SCHAEFFLER_COLORS` (17 entries) removed
- Dynamic PBR lookup covers all 35 materials
- Shows M:/R: values (metallic/roughness) in preview swatch
- Accepts optional `pbrMap` prop (avoids duplicate fetch when parent already has it)
- Falls back to own `useQuery` fetch when prop not provided (enabled: !pbrMapProp)
### [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
- [x] New endpoint is `async def` (FastAPI handler)
- [x] No auth on pbr-map (intentional — public non-sensitive data for all viewers)
- [x] No SQL injection (ORM `select()` only)
- [x] No `print()` in production code
- [x] No new endpoints — no role check needed
- [x] No SQL injections
- [x] Async consistency: `extract_step_metadata` is sync (called from sync Celery tasks)
- [x] Uses `logger` (not `print()`)
- [x] No hardcoded paths
- [x] Fallback preserves existing behavior
- [N/A] No new models or migrations
- [x] Endpoint registered (same router, already in main.py)
### Frontend / TypeScript
- [x] `npx tsc --noEmit` passes with 0 errors
- [x] New API types in `frontend/src/api/assetLibraries.ts`
- [x] No `as any` for API responses (one intentional `any` for THREE.MeshStandardMaterial in cadUtils to avoid THREE import)
- [x] No `bg-surface/50` Tailwind opacity syntax
- [x] `useQuery` with staleTime for PBR data
- [x] `SCHAEFFLER_COLORS` export removed — no orphaned references
- [x] All `useEffect`/`useCallback` dependency arrays include `pbrMap`
- [x] Material cloning prevents shared-instance mutation bugs
### Render Pipeline
- [x] `catalog_assets.py` runs on render-worker (Blender headless)
- [x] Existing `refresh_asset_library_catalog` task unchanged — picks up new script
- [x] No references to removed services
- [x] `tsc --noEmit` passes with 0 errors
- [x] No new API interfaces needed (pure client-side optimization)
- [x] No Tailwind opacity syntax violations
- [x] Icons from `lucide-react` only
### Security
- [x] No credentials in code
- [x] No hardcoded tokens
- [x] No hardcoded tokens or secrets
- [x] English variable names and comments
## Verification
| Gate | Result |
|------|--------|
| `npx tsc --noEmit` | 0 errors ✅ |
| `curl /api/asset-libraries/pbr-map` | 35 materials with PBR ✅ |
| `grep "SCHAEFFLER_COLORS" frontend/src/` | 0 references (only in comment) ✅ |
| `grep "previewColorForEntry" frontend/src/` | 7 matches (all pass pbrMap) ✅ |
| `grep "applyPBRToMaterial" frontend/src/` | 4 matches (definition + 3 usages) ✅ |
## Problems Found
### [cadUtils.ts:137] `applyPBRToMaterial` uses `any` type for mat parameter
**Severity**: Low
**Description**: The `mat` parameter is typed as `any` to avoid importing THREE in the utility module. This is documented with a comment and eslint-disable. The actual callers always pass `THREE.MeshStandardMaterial`.
**Recommendation**: Acceptable tradeoff. Alternative would be importing THREE types, but cadUtils is intentionally dependency-light.
### [InlineCadViewer] No partKey propagation from parent Group in onReady
**Severity**: Medium
**Description**: The InlineCadViewer's `onReady` callback doesn't propagate partKey from parent Group to child Mesh (unlike ThreeDViewer which does this at line ~540). This means `mesh.userData.partKey` may be undefined for child meshes of multi-primitive nodes, and the PBR application falls back to `resolvePartKey(normalizeMeshName(...))` which may not match if partKeyMap is empty.
**Impact**: PBR materials may not apply correctly in InlineCadViewer for products where the partKey was set on the parent Group but not propagated to children. This could explain the user's report of "no material changes visible."
**Recommendation**: Add the same parent-Group partKey propagation logic to InlineCadViewer's onReady callback, matching ThreeDViewer's pattern.
## Positives
1. **Correct color space handling**: linear→sRGB conversion uses the IEC 61966-2-1 formula (not simplified gamma), ensuring accurate color reproduction.
2. **Backwards compatible**: Old catalogs (string arrays) are gracefully skipped. Empty partKeyMap returns identity fallback.
3. **Smart caching**: PBR data rarely changes — 5min staleTime + 1h server cache is appropriate.
4. **Material cloning**: `_pbrApplied` flag prevents double-cloning on re-renders.
5. **35 materials covered**: All Schaeffler standard materials now have PBR data (vs 17 hardcoded colors before).
6. **Principled BSDF fallback**: Materials without the node still get viewport display color.
1. **Excellent fallback strategy**: Both callers try unified read first, fall back to separate reads on failure. Zero-risk deployment.
2. **Geometry attribute compatibility check** (useGeometryMerge.ts:75-81): Correctly compares attribute sets before merging — prevents crash on mismatched attributes.
3. **Clean restore logic**: `_restore()` properly disposes merged geometry and materials, restores visibility and raycast. No memory leaks on toggle-off.
4. **OCC.Core vs OCP dual-import handling**: `_using_ocp` flag with lambda wrappers for `_s` suffix dispatch matches existing codebase pattern.
5. **World transform baking** (useGeometryMerge.ts:71-73): Correctly applies `matrixWorld` to cloned geometry before merging.
6. **Cloned geometry disposal** (line 131): Disposes intermediate buffers after merge.
## Recommendation
Approved with one medium-priority fix needed: InlineCadViewer needs partKey propagation in its onReady callback to ensure PBR materials apply to all meshes.
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: ⚠️