Repository: pygfx/pygfx
Pull request #527 (2023-05-09 22:21:03)

Fix ups
Fix some leftover references to up. See #482.

* [x] Fixed some docstrings that still mentioned minus/reverse from when `reference_up`  was called `gravity`.
* [x] Fixed that `ob.world.reference_up`  was initially a tuple (its an array everywhere else).
* [x] Fixed that setting `ob.world.reference_up` was undone when the local transform changed (the change in `world.reference_up` was not propagated to the `local` transform.
* [x] Added a test to make sure the above stays working, based using `camera.show_pos`.
* [x] Prefixed two callback functions with an underscore.  
* [x] Update all (I think) cases that still used `ob.up` instead of `ob.world.reference_up`. 


Patch:
diff --git a/examples/feature_demo/multi_slice1.py b/examples/feature_demo/multi_slice1.py
index 971dc69..868d75f 100644
--- a/examples/feature_demo/multi_slice1.py
+++ b/examples/feature_demo/multi_slice1.py
@@ -60,7 +60,7 @@ for axis in [0, 1, 2]:
 
 # camera = gfx.PerspectiveCamera(70, 16 / 9)
 camera = gfx.PerspectiveCamera(0)
-camera.up = 0, 0, 1
+camera.world.reference_up = 0, 0, 1
 camera.local.position = (200, 200, 200)
 camera.show_pos((0, 0, 0))
 
diff --git a/pygfx/objects/_base.py b/pygfx/objects/_base.py
index 74dd572..99ab31f 100644
--- a/pygfx/objects/_base.py
+++ b/pygfx/objects/_base.py
@@ -196,7 +196,7 @@ class WorldObject(EventTarget, RootTrackable):
     def up(self):
         """
         Relic of old WorldObjects that aliases with the new ``transform.up``
-        direction. Prefer (minus) `obj.world.reference_up.` instead
+        direction. Prefer `obj.world.reference_up` instead.
 
         """
 
diff --git a/pygfx/utils/cube_camera.py b/pygfx/utils/cube_camera.py
index 5a936e7..6441551 100644
--- a/pygfx/utils/cube_camera.py
+++ b/pygfx/utils/cube_camera.py
@@ -51,32 +51,32 @@ class CubeCamera(WorldObject):
         # so camrea_px is actually looking at the neg-x direction, and camera_nx is looking at the pos-x direction.
 
         camera_px = PerspectiveCamera(fov, aspect, depth_range=depth_range)
-        camera_px.up = (0, 1, 0)
+        camera_px.world.reference_up = (0, 1, 0)
         camera_px.look_at((-1, 0, 0))
         self.add(camera_px)
 
         camera_nx = PerspectiveCamera(fov, aspect, depth_range=depth_range)
-        camera_nx.up = (0, 1, 0)
+        camera_nx.world.reference_up = (0, 1, 0)
         camera_nx.look_at((1, 0, 0))
         self.add(camera_nx)
 
         camera_py = PerspectiveCamera(fov, aspect, depth_range=depth_range)
-        camera_py.up = (0, 0, -1)
+        camera_py.world.reference_up = (0, 0, -1)
         camera_py.look_at((0, 1, 0))
         self.add(camera_py)
 
         camera_ny = PerspectiveCamera(fov, aspect, depth_range=depth_range)
-        camera_ny.up = (0, 0, 1)
+        camera_ny.world.reference_up = (0, 0, 1)
         camera_ny.look_at((0, -1, 0))
         self.add(camera_ny)
 
         camera_pz = PerspectiveCamera(fov, aspect, depth_range=depth_range)
-        camera_pz.up = (0, 1, 0)
+        camera_pz.world.reference_up = (0, 1, 0)
         camera_pz.look_at((0, 0, 1))
         self.add(camera_pz)
 
         camera_nz = PerspectiveCamera(fov, aspect, depth_range=depth_range)
-        camera_nz.up = (0, 1, 0)
+        camera_nz.world.reference_up = (0, 1, 0)
         camera_nz.look_at((0, 0, -1))
         self.add(camera_nz)
 
diff --git a/pygfx/utils/transform.py b/pygfx/utils/transform.py
index 578847c..698ff50 100644
--- a/pygfx/utils/transform.py
+++ b/pygfx/utils/transform.py
@@ -301,7 +301,7 @@ class AffineBase:
 
     @reference_up.setter
     def reference_up(self, value):
-        self._reference_up = np.asarray(value)
+        self._reference_up = la.vec_normalize(value)
         self.flag_update()
 
     @x.setter
@@ -503,7 +503,7 @@ class RecursiveTransform(AffineBase):
 
     This transform behaves semantically identical to an ordinary
     ``AffineTransform`` (same properties), except that users may define a
-    ``parent`` transform which proceeds the ``matrix`` used by the ordinary
+    ``parent`` transform which preceeds the ``matrix`` used by the ordinary
     ``AffineTransform``. The resulting ``RecursiveTransform`` then controls the
     total transform that results from combinign the two transforms via::
 
@@ -539,11 +539,11 @@ class RecursiveTransform(AffineBase):
     parent : AffineBase, optional
         The parent transform that preceeds the base transform.
     reference_up : ndarray, [3]
-        The direction of the reference_up vector
-        expressed in the target frame. It is the inverse of ``WorldObject.up``
-        and used by the axis properties (right, up, forward) to maintain a
-        common level of rotation around an axis when it is updated by it's
-        setter. By default, it points along the negative Y-axis.
+        The direction of the reference_up vector expressed in the target
+        frame. It is used by the axis properties (right, up, forward)
+        to maintain a common level of rotation around an axis when it
+        is updated by it's setter. By default, it points along the
+        positive Y-axis.
     is_camera_space : bool
         If True, the transform represents a camera space which means that it's
         ``forward`` and ``right`` directions are inverted.
@@ -587,33 +587,40 @@ class RecursiveTransform(AffineBase):
         if reference_up is None:
             # use own's reference_up
             reference_up = la.vec_transform(self.own.reference_up, self.parent.matrix)
-            self._reference_up = reference_up
+            self._reference_up = la.vec_normalize(reference_up)
         else:
             # use given reference_up (and sync own)
-            self._reference_up = reference_up
-            own_ref = la.vec_transform(reference_up, self.parent.inverse_matrix)
-            self.own._reference_up = own_ref
+            self._reference_up = la.vec_normalize(reference_up)
+            self._propagate_reference_up()
 
-        self.parent.on_update(self.parent_updated)
-        self.own.on_update(self.child_updated)
+        self.parent.on_update(self._parent_updated)
+        self.own.on_update(self._own_updated)
 
     def flag_update(self):
         self.last_modified = perf_counter_ns()
         super().flag_update()
 
     @callback
-    def parent_updated(self, parent: AffineBase):
-        own_ref = la.vec_transform(self.reference_up, parent.inverse_matrix)
-        parent_origin = la.vec_transform((0, 0, 0), parent.inverse_matrix)
-        self.own._reference_up = own_ref - parent_origin
-
+    def _parent_updated(self, parent: AffineBase):
+        self._propagate_reference_up()
         self.flag_update()
 
+    def _propagate_reference_up(self):
+        new_ref = la.vec_transform(self._reference_up, self._parent.inverse_matrix)
+        origin = la.vec_transform((0, 0, 0), self._parent.inverse_matrix)
+        self.own._reference_up = la.vec_normalize(new_ref - origin)
+
     @callback
-    def child_updated(self, own: AffineBase):
+    def _own_updated(self, own: AffineBase):
         new_ref = la.vec_transform(own.reference_up, self.parent.matrix)
-        self._reference_up = new_ref - self.parent.position
+        origin = self.parent.position
+        self._reference_up = la.vec_normalize(new_ref - origin)
+        self.flag_update()
 
+    @AffineBase.reference_up.setter
+    def reference_up(self, value):
+        self._reference_up = la.vec_normalize(value)
+        self._propagate_reference_up()
         self.flag_update()
 
     @property
@@ -623,18 +630,15 @@ class RecursiveTransform(AffineBase):
 
     @parent.setter
     def parent(self, value):
-        self.parent.remove_callback(self.parent_updated)
+        self.parent.remove_callback(self._parent_updated)
 
         if value is None:
             self._parent = AffineTransform()
         else:
             self._parent = value
 
-        own_ref = la.vec_transform(self.reference_up, self._parent.inverse_matrix)
-        parent_origin = la.vec_transform((0, 0, 0), self._parent.inverse_matrix)
-        self.own._reference_up = own_ref - parent_origin
-
-        self.parent.on_update(self.parent_updated)
+        self._propagate_reference_up()
+        self.parent.on_update(self._parent_updated)
         self.flag_update()
 
     @cached


Test patch:
diff --git a/tests/cameras/test_cameras.py b/tests/cameras/test_cameras.py
index d284196..23321b8 100644
--- a/tests/cameras/test_cameras.py
+++ b/tests/cameras/test_cameras.py
@@ -79,6 +79,49 @@ def test_camera_show_methods():
     assert np.allclose(camera.local.position, [250, 300, 550])
 
 
+def test_camera_reference_up():
+    # Avoid regressions, see #527
+
+    camera = gfx.PerspectiveCamera()
+
+    # Check default up
+    assert tuple(camera.world.reference_up) == (0, 1, 0)
+
+    # Using current up
+    camera.show_pos((1, 0, 0))
+    assert tuple(camera.world.reference_up) == (0, 1, 0)
+
+    # Using an up +Z
+    camera.show_pos((1, 0, 0), up=(0, 0, 1))
+    assert tuple(camera.world.reference_up) == (0, 0, 1)
+
+    # Using an up +X
+    camera.show_pos((0, 1, 0), up=(1, 0, 0))
+    assert tuple(camera.world.reference_up) == (1, 0, 0)
+
+    # Not specifying up, keeps the current up (no revert to default)
+    camera.show_pos((0, 1, 1))
+    assert tuple(camera.world.reference_up) == (1, 0, 0)
+
+    # Back to +Y
+    camera.show_pos((1, 0, 0), up=(0, 1, 0))
+    assert tuple(camera.world.reference_up) == (0, 1, 0)
+
+    # ---
+
+    # It normalizes too
+    camera.show_pos((1, 0, 0), up=(0, 0, 2))
+    assert tuple(camera.world.reference_up) == (0, 0, 1)
+
+    # Can also set up directly
+    camera.world.reference_up = (2, 0, 0)
+    assert tuple(camera.world.reference_up) == (1, 0, 0)
+
+    # Stays that way
+    camera.show_pos((1, 1, 0))
+    assert tuple(camera.world.reference_up) == (1, 0, 0)
+
+
 def _run_for_camera(camera, near, far, check_halfway):
     # Some notes:
     #
diff --git a/tests/objects/test_world_object.py b/tests/objects/test_world_object.py
index bb56de4..135a40c 100644
--- a/tests/objects/test_world_object.py
+++ b/tests/objects/test_world_object.py
@@ -277,6 +277,11 @@ def test_axis_setters():
     assert np.allclose(obj.world.up, (0, 1, 0))
     assert np.allclose(obj.world.forward, (0, 0, -1))
 
+    # these vectors are unit
+
+    obj.world.forward = (2, 0, 0)
+    assert np.allclose(obj.world.forward, (1, 0, 0))
+
     obj.world.up = (1, 1, 1)
     assert np.allclose(obj.world.up, (1 / np.sqrt(3), 1 / np.sqrt(3), 1 / np.sqrt(3)))
 
@@ -309,14 +314,19 @@ def test_reference_up():
     assert np.allclose(group.world.reference_up, (0, 1, 0))
     assert np.allclose(obj1.world.reference_up, (0, 1, 0))
 
-    # (world) up_reference is independent between objects
+    # (world) reference_up is independent between objects
     obj2 = gfx.WorldObject()
     obj2.world.rotation = (0, 0, 1, 0)
     group.add(obj1, keep_world_matrix=True)
 
+    obj3 = gfx.WorldObject()
+
     obj1.world.reference_up = (1, 2, 3)
     obj2.world.reference_up = (1, 0, 1)
+    obj3.world.reference_up = (0, 42, 0)
+    isqrt2 = 1 / np.sqrt(2)
 
     assert np.allclose(group.local.reference_up, (0, 1, 0))
-    assert np.allclose(obj1.world.reference_up, (1, 2, 3))
-    assert np.allclose(obj2.world.reference_up, (1, 0, 1))
+    assert np.allclose(obj1.world.reference_up, la.vec_normalize((1, 2, 3)))
+    assert np.allclose(obj2.world.reference_up, (isqrt2, 0, isqrt2))
+    assert np.allclose(obj3.world.reference_up, (0, 1, 0))
diff --git a/tests/renderers/test_reactivity.py b/tests/renderers/test_reactivity.py
index 559a6c6..9a0b750 100644
--- a/tests/renderers/test_reactivity.py
+++ b/tests/renderers/test_reactivity.py
@@ -141,7 +141,7 @@ def test_reactivity_mesh3():
     tex1 = gfx.cm.cividis
     tex2 = gfx.cm.inferno
     cmap3 = np.array([(1,), (0,), (0,), (1,)], np.int32)
-    tex3 = gfx.Texture(cmap3, dim=1).get_view(filter="linear")
+    tex3 = gfx.Texture(cmap3, dim=1)
 
     # only float32 color map is supported in MeshPhongMaterial for now
     obj = gfx.Mesh(geometry, gfx.MeshBasicMaterial(map=tex1))
@@ -157,7 +157,7 @@ def test_reactivity_mesh3():
     # Change to colormap of different format, need rebuild!
     obj.material.map = tex3
     print("uv", geometry.texcoords.data.shape)
-    print("map", obj.material.map.view_dim)
+    print("map", obj.material.map.dim)
     changed = render(obj)
     assert changed == {"bindings", "compile_shader", "compose_pipeline"}