Repository: pygfx/pygfx (python)

Issue:
Turn geometry into a simple store (and move TextGeometry api to Text object)
This has been discussed to some degree in several pull requests.

The geometry is a bit of an odd object, where we add buffers and textures without having properly documented how they should be named. To support this dynamic behavior, and to support properly tracking the buffers stored on it, it implements `__getattribute__`:

https://github.com/pygfx/pygfx/blob/735544f6b01bb171766eb7ce650f831ab81252d0/pygfx/geometries/_base.py#L95-L105

This makes attribute access on the geometry relatively slow. For most geometry there is not *that* much attribute access. But there is for `TextGeometry`; in benchmarks in #1006 I saw this take a significant portion of the CPU cycles in various stages of the processing of text.

 My proposal:

* Make `Geometry` subclass `Store` so it simply tracks all its attributes. (I have to double-check whether this works well.)
* Document clearly that `Geometry` is not to be subclassed. Just use it as a container to store buffers, textures, and maybe some other info like morph stuff.
* Document common buffer/texture names that are stored on a geometry.
* Each Material class documents what geometry attributes are supported.
* Move the text API to the `Text` world object, and a new `MultiText` world object.


Pull request:
Make geometry a simple Store
Closes #1011 

* [x] Refactor trackable logic a bit, so that a store can be used directly.
* [x] Reclass and strip Geometry.
* [x] Update Geometry docs .
* [x] Document usage of geometry in docstrings.

Patch:
diff --git a/pygfx/geometries/_base.py b/pygfx/geometries/_base.py
index 42c0bf7..485fb54 100644
--- a/pygfx/geometries/_base.py
+++ b/pygfx/geometries/_base.py
@@ -3,25 +3,58 @@ from __future__ import annotations
 from typing import TYPE_CHECKING, Iterable
 import numpy as np
 
-from ..utils.trackable import Trackable
+from ..utils.trackable import Store
 from ..resources import Resource, Buffer, Texture
 
 if TYPE_CHECKING:
-    import collections.abc
-    from numpy.typing import NDArray, ArrayLike
+    from numpy.typing import ArrayLike
 
+    ArrayOrBuffer = ArrayLike | Buffer
+    ArrayOrTexture = ArrayLike | Texture
+    ArrayOrTextureOrBuffer = ArrayLike | Texture | Buffer
 
-class Geometry(Trackable):
-    """Generic container for Geometry data.
+Optional = None
 
-    Parameters
-    ----------
-    kwargs : dict
-        A dict of attributes to define on the geometry object. Keys must match
-        the naming convention described in the implementation details section of
-        the :mod:`Geometries module <pygfx.geometries>`. If they don't they will
-        become optional attributes. Values must either be `Resources` or
-        ArrayLike.
+
+class Geometry(Store):
+    """An object's geometry is a container for data that 'defines' (the shape of) the object.
+
+    This class has no documented properties; attributes (usually buffers and
+    sometimes textures) can be freely added to it. What attributes are required
+    depends on the kind of object and the usage of the material. Some attributes
+    are optional. However, there are several common names.
+
+    Common buffer attributes:
+
+    * ``positions``: an Nx3 buffer representing vertex positions. Used by e.g. ``Mesh``, ``Line``, ``Points``, ``Text``.
+    * ``indices``: an Nx3 buffer representing the triangular faces of a ``Mesh``.
+    * ``normals``: an Nx3 buffer representing the surface normals of a ``Mesh``.
+    * ``texcoords``: an Nx1, Nx2, or Nx3 set of per-vertex texture coordinates. The dimensionality
+      should match that of the dimension of the colormap's texture (``material.map``).
+    * ``texcoords1``, ``texcoords2`` etc.: for additional texture coordinates. Usually Nx2.
+      E.g. a ``TextureMap`` with ``uv_channel`` set to 4 will use "texcoords4".
+    * ``colors``: per vertex or per-face color data for e.g. ``Mesh``, ``Line``, ``Points``.
+      Can be Nx1 (grayscale), Nx2 (gray plus alpha), Nx3 (RGB), or Nx4 (RGBA).
+    * ``sizes``: per vertex sizes for e.g. ``Points``.
+    * ``edge_colors`` per vertex edge colors for points with the marker material.
+    * ``rotations``: per vertex point/marker rotations.
+
+    Common texture attributes:
+
+    * ``grid``: a 2D or 3D texture for the ``Image`` and ``Volume`` objects, respectively.
+
+    For mesh morphing the following attributes are used:
+
+    * ``morph_targets_relative``: a bool indicating whether the morph positions are relative.
+    * ``morph_positions``: a list of arrays of per-vertex morph positions.
+    * ``morph_normals``: a list of arrays of per-vertex morph normals.
+    * ``morph_colors``: a list of arrays of per-vertex morph colors.
+
+    Instantiation
+    -------------
+    Most attributes of the geometry are buffers or textures. For convenience, these
+    can be passed as arrays, in which case they are automatically wrapped in a buffer
+    or texture.
 
     Example
     -------
