diff --git a/src/spatialgeometry/geom/CollisionShape.py b/src/spatialgeometry/geom/CollisionShape.py index e418964..e036dff 100644 --- a/src/spatialgeometry/geom/CollisionShape.py +++ b/src/spatialgeometry/geom/CollisionShape.py @@ -10,7 +10,7 @@ from abc import abstractmethod from collections import UserList from collections.abc import Iterable -from typing import Any +from typing import Any, cast import numpy as np from spatialmath.base.argcheck import getvector @@ -408,10 +408,10 @@ def __init__( raise FileNotFoundError(f"Mesh file not found: {filename!r}") super().__init__(stype="mesh", color=color, **kwargs) - self.filename = filename + self._filename = filename self.scale = scale self._use_vertex_colors = color is None - self.y_up = y_up + self._y_up = bool(y_up) # Overrides Shape.color's setter (keeping its getter) purely to track # whether a caller has explicitly asked for a flat color at any point @@ -434,7 +434,7 @@ def _init_coal(self) -> None: "Install with: pip install trimesh" ) - mesh = trimesh.load(self.filename, force="mesh") + mesh = cast(trimesh.Trimesh, trimesh.load(self.filename, force="mesh")) vertices = mesh.vertices if self.y_up: # See the LOUD WARNING on _Y_UP_TO_Z_UP above -- Swift's @@ -475,12 +475,16 @@ def scale(self, value: ArrayLike | float | None) -> None: @property def filename(self) -> str | None: - return self._filename + """ + Absolute path to this mesh's source file, as given to the + constructor. There's no use case for repointing an existing + :class:`Mesh` at a different file -- construct a new one instead. - @filename.setter - @mark_changed - def filename(self, value: str | None) -> None: - self._filename = value + This is a read-only property. + + :rtype: str + """ + return self._filename @property def y_up(self) -> bool: @@ -488,17 +492,15 @@ def y_up(self) -> bool: True if this mesh file was authored with +Y as "up" and needs the +Y -> +Z correction applied. - This is a read/write property. + This describes a fact about the mesh file itself, fixed at + construction -- it isn't live scene state. + + This is a read-only property. :rtype: bool """ return self._y_up - @y_up.setter - @mark_changed - def y_up(self, value: bool) -> None: - self._y_up = bool(value) - def to_dict(self) -> dict[str, Any]: shape = super().to_dict() shape["filename"] = self.filename @@ -520,8 +522,18 @@ def _local_corners(self) -> np.ndarray: "bounding box. Install with: pip install spatialgeometry[mesh]" " (or just pip install trimesh directly)" ) - mesh = trimesh.load(self.filename, force="mesh") - mn, mx = mesh.bounds + mesh = cast(trimesh.Trimesh, trimesh.load(self.filename, force="mesh")) + vertices = mesh.vertices + if self.y_up: + # Same correction _init_coal() applies -- see the LOUD WARNING + # on _Y_UP_TO_Z_UP above Mesh. Re-derive min/max from the + # rotated vertices rather than rotating mesh.bounds' two corner + # points directly -- correct either way for this particular + # transform (a signed permutation), but this stays correct even + # if _Y_UP_TO_Z_UP is ever generalised to an arbitrary rotation. + vertices = vertices @ _Y_UP_TO_Z_UP + mn = vertices.min(axis=0) + mx = vertices.max(axis=0) return aabb_corners(mn * self.scale, mx * self.scale) diff --git a/tests/test_Shape.py b/tests/test_Shape.py index 0e208b3..0c8330c 100644 --- a/tests/test_Shape.py +++ b/tests/test_Shape.py @@ -354,6 +354,17 @@ def test_mesh2(self): self.assertEqual(s0.to_dict(), ans) + def test_mesh_filename_and_y_up_are_readonly(self): + # Both only ever get read once, inside _init_coal() -- which is + # cached after first use (see self._cinit) and never re-runs -- so + # a setter would silently do nothing after a shape's first + # closest_point()/iscollided() call. Construct a new Mesh instead. + s0 = gm.Mesh(self.mesh_path, y_up=True) + with self.assertRaises(AttributeError): + s0.filename = "other.stl" + with self.assertRaises(AttributeError): + s0.y_up = False + def test_mesh_use_vertex_colors(self): # No explicit color -- defer to whatever's baked into the file. s0 = gm.Mesh(self.mesh_path) @@ -783,6 +794,26 @@ def test_mesh_extents(self): s1 = gm.Mesh(path, scale=[2, 2, 2]) nt.assert_almost_equal(s1.extents(), [2, 4, 6], decimal=6) + def test_mesh_extents_reflects_y_up_correction(self): + # Regression test: _local_corners() used to load the file itself + # via a separate trimesh.load() call 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 (file's + # own, uncorrected) frame. The correction swaps Y and Z (with a + # sign flip on the new Z), so a 1x2x3 (x,y,z) box becomes 1x3x2. + import tempfile + import trimesh + + with tempfile.TemporaryDirectory() as tmp: + path = f"{tmp}/asym.stl" + trimesh.creation.box(extents=[1, 2, 3]).export(path) + + s0 = gm.Mesh(path, y_up=False) + nt.assert_almost_equal(s0.extents(), [1, 2, 3], decimal=6) + + s1 = gm.Mesh(path, y_up=True) + nt.assert_almost_equal(s1.extents(), [1, 3, 2], decimal=6) + def test_mesh_extents_independent_of_collision_flag(self): # Unlike _init_coal(), the bounding box is a plain geometric fact # and must work even when collision=False.