feat(M5-M7): embed canonical material names in USD via customData + pxr direct read
- export_step_to_usd.py: accept --material_map CLI arg, write
schaeffler:canonicalMaterialName as customData on each Mesh prim,
fix geometry transform (strip shape Location before face exploration,
apply both face_loc and shape_loc sequentially)
- import_usd.py: after Blender USD import, use pxr to read customData
directly from the USD file — builds {part_key: material_name} lookup
(Blender ignores STRING primvars and customData, but pxr reads both)
- _blender_materials.py: add apply_material_library_direct() for exact
dict-based material assignment without name-matching heuristics
- _blender_scene_setup.py: prefer direct USD lookup, fall back to
name-matching for legacy USD files without material metadata
- export_glb.py (generate_usd_master_task): resolve material_map via
material_service.resolve_material_map() and pass to subprocess;
include material hash in cache key for invalidation
- ROADMAP.md: update P5 status, add M5-M7 milestones
Tested: 3/3 parts matched (ans_lfs120), 172/175 parts matched
(F-802007.TR4-D1-H122AG). Previous: 0/25 matched.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
+78
-77
@@ -1,4 +1,4 @@
|
||||
# Review Report: P2 USD Foundation
|
||||
# Review Report — P6/P9/P10
|
||||
Date: 2026-03-12
|
||||
|
||||
## Result: ⚠️ Minor issues
|
||||
@@ -7,99 +7,100 @@ Date: 2026-03-12
|
||||
|
||||
## Problems Found
|
||||
|
||||
### [backend/app/api/routers/cad.py:537] `save_manual_material_overrides` does not set `updated_at`
|
||||
### [frontend/src/pages/OrderDetail.tsx:1698,1701] `bg-surface-hover/30` opacity syntax on CSS variable
|
||||
**Severity**: Medium
|
||||
**Detail**: `hover:bg-surface-hover/30` and `group-hover:bg-surface-hover/30` are used in two existing table-row class strings (lines 1698 and 1701). `surface-hover` is defined in `tailwind.config.js` as `'var(--color-bg-surface-hover)'` — a CSS variable. Tailwind's `/opacity` modifier requires a literal colour value (RGB or HSL), not a `var()`. The opacity modifier silently produces no effect at these two sites.
|
||||
**Note**: These two lines are pre-existing and not part of the P10-2 diff, but they are in the file changed by this PR. Flag for a cleanup pass.
|
||||
**Recommendation**: Replace with `style={{ backgroundColor: 'var(--color-bg-surface-hover)', opacity: 0.3 }}` or define a separate Tailwind colour with explicit RGBA values.
|
||||
|
||||
### [backend/app/api/routers/orders.py:1034] Redundant local import of `sql_update`
|
||||
**Severity**: Low
|
||||
**Recommendation**: Add `cad.updated_at = datetime.utcnow()` before `db.commit()`, consistent with `save_part_materials` at line 430 which does set it. The ORM `onupdate` on `CadFile.updated_at` only fires when SQLAlchemy detects a dirty column via ORM tracking. JSONB mutations are not always detected reliably.
|
||||
**Detail**: `from sqlalchemy import update as sql_update` is imported locally inside `reject_order_line` at line 1034. The module already imports `update` at the top level (line 16). The local import is harmless but inconsistent with the rest of the file.
|
||||
**Recommendation**: Remove the local import and use `update` directly (or alias it `sql_update` at the top-level import if the alias is preferred for clarity).
|
||||
|
||||
---
|
||||
|
||||
### [backend/app/api/routers/cad.py:101,336] Hardcoded `"admin"` role string
|
||||
### [frontend/src/pages/OrderDetail.tsx:reject modal] Modal marks only `latest` notification read in batch
|
||||
**Severity**: Low
|
||||
**Recommendation**: Lines 101 (`user.role.value != "admin"`) and 336 (`user.role.value != "admin"`) predate the multi-role system. These are not new in P2 but they remain in changed files. Should be migrated to `is_privileged(user)` / `require_admin()` on the next pass. Not a P2 blocker.
|
||||
**Detail**: In `NotificationCenter.tsx`, when a batch notification is clicked, only `latest.id` is passed to `markOneMutation.mutate(latest.id)`. The other notifications in the batch remain `unread`. This means the unread badge count will not decrease to zero after clicking a batch entry if the grouped notifications besides the latest are unread.
|
||||
**Recommendation**: Collect all IDs in the batch (they are consumed into `j - i` slots) and mark them all read, or call a `markAllRead` mutation instead.
|
||||
|
||||
---
|
||||
|
||||
### [frontend/src/pages/Admin.tsx:42,50] `as any[]` for admin/users and templates queries
|
||||
### [backend/app/domains/pipeline/tasks/export_glb.py] Cache miss path sets `step_file_hash` but not `render_config`
|
||||
**Severity**: Low
|
||||
**Recommendation**: These two `as any[]` casts exist in pre-P2 code but are present in the changed file. Acceptable technical debt already present before P2; not introduced by P2 itself. Document for a future typing cleanup pass.
|
||||
**Detail**: When `effective_cache_key` is not `None` and there is no stored key match (cache miss), the code updates `cad_file.step_file_hash = _current_hash` and commits. But `render_config` on the `MediaAsset` is only written _after_ tessellation succeeds (in the create/update block). This is correct — `render_config` is written on the asset once it is created. However, the early `cad_file.step_file_hash` commit duplicates work that the post-tessellation path also does implicitly (via `_current_hash`). Not a correctness bug, but the early commit is redundant for the normal path and was introduced in the refactor; it was not present in the previous version.
|
||||
**Recommendation**: Remove the two early `cad_file.step_file_hash = _current_hash; session.commit()` blocks (in both `generate_gltf_geometry_task` and `generate_usd_master_task`). The hash is already stored after tessellation completes. If the intent is to persist the hash even on a cache miss before tessellation starts, add a comment explaining why.
|
||||
|
||||
---
|
||||
|
||||
### [render-worker/scripts/export_step_to_usd.py:560] `schaeffler:partKey` stored as custom data, not a USD attribute
|
||||
**Severity**: Low (design note, not a bug)
|
||||
**Recommendation**: The implementation uses `prim.SetCustomDataByKey("schaeffler:partKey", ...)` which stores the value in the prim's `customData` dictionary rather than as a typed USD attribute. Both are valid USD patterns; `customData` is non-animatable metadata and is fine for a stable identifier used only by Python tooling. If downstream tools (e.g. Houdini LOPs) need to query it via `UsdAttributeQuery`, it would need `prim.CreateAttribute("schaeffler:partKey", Sdf.ValueTypeNames.String).Set(part_key)`. For the current use case the choice is acceptable.
|
||||
|
||||
---
|
||||
|
||||
### [render-worker/scripts/export_step_to_usd.py] No `UsdUtils.FlattenLayerStack()` call
|
||||
**Severity**: Low (checklist item — not applicable to this milestone)
|
||||
**Recommendation**: This script writes a single `.usd` layer (`Usd.Stage.CreateNew` + `stage.Save()`), so there are no sublayers to flatten. `FlattenLayerStack()` is only relevant when compositing multiple USDA/USDC layers into a delivery package. Not applicable at M1/M2 scope.
|
||||
|
||||
---
|
||||
|
||||
### [backend/app/core/tenant_context.py:70,100] New sync engine created per call
|
||||
**Severity**: Low (performance, not correctness)
|
||||
**Recommendation**: `resolve_tenant_id_for_cad()` and `resolve_tenant_id_for_order_line()` create a new `create_engine()` on every call and dispose it immediately, opening a fresh Postgres connection each time. At current task volume this is fine, but a module-level lazy singleton engine would be more efficient. Not a blocking issue for P2.
|
||||
|
||||
---
|
||||
|
||||
### [backend/app/domains/pipeline/tasks/export_glb.py:89-95] Confusing `eng.dispose()` placement in cache-hit early-return path
|
||||
**Severity**: Medium (code clarity; not a runtime bug)
|
||||
**Recommendation**: In `generate_gltf_geometry_task`, the cache-hit early-return path calls `eng.dispose()` and `return` from inside the `with Session(eng) as session:` block (lines 89-95). The Python context manager correctly closes the session before `dispose()` is reached, so this is safe. But the style is confusing — the normal path disposes the engine at line 96 after the `with` block, while the cache-hit path calls it from inside. If code is ever added after line 95, double-dispose risk increases. Recommend factoring the early-return check out of the `with` block.
|
||||
|
||||
---
|
||||
|
||||
### [frontend/src/components/cad/ThreeDViewer.tsx:536] `as any` cast for Three.js scene userData
|
||||
**Severity**: Low
|
||||
**Recommendation**: `(sceneRef.current as any).userData` is necessary due to the `@react-three/fiber` ref typing. A narrower cast `(sceneRef.current as THREE.Object3D).userData` would be more type-safe without losing any functionality.
|
||||
### [frontend/src/pages/Orders.tsx] P10-2 kanban drag-to-reject not implemented
|
||||
**Severity**: Low / informational
|
||||
**Detail**: The plan.md listed P10-2 (kanban drag-to-reject in `Orders.tsx`) as an open task. The diff contains no changes to `Orders.tsx`. This is not a regression — the feature was simply not implemented in this sprint. The per-line reject button in `OrderDetail.tsx` (P10-2 alternative) is a valid substitute for many workflows.
|
||||
**Recommendation**: Track as carry-over to next sprint; not a merge blocker.
|
||||
|
||||
---
|
||||
|
||||
## Positives
|
||||
|
||||
- **All three migrations (060-062)** are correct, additive, and include proper `downgrade()` implementations. Migration 062 uses `UPDATE ... WHERE key = ...` safely — no unexpected DROP statements.
|
||||
- **USD exporter (`export_step_to_usd.py`)** is a comprehensive 631-line implementation: full XCAF traversal, correct OCC→USD Y-up coordinate swap (`(x, -z, y)`), index-space sharp-edge primvars stored against local mesh vertex indices (not world coordinates), `metersPerUnit=0.001` set correctly, and `MANIFEST_JSON:` stdout IPC protocol for the Celery task to parse without re-opening the USD file.
|
||||
- **`pxr` import is from `usd-core`** — correctly installed via `pip3 install "usd-core>=24.11"` in the Dockerfile.
|
||||
- **Celery task queue placement** is correct: both `generate_gltf_geometry_task` and `generate_usd_master_task` are on `thumbnail_rendering` (concurrency=1), ensuring no parallel Blender/OCC conflicts.
|
||||
- **`PipelineLogger` used** at the top of both new tasks with `self.request.id`. No `render_job_doc.celery_task_id` pattern needed here — that is specific to the `WorkflowRun` system; consistent with existing pipeline task patterns.
|
||||
- **Auth on all new endpoints**: `scene-manifest` (GET) requires `get_current_user`; `generate-usd-master` (POST) and `manual-material-overrides` (PUT) require `is_privileged(user)`. GET `manual-material-overrides` requires auth but not privilege — correct for read access.
|
||||
- **No SQL injections**: all DB access uses SQLAlchemy ORM or parameterized `text()` queries.
|
||||
- **Pydantic validation**: `ManualMaterialOverridesIn`, `PartEntry`, `SceneManifest` models validate all POST/PUT inputs.
|
||||
- **No hardcoded credentials or secrets** anywhere in the changed files.
|
||||
- **All English identifiers and comments** throughout.
|
||||
- **No `print()` in backend Python** — render-worker scripts use stdout as the IPC protocol with the Celery task, which is the correct approach.
|
||||
- **`storage_key` values are relative**: the `_prefix` strip logic in `export_glb.py` correctly strips `upload_dir` before storing.
|
||||
- **`_inject_glb_extras()` called after `RWGltf_CafWriter` export** — satisfies the render pipeline checklist item.
|
||||
- **Frontend types**: `SceneManifest`, `PartEntry`, `ManualMaterialOverridesResponse` all have proper TypeScript interfaces in `sceneManifest.ts` and `cad.ts`. No `as any` in the new API surface code.
|
||||
- **Loading states** with `isPending` and error feedback with `toast.error()` present in all new UI mutations.
|
||||
- **`effectiveMaterials` merge** in ThreeDViewer correctly combines legacy `partMaterials` (mesh-name keyed) and new `manualOverrides` (partKey keyed), providing backward compatibility for pre-P2 GLBs.
|
||||
- **`build_scene_manifest()` four-layer priority** (manual → auto → source → default) correctly implemented per spec.
|
||||
- **`generate_part_key()` algorithm duplicated consistently** between `export_step_to_usd.py` and `part_key_service.py` — same `_AF_RE` strip, same deduplication loop. Keys generated by the USD exporter match keys computed by the API service.
|
||||
- **New admin bulk-action endpoints** use `require_admin`, batch-fetch existing asset IDs efficiently (single query), and queue only missing items.
|
||||
- **`step_tasks.py` shim** correctly re-exports `generate_usd_master_task` for Celery task name resolution.
|
||||
- **New `tenant_context.py`** provides clean, documented sync helpers for RLS in Celery tasks, using parameterized `SET LOCAL` queries to prevent injection.
|
||||
- **`vite-env.d.ts`** fix (`/// <reference types="vite/client" />`) correctly resolves the `ImportMeta.env` TypeScript error without over-typing.
|
||||
- **`GPUProbeResult` interface extended** (`timestamp`, `devices`, `render_time_s`) without breaking existing callers — optional fields only.
|
||||
### P6-2 — Admin.tsx progressive disclosure toggle
|
||||
- `showAdvancedTess` state hides the four manual deflection inputs behind an "Advanced: manual deflection values" toggle using `ChevronRight`/`ChevronDown` icons from the existing lucide-react import. No new dependencies.
|
||||
- The Save button is correctly placed **outside** the `{showAdvancedTess && ...}` block — settings can be saved without expanding the advanced section, as required.
|
||||
- Help text added to six inputs across the viewer settings section: both `title` attribute (browser tooltip) and a `<p className="text-xs text-content-muted">` description. All descriptions are accurate.
|
||||
- Label renamed from "Scene / Viewer" to "Scene (USD Master)" — correctly reflects the post-P9 architecture where the geometry GLB is derived from the USD pipeline.
|
||||
|
||||
---
|
||||
### P9-1/P9-2 — Composite cache key in export_glb.py
|
||||
- Cache key is correctly formed as `{hash}:{linear_deflection}:{angular_deflection}:{tessellation_engine}` for the GLB task and `{hash}:{linear_deflection}:{angular_deflection}:{sharp_threshold}` for the USD master task — all settings that affect the tessellation output are included.
|
||||
- Disk existence check added before returning a cache hit: `if _asset_disk_path.exists()` prevents stale DB records from silently skipping re-generation when the file was deleted from disk. This is a meaningful correctness improvement over the previous version.
|
||||
- `render_config = {"cache_key": effective_cache_key}` is stored on both `create` and `update` code paths — consistent.
|
||||
- `linear_deflection`/`angular_deflection`/`tessellation_engine` variables are now read **inside** the `with Session` block (before the cache check), which means they are available for the cache key computation. The previous placement (after the `with` block) would have required reading settings twice. Clean refactor.
|
||||
- `render_config` field confirmed present on `MediaAsset` model at `backend/app/domains/media/models.py:51` (`Mapped[dict | None] = mapped_column(JSONB, nullable=True)`).
|
||||
|
||||
## Summary of Issues
|
||||
### P9-3 — `step_hash` exposed in CAD API response
|
||||
- `"step_hash": cad.step_file_hash` added to the `get_objects` endpoint at `cad.py:232`. Field is nullable; no schema change required. Clean, minimal addition.
|
||||
- `logger = logging.getLogger(__name__)` added at module level (was missing before) — bonus improvement.
|
||||
|
||||
| # | File | Line | Severity | Description |
|
||||
|---|------|------|----------|-------------|
|
||||
| 1 | `cad.py` | 537 | Low | `updated_at` not set on `save_manual_material_overrides` |
|
||||
| 2 | `cad.py` | 101, 336 | Low | Pre-existing hardcoded `"admin"` role strings (not introduced by P2) |
|
||||
| 3 | `Admin.tsx` | 42, 50 | Low | Pre-existing `as any[]` casts (not introduced by P2) |
|
||||
| 4 | `export_step_to_usd.py` | 560 | Low | `schaeffler:partKey` in `customData` vs typed attribute (design choice, acceptable) |
|
||||
| 5 | `export_step_to_usd.py` | — | Low | No `FlattenLayerStack()` — not applicable at current scope |
|
||||
| 6 | `tenant_context.py` | 70, 100 | Low | New sync engine per call (performance concern only) |
|
||||
| 7 | `export_glb.py` | 89-95 | Medium | Confusing `eng.dispose()` in cache-hit early-return inside `with` block |
|
||||
| 8 | `ThreeDViewer.tsx` | 536 | Low | `as any` for scene userData (could use `THREE.Object3D`) |
|
||||
### P10-1 — Notification batching in NotificationCenter.tsx
|
||||
- `groupNotifications` helper is self-contained, pure, and operates on the already-fetched `data.items` array. No backend round-trips.
|
||||
- Grouping correctly requires: same `entity_id`, same render action class (`render.completed` or `render.failed`), and timestamps within 5 minutes. The `Math.abs(tM - t0) > 5 * 60 * 1000` guard prevents grouping renders that arrived far apart.
|
||||
- `failed` and `done` counters tracked separately — batch label shows `"Render batch: 3 done, 1 failed"` when mixed, which is more informative than a single count.
|
||||
- Batch row uses `AlertTriangle` for any failures, `CheckCircle` when all succeeded — correct visual priority.
|
||||
- `createPortal` used for single-line reject modal — addresses the checklist requirement for modal inside a `<tr>`.
|
||||
|
||||
No critical or blocking issues. All new endpoints are properly secured, migrations are clean, the USD exporter correctly authors index-space primvars and sets `schaeffler:partKey` metadata, and the frontend type coverage of the new API surface is complete.
|
||||
### P10-2 — Per-line reject button and modal in OrderDetail.tsx
|
||||
- `createPortal(…, document.body)` correctly escapes the `<tr>` context. Modal has both backdrop-click and X-button close.
|
||||
- `rejectLineMut` invalidates `['order', orderId]` on success — optimistic cache invalidation is correct.
|
||||
- `canRejectLine` guard (`isPrivileged && line.item_status !== 'rejected'`) prevents double-reject. Already-rejected lines do not show the button.
|
||||
- `rejectOrderLine` in `orders.ts` has a typed return interface (`{ rejected: boolean; line_id: string; reason: string }`) — no `as any` for the new function.
|
||||
- Backend `reject_order_line` endpoint: permission check runs before any DB queries (`_is_privileged` returns false for `client` role). `order_id` used in the `OrderLine.where` clause so a user cannot reject a line belonging to a different order by guessing `line_id`. No SQL injection vectors.
|
||||
- `item_status = "rejected"` is a valid `ItemStatus` enum value (confirmed at `domains/orders/models.py:53`).
|
||||
- `notes` column confirmed present and nullable on `OrderLine` (the model imported at the top of `orders.py` from `app.models.order_line`).
|
||||
|
||||
### P6-1 — MediaAssetType deprecation comments
|
||||
- `gltf_geometry` and `gltf_production` enum values annotated with inline `# DEPRECATED` comments describing the replacement (`usd_master`). Correct and appropriately concise — preserves backward compatibility while signalling intent.
|
||||
|
||||
### Migration 6ebfe2737531 — Tessellation key rename
|
||||
- Renames `gltf_production_linear_deflection` → `scene_linear_deflection` and `gltf_production_angular_deflection` → `scene_angular_deflection` in `system_settings`.
|
||||
- No stale references to the old keys found anywhere in backend or render-worker code.
|
||||
- `downgrade()` correctly reverses both renames.
|
||||
|
||||
### admin.py — `require_admin` → `require_global_admin` migration
|
||||
- All 14 admin router function dependencies updated. `require_global_admin` is defined in `auth.py` as the authoritative check; `require_admin` preserved as a backward-compat alias.
|
||||
|
||||
### render_blender.py — `tessellation_engine` parameter threading
|
||||
- `_glb_from_step`, `render_still`, and `render_turntable_to_file` all accept `tessellation_engine` with default `"occ"`. Passed through to the subprocess CLI flag `--tessellation_engine`. Consistent with `step_processor._get_all_settings` which now includes `"tessellation_engine": "occ"` as a fallback default.
|
||||
|
||||
### export_step_to_usd.py — Blender mesh prim naming fix
|
||||
- `mesh_path = f"{part_path}/{part_key}"` (was `f"{part_path}/Mesh"`). Blender 5.0 collapses single-child Xform+Mesh into the leaf prim name — using `part_key` as the leaf name means the imported object name equals the canonical part key with no post-import rename step.
|
||||
- Learning documented in `LEARNINGS.md` with the precise Blender 5.0 USD import behaviour.
|
||||
|
||||
### General
|
||||
- No SQL injection vectors. All DB writes use ORM or parameterized bulk-update.
|
||||
- No `print()` introduced in backend Python files. Render-worker scripts use `print()` as their logging channel (subprocess output captured by the caller) — consistent with the rest of those scripts.
|
||||
- No hardcoded file paths or credentials in any changed file.
|
||||
- All FastAPI route handlers `async def`; all Celery tasks `def`. Async consistency maintained.
|
||||
- LEARNINGS.md and ROADMAP.md updated in the same change set.
|
||||
|
||||
---
|
||||
|
||||
## Recommendation
|
||||
|
||||
Approved. Fix issue #1 (`updated_at` missing in `save_manual_material_overrides`) and issue #7 (refactor `eng.dispose()` in cache-hit path for clarity) before next production deployment. All remaining items are Low severity and can be addressed in a follow-up cleanup commit.
|
||||
Approve with the redundant local `sql_update` import cleaned up (Low) and the pre-existing `bg-surface-hover/30` opacity issue tracked for a follow-up pass (Medium, not introduced by this PR). The unread-badge issue in notification batching and the missing P10-2 kanban drag are carry-over tasks — neither is a regression. No blockers to merging.
|
||||
|
||||
---
|
||||
|
||||
Review complete. Result: ⚠️
|
||||
|
||||
Reference in New Issue
Block a user