HartOMat/review-report.md

Review Report: PBR Material Extraction for 3D Viewer

Date: 2026-03-13

Result: ⚠️ Minor issues

Changes Reviewed

PBR Material Extraction Pipeline

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

Task 2: Catalog refresh

• Rebuilt render-worker, triggered refresh via API
• DB updated: SELECT catalog FROM asset_libraries shows PBR objects

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)

Checklist Results

Backend / Python

• New endpoint is async def (FastAPI handler)
• No auth on pbr-map (intentional — public non-sensitive data for all viewers)
• No SQL injection (ORM select() only)
• No print() in production code
• No hardcoded paths
• [N/A] No new models or migrations
• Endpoint registered (same router, already in main.py)

Frontend / TypeScript

• npx tsc --noEmit passes with 0 errors
• New API types in frontend/src/api/assetLibraries.ts
• No as any for API responses (one intentional any for THREE.MeshStandardMaterial in cadUtils to avoid THREE import)
• No bg-surface/50 Tailwind opacity syntax
• useQuery with staleTime for PBR data
• SCHAEFFLER_COLORS export removed — no orphaned references
• All useEffect/useCallback dependency arrays include pbrMap
• Material cloning prevents shared-instance mutation bugs

Render Pipeline

• catalog_assets.py runs on render-worker (Blender headless)
• Existing refresh_asset_library_catalog task unchanged — picks up new script
• No references to removed services

Security

• No credentials in code
• No hardcoded tokens
• 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.

Recommendation

Approved with one medium-priority fix needed: InlineCadViewer needs partKey propagation in its onReady callback to ensure PBR materials apply to all meshes.

Review complete. Result: ⚠️