Auto-fixes for pim/feat/scene-cooking#2696
Open
paul-nechifor wants to merge 30 commits into
Open
Conversation
except Exception: pass silently dropped visual-write, convex-hull, and mesh-simplification failures, and buried simplification failures at debug level (hidden by default). Catch the specific geometry-library failure modes instead and log a warning so real problems surface.
except Exception buried every primitive-fit failure at debug (hidden by default) and could swallow programming errors along with expected degenerate-geometry failures. Catch the specific numpy/geometry exceptions and log at warning.
_run_coacd wrapped `import coacd` in the same except as the actual decomposition call, so a missing dependency was reported as "CoACD failed" and silently downgraded to a single-hull fallback. Import coacd outside the try and narrow the except to the fitting call.
collision_policy.py and entities/collision.py each reimplemented "silence once" via a function-attribute global flag guarded by type: ignore[attr-defined]. coacd.set_log_level is cheap, so replace both with a shared silence_coacd_logging() called unconditionally.
GeomEmission was superseded by PrimDecision and never instantiated or imported anywhere. The module and PrimDecision docstrings still described a nonexistent emit_for_prim() dispatcher and GeomEmission records; point them at the real decide_for_prim()/PrimDecision API.
open3d / open3d-unofficial-arm are unconditional [project.dependencies] (needed by the package regardless), so re-declaring them under the scene extra had no effect beyond a misleading comment implying the extra provides open3d.
planning.py repeated the same unnamed 1e-4 clamp expression verbatim at two call sites. Name it _MIN_EXTENT_M and factor the clamp into a shared _safe_extents() helper.
The ray origin height and its depth-reconstruction offset were two separate 1000.0 literals that must stay equal. Name it once so the relationship is explicit instead of implicit.
Hoist the repeated 4-byte alignment literal to GLB_ALIGNMENT and name the PNG IHDR bit-depth byte offset/threshold used by _is_high_bit_depth_png so the intent is explicit instead of bare 25/24/8 literals.
except Exception wrapped Image.open/load/convert/save, hiding programming errors behind a generic RuntimeError. Only Image.open and image.load() can actually fail on bad texture data (OSError, which covers PIL.UnidentifiedImageError); scope the catch to those calls.
_write_test_glb hardcoded the header size, chunk header size, magic, version, and chunk type values that already exist as public constants in glb.py, duplicating the format definition instead of testing against it.
The "rerun" entry in _BROWSER_VISUAL_PROFILES restated every BrowserVisualSpec default value-for-value. Fall through to the dataclass defaults instead; browser_visual_spec_for_target still validates "rerun" as a known target via BROWSER_VISUAL_TARGETS.
Worker cap, progress-log thresholds, viewer bounds padding, and the visual zfar extent multiplier/floor were unnamed literals; give them names so the tuning knobs are discoverable.
collision_export.py and normalize.py each hardcoded ~/.cache/dimos/..., ignoring XDG_CACHE_HOME. Add a shared CACHE_DIR to dimos/constants.py (matching the existing STATE_DIR pattern) and base both cache dirs on it.
…kipping pytest.importorskip made these tests silently pass-by-skipping when a scene-cooking dependency wasn't installed. Import both normally so collection errors loudly instead.
`object` doesn't support attribute access, so callers had to sprinkle `# type: ignore[attr-defined]`/`[union-attr]` at every use site. These values are genuinely untyped JSON-ish data, which is what Any is for; switching removes the ignores entirely.
numpy is a core dependency, not a heavy/lazy one; the two inline imports inside cook_entity_collision_hulls and _run_coacd had no justifying comment for staying local.
…t.py json is stdlib and scipy is a core dependency (collision_policy.py already imports it at top level); the four inline imports had no justifying "lazy heavy dep" comment.
mujoco lives in the sim extra, not scene, so keeping the import inline is intentional (browser/rerun-only cooks shouldn't require it) -- just missing the comment explaining why, unlike other lazy imports in this package.
open3d, an equally heavy dependency, is already imported at module scope in this file, so keeping trimesh lazy in _write_glb with no comment was inconsistent.
Explain why this one import stays function-local: it avoids pulling open3d/trimesh into every other test in this module at collection time.
sidecar.py, cook.py, and normalize.py used %s-style logger calls while structured key/value logging dominates the rest of the repo.
_load_usd_mesh and load_scene_prims each ran an identical nested Python loop (per-face, per-triangle) to fan-triangulate USD n-gons -- slow on large meshes. Extract one shared _fan_triangulate() that does the same work with vectorized numpy indexing. Verified against the original nested-loop logic on 200 randomized face_counts/face_verts inputs (including degenerate 0/1/2-gons) and against a live pxr.Usd stage with a mixed quad+triangle mesh.
Verified against the full-project mypy run (ScenePrimMesh.vertices/ triangles are consumed across the whole scene_cooking package) and against a live pxr.Usd smoke test.
Traced each function's actual dtype from its call sites (float32 for hull geometry, float64 for the visual passthrough / scene-bounds path, mixed float precision in _write_mesh_obj which both feed). Verified with the full-project mypy run and a runtime smoke test of the hull/box/OBJ-writer helpers.
Traced each function's dtype from decide_for_prim's single call site (collision_export.py always passes float64 vertices / int32 triangles). Hull-producing paths (CoACD, sheet prisms, single-hull pass-through) mix float32/float64 vertex precision by design, so PrimDecision.hulls and _coacd_decompose keep NDArray[np.floating[Any]] where that's genuine. Verified with the full-project mypy run and a runtime smoke test of decide_for_prim.
paul-nechifor
requested review from
leshy,
mustafab0 and
spomichter
as code owners
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Contributor
Greptile SummaryThis is an automated cleanup PR for the
Confidence Score: 4/5Safe to merge for the vast majority of scenes; the only real risk is that a handful of exception narrowings could let unexpected errors propagate rather than silently fall back to coarser geometry. Most changes are mechanical improvements (type annotations, constant extraction, lazy-import promotion, structlog keyword style). The vectorized dimos/experimental/scene_cooking/mujoco/collision_export.py — specifically Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cook_scene_package] --> B[_cook_entity_collision]
A --> C[bake_scene_mjcf]
B --> D[cook_entity_collision_hulls]
D --> E["import coacd (hard requirement)"]
E --> F{coacd.run_coacd}
F -->|"AssertionError / RuntimeError / ValueError"| G["return empty list - single convex hull"]
F -->|success| H[return hull parts]
C --> I["_bake_prims (ProcessPoolExecutor)"]
I --> J[_process_one_prim]
J --> K[_simplify_mesh_geom]
K -->|RuntimeError| L[convex hull fallback]
K -->|"ValueError / other"| M["propagates to _process_one_prim"]
J --> N["_oriented_box / _fallback_box_geom"]
N -->|"ValueError / RuntimeError / ZeroDivisionError"| O[AABB fallback]
N -->|"other exception"| P["propagates - prim bake fails"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[cook_scene_package] --> B[_cook_entity_collision]
A --> C[bake_scene_mjcf]
B --> D[cook_entity_collision_hulls]
D --> E["import coacd (hard requirement)"]
E --> F{coacd.run_coacd}
F -->|"AssertionError / RuntimeError / ValueError"| G["return empty list - single convex hull"]
F -->|success| H[return hull parts]
C --> I["_bake_prims (ProcessPoolExecutor)"]
I --> J[_process_one_prim]
J --> K[_simplify_mesh_geom]
K -->|RuntimeError| L[convex hull fallback]
K -->|"ValueError / other"| M["propagates to _process_one_prim"]
J --> N["_oriented_box / _fallback_box_geom"]
N -->|"ValueError / RuntimeError / ZeroDivisionError"| O[AABB fallback]
N -->|"other exception"| P["propagates - prim bake fails"]
Reviews (1): Last reviewed commit: "refactor: bare np.ndarray -> NDArray[......" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+891
to
+892
| except RuntimeError: | ||
| logger.warning("mesh simplification failed; falling back to convex hull", exc_info=True) |
Contributor
There was a problem hiding this comment.
_simplify_mesh_geom previously caught Exception so any open3d error would fall through to _convex_hull_mesh. Narrowing to RuntimeError alone means open3d errors that surface as ValueError, ZeroDivisionError, or numpy-level exceptions bypass the convex hull fallback entirely. When the caller (_process_one_prim) then catches the propagated ValueError/ZeroDivisionError, the whole visual OBJ write is abandoned rather than falling back to a coarser hull.
Comment on lines
+786
to
789
| import trimesh # type: ignore[import-untyped] | ||
|
|
||
| try: | ||
| tm = trimesh.Trimesh(vertices=vertices, faces=np.empty((0, 3), dtype=np.int32)) |
Contributor
There was a problem hiding this comment.
_fallback_box_geom
The original except Exception: pass silently fell through to the AABB path regardless of cause. The new handler only catches (ValueError, RuntimeError, ZeroDivisionError). If trimesh raises an AttributeError (e.g., a None OBB primitive) or another uncovered exception, it propagates to _emit_primitive_geom and can crash the bake for that prim rather than gracefully using AABB.
Comment on lines
+115
to
+117
| import coacd # type: ignore[import-not-found, import-untyped] | ||
|
|
||
| if not getattr(_run_coacd, "_coacd_silenced", False): | ||
| coacd.set_log_level("error") | ||
| _run_coacd._coacd_silenced = True # type: ignore[attr-defined] | ||
| silence_coacd_logging() |
Contributor
There was a problem hiding this comment.
import coacd now outside try — ModuleNotFoundError is no longer caught
Previously the entire coacd call (including the import) was inside the except Exception block, so a missing coacd installation would return [] and fall back to a single convex hull silently. Now import coacd is unconditional, so a missing module raises ModuleNotFoundError straight to the caller. The docstring and test file (import coacd # noqa: F401 ensures collection fails loudly) confirm this is intentional — but it is a breaking behaviour change for callers that relied on the silent fallback and may be running without the scene extra installed. Is this intentional — i.e. coacd is now a hard requirement whenever cook_entity_collision_hulls is called, not an optional accelerator?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These are automated fixes. Each fix is a separate commit. Use
git rebase -ito drop any you disagree with.