fix: make Mesh.filename/y_up read-only, fix corners() ignoring y_up

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,30 +475,32 @@ 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:
         """
         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)
 
 

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.