fix: make Mesh.filename/y_up read-only, fix corners() ignoring y_up - #33
Merged
Merged
Conversation
Both properties are only ever read once, inside _init_coal(), which is cached via self._cinit and never re-runs after a shape's first closest_point()/iscollided() call -- so their setters were a footgun: mutate either after that point and the collision geometry silently keeps using the old value. Made both read-only (set only via the constructor), matching the existing pattern for `collision` on this same class. There's no use case for repointing an existing Mesh at a different file -- construct a new one instead. Also fixes a real, independent bug this surfaced: _local_corners() (used by corners()/bounds()/extents()) did its own separate trimesh.load() and never applied the _Y_UP_TO_Z_UP correction _init_coal() applies, so a y_up=True mesh's bounding box was silently computed in the wrong (uncorrected) frame -- inconsistent with what's actually rendered/ collision-checked. Fixed by re-deriving min/max from the corrected vertices, matching _init_coal()'s own approach. Also adds `cast(trimesh.Trimesh, ...)` in both places that call trimesh.load(..., force="mesh") -- the return type is statically a looser Geometry, but force="mesh" guarantees a Trimesh at runtime. Two new tests: read-only enforcement on both properties, and a y_up-vs-extents() regression test (1x2x3 box -> 1x3x2 under the correction).
…-corners # Conflicts: # src/spatialgeometry/geom/CollisionShape.py
Merge fallout, not a real regression: jhavl#35 (merged after this branch was written) added a FileNotFoundError check at Mesh construction, so this test's literal "test.stl" placeholder -- fine when the test was written, since nothing checked it existed -- now fails construction before ever reaching the read-only assertions the test is actually about. Use the real placeholder file (self.mesh_path, from jhavl#35's own setUpClass) instead, matching every other Mesh-constructing test in this file.
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.
Summary
Mesh.filename/Mesh.y_uphad public setters, but both are only ever read once, inside_init_coal()-- which is cached (self._cinit) and never re-runs after a shape's firstclosest_point()/iscollided()call. Mutating either after that point silently left the collision geometry stale, no error. Made both read-only, set only via the constructor -- matches the existing pattern forcollisionon this same class. No real use case for repointing an existingMeshat a different file; construct a new one instead._local_corners()(backingcorners()/bounds()/extents()) does its own separatetrimesh.load()and never applied the_Y_UP_TO_Z_UPcorrection_init_coal()applies -- so ay_up=Truemesh's bounding box was silently computed in the wrong (uncorrected) frame, inconsistent with what's actually rendered/collision-checked. Fixed by re-deriving min/max from the corrected vertices.cast(trimesh.Trimesh, ...)at bothtrimesh.load(..., force="mesh")call sites -- the return type is statically the looserGeometry, butforce="mesh"guarantees aTrimeshat runtime.Test plan
pytest tests/-- 161 passed (159 existing + 2 new: read-only enforcement on both properties, and ay_up-vs-extents()regression test: a 1x2x3 box becomes 1x3x2 under the correction).🤖 Generated with Claude Code