Hartmut
47b5d42bb5
M1 — dead code removed: - Delete blender-renderer/ and threejs-renderer/ source files - Remove PIL/Pillow fallback block from step_processor.py (_generate_thumbnail_placeholder, _finalise_image JPG path) - Remove stl_quality param from render_blender.py, render_still_task, render_turntable_task (was always "low"; hardcode deflection values) - render_turntable_task now reads scene_linear/angular_deflection from system_settings (consistent with export_glb.py pipeline) M3 — blender_render.py split from 263 → 68 lines: - _blender_args.py: parse_args() — all 25 positional + named args - _blender_scene_setup.py: setup_scene() — MODE A/B including USD import - _blender_render_config.py: configure_and_render() — engine + output Post-review fixes: - _db_engine.dispose() after settings read in render_turntable_task - _finalise_image() fmt param removed (always PNG; PIL never installed) - _blender_import.py committed together with new submodules to satisfy import_usd_file dependency Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
51 lines
4.6 KiB
Markdown
51 lines
4.6 KiB
Markdown
# Review Report: P1 Remaining Cleanup — M1 Dead Code + M3 blender_render.py Split
|
||
Date: 2026-03-12
|
||
|
||
## Result: ⚠️ Minor issues
|
||
|
||
---
|
||
|
||
## Problems Found
|
||
|
||
### [_blender_scene_setup.py:12] `import_usd_file` depends on uncommitted `_blender_import.py` change
|
||
**Severity**: Medium
|
||
**Description**: `_blender_scene_setup.py` imports `import_usd_file` from `_blender_import` (line 12). This function does NOT exist in the committed `_blender_import.py` at HEAD — it is only present in a pre-existing uncommitted modification to that file (`_blender_import.py` shows `??` is untracked/modified). If the new submodule files (`_blender_args.py`, `_blender_scene_setup.py`, `_blender_render_config.py`) are committed without including the `_blender_import.py` change, every Blender render will crash with `ImportError: cannot import name 'import_usd_file'`.
|
||
**Recommendation**: Ensure `render-worker/scripts/_blender_import.py` (with the `import_usd_file` addition) is staged and committed in the same commit as the new submodule files.
|
||
|
||
---
|
||
|
||
### [step_processor.py:561] `_finalise_image()` ignores `fmt` parameter — always produces `.png`
|
||
**Severity**: Low
|
||
**Description**: The new `_finalise_image(src, dst, fmt)` accepts `fmt` but always does `out = dst.with_suffix(".png")`. When `thumbnail_format = "jpg"` is configured in system settings, the callers compute `final_path` with a `.jpg` extension and pass `fmt="jpg"`, but the returned path will always have `.png` extension. The `fmt` parameter is silently ignored.
|
||
|
||
This is a deliberate consequence of removing PIL (which was never installed anyway), so the behavior change is acceptable. However, the dead `fmt` parameter creates misleading API.
|
||
**Recommendation**: Remove the `fmt` parameter from `_finalise_image()` and update all callers to drop it. This makes the intent clear: format conversion is not supported.
|
||
|
||
---
|
||
|
||
### [domains/rendering/tasks.py:210–215] New SQLAlchemy engine created per `render_turntable_task` call
|
||
**Severity**: Low
|
||
**Description**: `render_turntable_task` now calls `_create_engine(app_settings.database_url_sync)` inside the task body. This creates a new connection pool on every invocation. For a low-frequency task this is acceptable, but it leaks connection pool resources if the task runs frequently.
|
||
**Recommendation**: The `_db_engine` should be disposed after use. Add `_db_engine.dispose()` after the `with _Session(_db_engine) as _s:` block closes.
|
||
|
||
---
|
||
|
||
## Positives
|
||
|
||
- **All 7 plan tasks complete**: blender_render.py correctly reduced from 263 → 68 lines (meets ≤ 80 target).
|
||
- **`stl_quality` fully removed**: Zero matches in `render_blender.py`, `step_processor.py`, `domains/rendering/tasks.py` — clean sweep.
|
||
- **PIL fallback completely excised**: `_generate_thumbnail_placeholder()` and `from PIL import Image` both gone; the 68-line Pillow-placeholder function is deleted, reducing `step_processor.py` by ~70 lines.
|
||
- **Submodule separation is clean**: `_blender_args.py`, `_blender_scene_setup.py`, `_blender_render_config.py` follow the existing `_blender_gpu.py` / `_blender_import.py` / `_blender_scene.py` naming and import pattern exactly.
|
||
- **`_blender_args.py` fidelity**: All 25 positional args correctly mapped; named args `--mesh-attributes` and `--usd-path` preserved; `use_template` bool computed and included in namespace; template file existence check preserved.
|
||
- **`_blender_scene_setup.py` correctness**: MODE A and MODE B correctly separated; shadow catcher, lighting_only, auto-camera, and material library logic all preserved faithfully; `needs_auto_camera` conditional for MODE B correctly omits `setup_auto_lights` (template provides its own lights).
|
||
- **`_blender_render_config.py` correctness**: `configure_engine` signature matches exactly; colour management block (`Standard`/`None` look) correctly gated on `not use_template`; CYCLES verification block preserved.
|
||
- **`_lap` closure works correctly**: `_lap` is defined in `blender_render.py`, captures `_t0` and `_timings` from outer scope, passed as callback to both submodules — timing summary at end of `blender_render.py` correctly reflects labels from all phases.
|
||
- **`render_turntable_task` GLB path fixed**: Changed from `f"{stem}_{stl_quality}.glb"` (always `_low.glb`) to `{stem}_thumbnail.glb` — now consistent with `render_blender.py` service cache key.
|
||
- **Security**: No hardcoded credentials, no SQL injection vectors. Raw SQL in `render_turntable_task` is a parameterless `SELECT` with no user input.
|
||
|
||
---
|
||
|
||
## Recommendation
|
||
|
||
Fix the `_db_engine.dispose()` omission (low, 1 line) and commit `_blender_import.py` together with the new submodule files. The `fmt` parameter cleanup is optional polish. Fix inline and re-apply without re-review.
|