@@ -29,19 +62,41 @@ class Geometry(Trackable):
     .. code-block:: py
 
         g = Geometry(positions=[[1, 2], [2, 4], [3, 5], [4, 1]])
+        g.positions  # Buffer
         g.positions.data  # numpy array
 
     """
 
-    def __init__(self, **kwargs: Resource | ArrayLike | collections.abc.Buffer):
+    def __init__(
+        self,
+        *,
+        positions: ArrayOrBuffer = Optional,
+        indices: ArrayOrBuffer = Optional,
+        normals: ArrayOrBuffer = Optional,
+        texcoords: ArrayOrBuffer = Optional,
+        colors: ArrayOrBuffer = Optional,
+        grid: ArrayOrTexture = Optional,
+        **other_attributes: ArrayOrTextureOrBuffer,
+    ):
         super().__init__()
 
-        self._aabb: NDArray | None = None
-        self._aabb_rev: int | None = None
-        self._bsphere: NDArray | None = None
-        self._bsphere_rev: int | None = None
-
-        for name, val in kwargs.items():
+        # We separately declare some possible attribute in the signature to help users via autocompletion.
+        # We just merge these with the kwargs so we can process them uniformly.
+        all_attributes = other_attributes.copy()
+        if positions is not Optional:
+            all_attributes["positions"] = positions
+        if indices is not Optional:
+            all_attributes["indices"] = indices
+        if normals is not Optional:
+            all_attributes["normals"] = normals
+        if texcoords is not Optional:
+            all_attributes["texcoords"] = texcoords
+        if colors is not Optional:
+            all_attributes["colors"] = colors
+        if grid is not Optional:
+            all_attributes["grid"] = grid
+
+        for name, val in all_attributes.items():
             # Get resource object
             if isinstance(val, Resource):
                 resource = val
@@ -91,17 +146,18 @@ class Geometry(Trackable):
             # Store
             setattr(self, name, resource)
 
-    def __setattr__(self, key: str, new_value: Resource) -> None:
-        if not key.startswith(("_", "morph_")):
-            if isinstance(new_value, Trackable) or key in self._store:
-                return setattr(self._store, key, new_value)
-        object.__setattr__(self, key, new_value)
-
-    def __getattribute__(self, key: str) -> Resource:
-        if not key.startswith(("_", "morph_")):
-            if key in self._store:
-                return getattr(self._store, key)
-        return object.__getattribute__(self, key)
+    def __repr__(self) -> str:
+        # A Store is a subclass of a dict, but it does not look like a dict,
+        # e.g. you cannot do geometry.items() or any of the regular dict
+        # methods, because *all*  atrribute access is converted to dict key
+        # access. So let's forget about this being a dict and also provide a
+        # useful repr.
+        lines = ["Geometry("]
+        for key in dir(self):
+            val = self[key]
+            lines.append(f"    {key}={val!r},")
+        lines.append(f") # at {hex(id(self))}")
+        return "\n".join(lines)
 
     def __dir__(self) -> Iterable[str]:
-        return [*object.__dir__(self), *self._store]
+        return sorted([name for name in self if not name.startswith("_trackable_")])
diff --git a/pygfx/objects/_base.py b/pygfx/objects/_base.py
index 11657a4..8edfa83 100644
--- a/pygfx/objects/_base.py
+++ b/pygfx/objects/_base.py
@@ -91,7 +91,9 @@ class WorldObject(EventTarget, Trackable):
     Parameters
     ----------
     geometry : Geometry
-        The data defining the shape of the object.
+        The data defining the shape of the object. See the documentation
+        on the different WorldObject subclasses for what attributes the
+        geometry should and may have.
     material : Material
         The data defining the appearance of the object.
     visible : bool
diff --git a/pygfx/objects/_more.py b/pygfx/objects/_more.py
index 885f397..a228c6e 100644
--- a/pygfx/objects/_more.py
+++ b/pygfx/objects/_more.py
@@ -121,7 +121,9 @@ class Line(WorldObject):
     Parameters
     ----------
     geometry : Geometry
-        The data defining the shape of the object.
+        The data defining the shape of the object. Must contain at least a
+        "positions" buffer. Depending on the usage of the material, can also
+        include buffers "texcoords", "colors", and "sizes".
     material : Material
         The data defining the appearance of the object.
     visible : bool
@@ -150,7 +152,9 @@ class Points(WorldObject):
     Parameters
     ----------
     geometry : Geometry
-        The data defining the shape of the object.
+        The data defining the shape of the object. Must contain at least a
+        "positions" buffer. Depending on the usage of the material, can also
+        include buffers "texcoords", "colors", "sizes", "edge_colors", "rotations".
     material : Material
         The data defining the appearance of the object.
     visible : bool
@@ -181,7 +185,10 @@ class Mesh(WorldObject):
     Parameters
     ----------
     geometry : Geometry
-        The data defining the shape of the object.
+        The data defining the shape of the object. Must contain at least a
+        "positions" and "indices" buffers. Depending on the usage of the material, can also
+        include buffers "normals", "colors", "texcoords", "texcoords2", etc.
+        Advanced use of meshes also support "tangents", "skin_indices",  and "skin_weights".
     material : Material
         The data defining the appearance of the object.
     visible : bool
@@ -306,7 +313,8 @@ class Image(WorldObject):
     Parameters
     ----------
     geometry : Geometry
-        The data defining the shape of the object.
+        The data defining the shape of the object. Must contain at least a
+        "grid" attribute for a 2D texture.
     material : Material
         The data defining the appearance of the object.
     visible : bool
@@ -349,7 +357,8 @@ class Volume(WorldObject):
     Parameters
     ----------
     geometry : Geometry
-        The data defining the shape of the object.
+        The data defining the shape of the object. Must contain at least a
+        "grid" attribute for a 3D texture.
     material : Material
         The data defining the appearance of the object.
     visible : bool
diff --git a/pygfx/objects/_text.py b/pygfx/objects/_text.py
index 247bba1..3a722a1 100644
--- a/pygfx/objects/_text.py
+++ b/pygfx/objects/_text.py
@@ -137,10 +137,11 @@ class Text(WorldObject):
 
     Parameters
     ----------
-    geometry : Geometry
-        An empty geometry where the ``Text`` will store the data to render the text.
-        Can be omitted. For backwards compatibility, this can also be a string, as an alias
-        for the `text` arg.
+    geometry : Geometry | str
+        The geometry on which the ``Text`` will store the buffers required to
+        render the text ("positions" and "glyph_data"). If omitted, a new
+        geometry is created automatically. This argument can also be used to
+        provide the text to render as a string (as an alias for the `text` arg).
     material : TextMaterial
         The text material to use. Can be omitted.
     text : str | list[str]
diff --git a/pygfx/renderers/wgpu/shaders/meshshader.py b/pygfx/renderers/wgpu/shaders/meshshader.py
index 7b80d1e..0749000 100644
--- a/pygfx/renderers/wgpu/shaders/meshshader.py
+++ b/pygfx/renderers/wgpu/shaders/meshshader.py
@@ -232,23 +232,15 @@ class MeshShader(BaseShader):
             )
 
         if self["use_morph_targets"]:
-            morph_texture, stride, width, morph_count = getattr(
-                geometry, "_gfx_morph_texture", (None, None, None, None)
-            )
-            if morph_texture is None:
-                morph_texture, stride, width, morph_count = self._encode_morph_texture(
-                    geometry, shared
-                )
-                geometry._gfx_morph_texture = (
-                    morph_texture,
-                    stride,
-                    width,
-                    morph_count,
-                )
-
-            morph_target_influences = (
-                wobject._morph_target_influences
-            )  # the influences buffer
+            # Get or create the morph texture and associated info
+            try:
+                morph_texture_info = geometry["_gfx_morph_texture"]
+            except KeyError:
+                morph_texture_info = self._encode_morph_texture(geometry, shared)
+                geometry["_gfx_morph_texture"] = morph_texture_info
+            morph_texture, stride, width, morph_count = morph_texture_info
+
+            morph_target_influences = wobject._morph_target_influences  # buffer
 
             if morph_texture and morph_target_influences:
                 view = GfxTextureView(morph_texture, view_dim="2d-array")
diff --git a/pygfx/utils/enums.py b/pygfx/utils/enums.py
index caade50..b515a54 100644
--- a/pygfx/utils/enums.py
+++ b/pygfx/utils/enums.py
@@ -76,7 +76,7 @@ class SizeMode(Enum):
     """The SizeMode enum specifies how an object's size/width/thickness is established."""
 
     uniform = None  #: Use a uniform size.
