# 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
- [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 hardcoded paths
- [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

### Security
- [x] No credentials in code
- [x] No hardcoded tokens
- [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.

## 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: ⚠️