-    vertex = None  #: Use a per-vertex size specified on the geometry.
+    vertex = None  #: Use a per-vertex size specified with ``geometry.sizes``.
 
 
 class RotationMode(Enum):
@@ -86,7 +86,7 @@ class RotationMode(Enum):
     """
 
     uniform = None  #: Use a uniform rotation.
-    vertex = None  #: Use a per-vertex rotation specified on the geometry.
+    vertex = None  #: Use a per-vertex rotation specified with ``geometry.rotations``.
 
 
 class CoordSpace(Enum):
diff --git a/pygfx/utils/trackable.py b/pygfx/utils/trackable.py
index 695a28b..b6e3d86 100644
--- a/pygfx/utils/trackable.py
+++ b/pygfx/utils/trackable.py
@@ -143,6 +143,8 @@ class Store(dict):
             tracker._track_set((id, key), old_value, new_value)
 
     def __getattribute__(self, key):
+        if key.startswith("__"):
+            return dict.__getattribute__(self, key)
         value = undefined
         try:
             value = self[key]
@@ -262,8 +264,13 @@ class PropTracker:
         # tree behind that trackable (to the extend that we track it).
 
         is_trackable = 0
-        is_trackable |= 1 * isinstance(old_value, Trackable)
-        is_trackable |= 2 * isinstance(new_value, Trackable)
+        old_store = new_store = None
+        if isinstance(old_value, (Store, Trackable)):
+            is_trackable |= 1
+            old_store = old_value if isinstance(old_value, Store) else old_value._store
+        if isinstance(new_value, (Store, Trackable)):
+            is_trackable |= 2
+            new_store = new_value if isinstance(new_value, Store) else new_value._store
 
         # Get labels
         labels = self._trackable_names.get(name, None)
@@ -272,16 +279,17 @@ class PropTracker:
             # Register / unregister stores
             if is_trackable & 1:
                 for label in labels:
-                    self._untrack_store(old_value._store, label)
+                    self._untrack_store(old_store, label)
             if is_trackable & 2:
                 for label in labels:
-                    self._track_store(new_value._store, label)
+                    self._track_store(new_store, label)
             # If this was and is a trackable, we only process labels starting with "!"
             if is_trackable == 3 and type(old_value) is type(new_value):
                 labels = set(label for label in labels if label.startswith("!"))
 
             # Follow the hierarchy
-            self._track_set_follow_tree(old_value, new_value)
+            if old_store is not None:
+                self._track_set_follow_tree(old_store, new_store)
 
         # Update changed values
         if labels:
@@ -299,51 +307,56 @@ class PropTracker:
             elif comp_value != cur_value:
                 self._trackable_values[name] = ref_value, comp_value
 
-    def _track_set_follow_tree(self, old_value, new_value):
-        if isinstance(old_value, Trackable):
-            # Get names of stuff we track on the old trackable store
-            id = old_value._store["_trackable_id"]
-            sub_names = [n for n in self._trackable_names.keys() if n[0] == id]
+    def _track_set_follow_tree(self, old_store, new_store):
+        # old_store must be a store
+        # new_store must be a store or None
 
-            # We need to follow these
-            for name1 in sub_names:
-                _, key = name1
-                try:
-                    value1 = old_value._store[key]
-                except KeyError:
-                    continue
+        # Get names of stuff we track on the old trackable store
+        id = old_store["_trackable_id"]
+        sub_names = [n for n in self._trackable_names.keys() if n[0] == id]
+
+        # We need to follow these
+        for name1 in sub_names:
+            _, key = name1
+            try:
+                value1 = old_store[key]
+            except KeyError:
+                continue
 
-                # Try get matching path in the new hierarchy
+            # Try get matching path in the new hierarchy
+            value2 = undefined
+            if new_store is not None:
                 try:
-                    name2 = new_value._store["_trackable_id"], key
-                    value2 = new_value._store[key]
+                    name2 = new_store["_trackable_id"], key
+                    value2 = new_store[key]
                 except (AttributeError, KeyError):
-                    value2 = undefined
-
-                if value2 is not undefined:
-                    # Rename
-                    self._trackable_names[name2] = self._trackable_names.pop(name1)
-                    self._trackable_values[name2] = self._trackable_values.pop(name1)
-                    try:
-                        self._trackable_changed[name2] = self._trackable_changed.pop(
-                            name1
-                        )
-                    except KeyError:
-                        pass
-                    # Recurse
-                    self._track_set(name2, value1, value2)
-
-                else:
-                    # Unregister
-                    labels = self._trackable_names.pop(name1, ())
-                    self._trackable_values.pop(name1, None)
-                    dirty_labels_for_this_name = self._trackable_changed.setdefault(
-                        name1, set()
-                    )
-                    dirty_labels_for_this_name.update(labels)
-                    # Unregister stores
-                    if isinstance(value1, Trackable):
-                        for label in labels:
-                            self._untrack_store(value1._store, label)
-                    # Recurse
-                    self._track_set_follow_tree(value1, value2)
+                    pass
+
+            if value2 is not undefined:
+                # Rename
+                self._trackable_names[name2] = self._trackable_names.pop(name1)
+                self._trackable_values[name2] = self._trackable_values.pop(name1)
+                try:
+                    self._trackable_changed[name2] = self._trackable_changed.pop(name1)
+                except KeyError:
+                    pass
+                # Recurse
+                self._track_set(name2, value1, value2)
+
+            else:
+                # Unregister
+                labels = self._trackable_names.pop(name1, ())
+                self._trackable_values.pop(name1, None)
+                dirty_labels_for_this_name = self._trackable_changed.setdefault(
+                    name1, set()
+                )
+                dirty_labels_for_this_name.update(labels)
+                # Unregister stores
+                if isinstance(value1, Trackable):
+                    for label in labels:
+                        self._untrack_store(value1._store, label)
+                # Recurse
+                if isinstance(value1, Store):
+                    self._track_set_follow_tree(value1, None)
+                elif isinstance(value1, Trackable):
+                    self._track_set_follow_tree(value1._store, None)


Test patch:
diff --git a/tests/utils/test_trackable.py b/tests/utils/test_trackable.py
index 29925a9..b6a636b 100644
--- a/tests/utils/test_trackable.py
+++ b/tests/utils/test_trackable.py
@@ -2,7 +2,7 @@ import gc
 import time
 import weakref
 
-from pygfx.utils.trackable import Trackable, PropTracker
+from pygfx.utils.trackable import Trackable, Store, PropTracker
 
 # ruff: noqa: B018 - in these tests we access values for their side-effect
 
@@ -628,6 +628,98 @@ def test_track_trackables2():
     assert root.pop_changed() == {"resources"}
 
 
+def test_track_substore0():
+    # This represents tracking the resource objects themselves - without "!"
+
+    root = MyRootTrackable()
+    t1 = Store()
+    t2 = Store()
+
+    t1.foo = t2.foo = 42
+
+    root.sub1 = t1
+
+    with root.track_usage("format"):
+        root.sub1.foo
+
+    with root.track_usage("resources"):
+        root.sub1
+
+    t1.foo = 43
+    assert root.pop_changed() == {"format"}
+
+    root.sub1 = t2
+    assert root.pop_changed() == {"format"}  # no resources
+
+    t2.foo = 43
+    assert root.pop_changed() == {"format"}
+
+    root.sub1 = t1
+    assert root.pop_changed() == set()  # no resources
+
+
+def test_track_substore1():
+    # This represents tracking the resource objects themselves - with "!"
+
+    root = MyRootTrackable()
+    t1 = Store()
+    t2 = Store()
+
+    t1.foo = t2.foo = 42
+
+    root.sub1 = t1
+
+    with root.track_usage("format"):
+        root.sub1.foo
+
+    with root.track_usage("!resources"):
+        root.sub1
+
+    t1.foo = 43
+    assert root.pop_changed() == {"format"}
+
+    root.sub1 = t2
+    assert root.pop_changed() == {"format", "resources"}
+
+    t2.foo = 43
+    assert root.pop_changed() == {"format"}
+
+    root.sub1 = t1
+    assert root.pop_changed() == {"resources"}
+
+
+def test_track_substore2():
+    # This represents tracking the resource objects themselves - deeper
+
+    root = MyRootTrackable()
+    tree1 = Store()
+    tree2 = Store()
+    tree1.sub1 = t1 = Store()
+    tree2.sub1 = t2 = Store()
+
+    tree1.sub1.foo = tree2.sub1.foo = 42
+
+    root.sub1 = tree1
+
+    with root.track_usage("format"):
+        root.sub1.sub1.foo
+
+    with root.track_usage("!resources"):
+        root.sub1.sub1
+
+    t1.foo = 43
+    assert root.pop_changed() == {"format"}
+
+    root.sub1 = tree2
+    assert root.pop_changed() == {"format", "resources"}
+
+    t2.foo = 43
+    assert root.pop_changed() == {"format"}
+
+    root.sub1 = tree1
+    assert root.pop_changed() == {"resources"}
+
+
 def test_track_trackables_typing1():
     # This represents tracking the resource objects themselves