Repository: pygfx/pygfx (python)

Issue:
Check dtype of buffer and texture data
Right now, you can pass int64 for `geometry.indices` and get correct results on some machines but not on others (?) see #529.

I would have thought that this gets detected when converting the dtype to the gpu type. I suppose some cases somehow slip through.

Pull request:
Buffer and texture dtype validation
Closes #579, and some general improvements, ref #19.

This fixes that 64-bit data could be passed to a buffer without pygfx complaining about it, resulting in a broken visualization.

* [x] Data passed to `Geometry(...)` is simply passed to the buffer constructor, so any checks happen in one place.
* [x] The format is now derived in the `Buffer`  and  `Texture` constructor, instead of lazily. Fail early.
* [x] Fix the detection of float64 arrays and also detect int64 arrays (The original issue).
* [x] `Buffer.format` is now `None` for e.g. uniform buffers (instead of raising an error).
* [x] Added some unit tests for buffer, texture and geometry.


Patch:
diff --git a/pygfx/geometries/_base.py b/pygfx/geometries/_base.py
index 7966d4e..12866f5 100644
--- a/pygfx/geometries/_base.py
+++ b/pygfx/geometries/_base.py
@@ -40,13 +40,11 @@ class Geometry(Trackable):
             if isinstance(val, Resource):
                 resource = val
             else:
-                if not isinstance(val, np.ndarray):
-                    dtype = np.uint32 if name == "indices" else np.float32
-                    val = np.asanyarray(val, dtype=dtype)
-                if val.dtype == np.float64:
-                    raise ValueError(
-                        "64-bit float is not supported, use 32-bit floats instead"
-                    )
+                # Convert literal arrays to numpy arrays (buffers and textures require memoryview compatible data).
+                if isinstance(val, list):
+                    dtype = "int32" if name == "indices" else "float32"
+                    val = np.array(val, dtype=dtype)
+                # Create texture or buffer
                 if name == "grid":
                     dim = val.ndim
                     if dim > 2 and val.shape[-1] <= 4:
@@ -59,7 +57,10 @@ class Geometry(Trackable):
             if isinstance(resource, Buffer):
                 format = resource.format
                 if name == "indices":
-                    pass  # No assumptions about shape; they're considered flat anyway
+                    # Make no assumptions about shape. Shader will need to validate.
+                    # For meshes will be Nx3 or Nx4, but other dtypes may support
+                    # multidimensional stuff for fancy graphics.
+                    pass
                 elif name == "positions":
                     if not format.startswith("3x"):
                         raise ValueError("Expected Nx3 data for positions")
diff --git a/pygfx/resources/_base.py b/pygfx/resources/_base.py
index e3d7943..6a8d5bb 100644
--- a/pygfx/resources/_base.py
+++ b/pygfx/resources/_base.py
@@ -17,6 +17,44 @@ FORMAT_MAP = {
 }
 
 
+def get_item_format_from_memoryview(mem):
+    """Get the per-item format specifier from a memoryview.
+    Returns None if the format appears to be a structured array.
+    Raises an error if GPU-incompatible dtypes are used (64 bit).
+    """
+
+    # Uniform buffers are scalars with a structured dtype.
+    # But can also create storage buffers with complex formats.
+    if len(mem.format) > 2:
+        return None
+
+    # GPUs generally don't support 64-bit buffers or textures.
+    # Note: the Python docs say that l and L are 32 bit, but converting
+    # a int64 numpy array to a memoryview gives a format of 'l' instead
+    # of 'q' on some systems/configs? So we need to check the itemsize.
+    if mem.itemsize == 8:
+        kind = "64-bit"
+        if mem.format in "fd":
+            kind = "float64"
+        elif mem.format in "ilq":
+            kind = "int64"
+        elif mem.format in "ILQ":
+            kind = "uint64"
+        raise ValueError(
+            f"A dtype of {kind} is not supported for buffers, use a 32-bit variant instead."
+        )
+
+    # Get normalized format
+    format = str(mem.format)
+    format = STRUCT_FORMAT_ALIASES.get(format, format)
+
+    if format not in FORMAT_MAP:
+        raise TypeError(
+            f"Cannot convert {format!r} to wgpu format. You should provide data with a different dtype."
+        )
+    return FORMAT_MAP[format]
+
+
 class Resource(Trackable):
     """Resource base class."""
 
diff --git a/pygfx/resources/_buffer.py b/pygfx/resources/_buffer.py
index 201776a..891e4b7 100644
--- a/pygfx/resources/_buffer.py
+++ b/pygfx/resources/_buffer.py
@@ -1,10 +1,10 @@
 import numpy as np
 
-from ._base import Resource, STRUCT_FORMAT_ALIASES, FORMAT_MAP
+from ._base import Resource, get_item_format_from_memoryview
 
 
 class Buffer(Resource):
-    """A contigous piece of GPU memory.
+    """A contiguous piece of GPU memory.
 
     Buffers can be used as index buffer, vertex buffer, uniform buffer, or
     storage buffer. You can provide (and update data for it), or use it as a
@@ -15,7 +15,7 @@ class Buffer(Resource):
     data : array
         The initial data of the array data. It must support the buffer-protocol,
         (e.g. a bytes or numpy array). If None, nbytes and nitems must be
-        provided. The data is copied if it's not float32 or not contiguous.
+        provided.
     nbytes : int
         The size of the buffer in bytes. Ignored if ``data`` is used.
     nitems : int
@@ -25,7 +25,6 @@ class Buffer(Resource):
         buffer. If None, this is automatically set from the data. This must be a
         pygfx format specifier, e.g. "3xf4", but can also be a format specific
         to the render backend if necessary (e.g. from ``wgpu.VertexFormat``).
-
     """
 
     def __init__(
@@ -42,6 +41,7 @@ class Buffer(Resource):
         # To specify the buffer size
         # The actual data (optional)
         self._data = None
+        self._format = None
         self._gfx_pending_uploads = []  # list of (offset, size) tuples
 
         # Backends-specific attributes for internal use
@@ -50,16 +50,13 @@ class Buffer(Resource):
 
         # Get nbytes
         if data is not None:
-            mem = memoryview(data)
-            if mem.format == "d":
-                raise ValueError("Float64 data is not supported, use float32 instead.")
-            # if not mem.contiguous or mem.format == "d":
-            #     format = "f" if mmemformat == "d" else mem.format
-            #     x = np.empty(mem.shape, format)
-            #     x[:] = mem
-            #     mem = memoryview(x)
             self._data = data
-            self._mem = mem
+            self._mem = mem = memoryview(data)
+            subformat = get_item_format_from_memoryview(mem)
+            if subformat:
+                shape = (mem.shape + (1,)) if len(mem.shape) == 1 else mem.shape
+                if len(shape) == 2:  # if not, the user does something fancy
+                    self._format = (f"{shape[-1]}x" + subformat).lstrip("1x")
             the_nbytes = mem.nbytes
             the_nitems = mem.shape[0] if mem.shape else 1
             if the_nitems:
@@ -76,9 +73,11 @@ class Buffer(Resource):
                 "Buffer must be instantiated with either data or nbytes and nitems."
             )
 
+        if format is not None:
+            self._format = str(format)
+
         self._store.nbytes = the_nbytes
         self._store.nitems = the_nitems
-        self._store.format = format
 
         self.draw_range = 0, the_nitems
 
@@ -121,26 +120,21 @@ class Buffer(Resource):
             # This generic solution fails when nitems is zero
             return self._store.nbytes // self._store.nitems
         else:
-            shape = self._data.shape
+            shape = self._mem.shape
             if shape:
                 shape = shape[1:]
             factor = int(np.prod(shape)) or 1
-            return factor * self._data.itemsize
+            return factor * self._mem.itemsize
 
     @property
     def format(self):
-        """The vertex format. Usually a pygfx format specifier (e.g. u2
-        for scalar uint16, or 3xf4 for 3xfloat32), but can also be a
-        overriden to a backend-specific format.
+        """The buffer format.
+
+        Usually a pygfx format specifier (e.g. u2 for scalar uint16,
+        or 3xf4 for 3xfloat32), but can also be a overriden to a
+        backend-specific format. Can also be None e.g. for uniform buffers.
         """
-        format = self._store.format
-        if format is not None:
-            return format
-        elif self.data is not None:
-            self._store["format"] = format_from_memoryview(self.mem)
-            return self._store.format
-        else:
-            raise ValueError("Buffer has no data nor format.")
+        return self._format
 
     @property
     def vertex_byte_range(self):
@@ -221,20 +215,3 @@ class Buffer(Resource):
         # Slice it
         sub_arr = arr[offset : offset + size]
         return memoryview(np.ascontiguousarray(sub_arr))
-
-
-def format_from_memoryview(mem):
-    shape = mem.shape
-    if len(shape) == 1:
-        shape = shape + (1,)
-    assert len(shape) == 2
-    format = str(mem.format)
-    format = STRUCT_FORMAT_ALIASES.get(format, format)
-    if format in ("d", "float64"):
-        raise ValueError("64-bit float is not supported, use 32-bit float instead")
-    elif format not in FORMAT_MAP:
-        raise TypeError(
-            f"Cannot convert {format!r} to vertex format. Maybe specify format?"
-        )
-    format = f"{shape[-1]}x" + FORMAT_MAP[format]
-    return format.lstrip("1x")
diff --git a/pygfx/resources/_texture.py b/pygfx/resources/_texture.py
index b1e2e61..e679cc7 100644
--- a/pygfx/resources/_texture.py
+++ b/pygfx/resources/_texture.py
@@ -1,6 +1,6 @@
 import numpy as np
 
-from ._base import Resource, STRUCT_FORMAT_ALIASES, FORMAT_MAP
+from ._base import Resource, get_item_format_from_memoryview
 
 
 class Texture(Resource):
@@ -14,7 +14,7 @@ class Texture(Resource):
         data : array, optional
             Array data of any type that supports the buffer-protocol, (e.g. a
             bytes or numpy array). If None, nbytes and nitems must be provided.
-            Can be any dtype except float64.
+            The dtype must be compatible with the rendering backend.
         dim : int
             The dimensionality of the array (1, 2 or 3).
         size : tuple, [3]
@@ -54,6 +54,7 @@ class Texture(Resource):
         self._store.dim = int(dim)
         # The actual data (optional)
         self._data = None
+        self._format = None
         self._gfx_pending_uploads = []  # list of (offset, size) tuples
 
         # Backends-specific attributes for internal use
@@ -61,8 +62,6 @@ class Texture(Resource):
         self._wgpu_usage = 0
         self._wgpu_mip_level_count = 1
 
-        self._store.format = None if format is None else str(format)
-
         self._colorspace = (colorspace or "srgb").lower()
         assert self._colorspace in ("srgb", "physical")
 
@@ -71,13 +70,27 @@ class Texture(Resource):
         size = None if size is None else (int(size[0]), int(size[1]), int(size[2]))
 
         if data is not None:
-            mem = memoryview(data)
-            if mem.format == "d":
-                raise ValueError("Float64 data is not supported, use float32 instead.")
             self._data = data
-            self._mem = mem
+            self._mem = mem = memoryview(data)
             self._store.nbytes = mem.nbytes
             self._store.size = self._size_from_data(mem, dim, size)
+            subformat = get_item_format_from_memoryview(mem)
+            if subformat is None:
+                raise ValueError(
+                    f"Unsupported dtype/format for texture data: {mem.format}"
+                )
+            shape = mem.shape
+            collapsed_size = [x for x in self.size if x > 1]
+            if len(shape) == len(collapsed_size) + 1:
+                nchannels = shape[-1]
+            else:
+                assert len(shape) == len(collapsed_size)
+                nchannels = 1
+            if not (1 <= nchannels <= 4):
+                raise ValueError(
+                    f"Expected 1-4 texture color channels, got {nchannels}."
+                )
+            self._format = (f"{nchannels}x" + subformat).lstrip("1x")
             self.update_range((0, 0, 0), self.size)
         elif size is not None and format is not None:
             self._store.size = size
@@ -87,6 +100,9 @@ class Texture(Resource):
                 "Texture must be instantiated with either data or size and format."
             )
 
+        if format is not None:
+            self._format = str(format)
+
     @property
     def rev(self):
         """An integer that is increased when update_range() is called."""
@@ -132,14 +148,7 @@ class Texture(Resource):
         (e.g. u2 for scalar uint16, or 3xf4 for RGB float32),
         but can also be a overriden to a backend-specific format.
         """
-        format = self._store.format
-        if format is not None:
-            return format
-        elif self.data is not None:
-            self._store["format"] = format_from_memoryview(self.mem, self.size)
-            return self._store.format
-        else:
-            raise ValueError("Texture has no data nor format.")
+        return self._format
 
     @property
     def colorspace(self):
@@ -244,25 +253,3 @@ class Texture(Resource):
         raise DeprecationWarning(
             "Texture.get_view() is removed, TextureView is no longer public API: just use plain textures."
         )
-
-
-def format_from_memoryview(mem, size):
-    format = str(mem.format)
-    format = STRUCT_FORMAT_ALIASES.get(format, format)
-    # Process channels
-    shape = mem.shape
-    collapsed_size = [x for x in size if x > 1]
-    if len(shape) == len(collapsed_size) + 1:
-        nchannels = shape[-1]
-    else:
-        assert len(shape) == len(collapsed_size)
-        nchannels = 1
-    assert 1 <= nchannels <= 4
-    if format in ("d", "float64"):
-        raise TypeError("GPU's don't support float64 texture formats.")
-    elif format not in FORMAT_MAP:
-        raise TypeError(
-            f"Cannot convert {format!r} to texture format. Maybe specify format?"
-        )
-    format = f"{nchannels}x" + FORMAT_MAP[format]
-    return format.lstrip("1x")
diff --git a/pygfx/utils/trackable.py b/pygfx/utils/trackable.py
index 9e172a4..87d0cfe 100644
--- a/pygfx/utils/trackable.py
+++ b/pygfx/utils/trackable.py
@@ -287,7 +287,7 @@ class Root:
                 for label in labels:
                     self._track_store(new_value._store, label)
             # If this was and is a trackable, we only process labels starting with "!"
-            if is_trackable == 3 and type(old_value) == type(new_value):
+            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


Test patch:
diff --git a/tests/geometries/test_geometry.py b/tests/geometries/test_geometry.py
new file mode 100644
index 0000000..9be8ba1
--- /dev/null
+++ b/tests/geometries/test_geometry.py
@@ -0,0 +1,50 @@
+import numpy as np
+import pygfx as gfx
+import pytest
+
+
+def test_different_data_types():
+    # Numpy array, let's do float16 while we're at it
+    a = np.zeros((10, 3), np.float16)
+    g = gfx.Geometry(foo=a)
+    assert g.foo.format == "3xf2"
+    assert g.foo.data is a
+
+    # Memoryview
+    a = memoryview(np.zeros((10, 2), np.int16))
+    g = gfx.Geometry(foo=a)
+    assert g.foo.format == "2xi2"
+    assert g.foo.data is a
+
+    # Bytes
+    a = b"0000000000000000"
+    g = gfx.Geometry(foo=a)
+    assert g.foo.format == "u1"
+    assert g.foo.data is a
+
+    # Lists
+    a = [[1, 2, 3], [4, 5, 6], [7, 8, 9]]
+    g = gfx.Geometry(indices=a)
+    assert g.indices.format == "3xi4"  # <-- indices must be int
+    assert isinstance(g.indices.data, np.ndarray)
+    g = gfx.Geometry(positions=a)
+    assert g.positions.format == "3xf4"  # <-- positions are usually f32
+    assert isinstance(g.positions.data, np.ndarray)
+    g = gfx.Geometry(foo=a)
+    assert g.foo.format == "3xf4"  # <-- assumed that f32 is best
+    assert isinstance(g.foo.data, np.ndarray)
+
+
+def test_check_positions():
+    # ok
+    a = np.zeros((10, 3), np.float32)
+    g = gfx.Geometry(positions=a)
+    assert g.positions.data is a
+
+    a = np.zeros((10, 4), np.float32)
+    with pytest.raises(ValueError):
+        gfx.Geometry(positions=a)
+
+    a = np.zeros((10, 2), np.float32)
+    with pytest.raises(ValueError):
+        gfx.Geometry(positions=a)
diff --git a/tests/resources/test_buffer.py b/tests/resources/test_buffer.py
index 7a0da3b..593ed86 100644
--- a/tests/resources/test_buffer.py
+++ b/tests/resources/test_buffer.py
@@ -1,5 +1,6 @@
 import numpy as np
 import pygfx as gfx
+import pytest
 
 
 def test_empty_data():
@@ -28,3 +29,57 @@ def test_nonempty_data():
     b = gfx.Buffer(np.zeros((2, 4, 5), np.float32))
     assert b.draw_range == (0, 2)
     assert b.itemsize == 20 * 4
+
+
+def test_different_data_types():
+    # Numpy array, let's do float16 while we're at it
+    a = np.zeros((10, 3), np.float16)
+    b = gfx.Buffer(a)
+    assert b.format == "3xf2"
+
+    # Memoryview
+    a = memoryview(np.zeros((10, 2), np.int16))
+    b = gfx.Buffer(a)
+    assert b.format == "2xi2"
+
+    # Bytes
+    a = b"0000000000000000"
+    b = gfx.Buffer(a)
+    assert b.format == "u1"
+
+    # You can specify the format ...
+    b = gfx.Buffer(a, format="2xf4")
+    assert b.format == "2xf4"
+    # ... but some of the props will be wrong, so probably a bad idea
+    assert b.nitems == 16
+    assert b.itemsize == 1
+
+    # Lists not supported
+    with pytest.raises(TypeError):
+        gfx.Buffer([1, 2, 3, 4, 5])
+
+
+def test_unsupported_dtypes():
+    a = np.zeros((10, 3), np.float64)
+    with pytest.raises(ValueError):
+        gfx.Buffer(a)
+
+    a = np.zeros((10, 3), np.int64)
+    with pytest.raises(ValueError):
+        gfx.Buffer(a)
+
+    a = np.zeros((10, 3), np.uint64)
+    with pytest.raises(ValueError):
+        gfx.Buffer(a)
+
+
+def test_special_data():
+    # E.g. uniform buffers, storage buffers with structured dtype, multidimensional storage data.
+
+    a = np.zeros((10, 3, 2), np.float16)
+    b = gfx.Buffer(a)
+    assert b.format is None
+
+    a = np.zeros((), dtype=[("a", "<i4"), ("b", "<f4")])
+    b = gfx.Buffer(a)
+    assert b.format is None
diff --git a/tests/resources/test_texture.py b/tests/resources/test_texture.py
new file mode 100644
index 0000000..d298135
--- /dev/null
+++ b/tests/resources/test_texture.py
@@ -0,0 +1,111 @@
+import numpy as np
+import pygfx as gfx
+import pytest
+
+
+def test_different_data_types():
+    # Numpy array, let's do float16 while we're at it
+    a = np.zeros((10, 10), np.float16)
+    b = gfx.Texture(a, dim=2)
+    assert b.format == "f2"
+
+    # Memoryview
+    a = memoryview(np.zeros((10, 10), np.int16))
+    b = gfx.Texture(a, dim=2)
+    assert b.format == "i2"
+
+    # Lists not supported
+    with pytest.raises(TypeError):
+        gfx.Texture([1, 2, 3, 4, 5], dim=1)
+
+
+def test_unsupported_dtypes():
+    a = np.zeros((10, 10), np.float64)
+    with pytest.raises(ValueError):
+        gfx.Texture(a, dim=2)
+
+    a = np.zeros((10, 10), np.int64)
+    with pytest.raises(ValueError):
+        gfx.Texture(a, dim=2)
+
+    a = np.zeros((10, 10), np.uint64)
+    with pytest.raises(ValueError):
+        gfx.Texture(a, dim=2)
+
+
+def test_supported_shapes():
+    # 1D
+    for i in range(4):
+        if i == 0:
+            a = np.zeros((10,), np.float32)
+        else:
+            a = np.zeros((10, i), np.float32)
+        t = gfx.Texture(a, dim=1)
+        assert t.size == (10, 1, 1)
+
+    # 2D
+    for i in range(4):
+        if i == 0:
+            a = np.zeros((10, 10), np.float32)
+        else:
+            a = np.zeros((10, 10, i), np.float32)
+        t = gfx.Texture(a, dim=2)
+        assert t.size == (10, 10, 1)
+
+    # 3D
+    for i in range(4):
+        if i == 0:
+            a = np.zeros((10, 10, 10), np.float32)
+        else:
+            a = np.zeros((10, 10, 10, i), np.float32)
+        t = gfx.Texture(a, dim=3)
+        assert t.size == (10, 10, 10)
+
+    # Stack of 1D images
+    a = np.zeros((10, 10), np.float32)
+    t = gfx.Texture(a, dim=1, size=(10, 10, 1))
+
+    # Stack of 2D images
+    a = np.zeros((10, 10, 10), np.float32)
+    t = gfx.Texture(a, dim=2, size=(10, 10, 10))
+
+
+def test_unsupported_shapes():
+    # Always fail on empty array
+    a = np.zeros((), np.float32)
+    for dim in (1, 2, 3):
+        with pytest.raises(ValueError):
+            gfx.Texture(a, dim=dim)
+
+    # Always fail on 4D array
+    a = np.zeros((5, 5, 5, 5), np.float32)
+    for dim in (1, 2, 3):
+        with pytest.raises(ValueError):
+            gfx.Texture(a, dim=dim)
+
+    # 1D
+    for i in [0, 5, 6]:
+        a = np.zeros((10, i), np.float32)
+        with pytest.raises(ValueError):
+            gfx.Texture(a, dim=1)
+
+    # 2D
+    a = np.zeros((10,), np.float32)
+    with pytest.raises(ValueError):
+        gfx.Texture(a, dim=3)
+    for i in [0, 5, 6]:
+        a = np.zeros((10, 10, i), np.float32)
+        with pytest.raises(ValueError):
+            gfx.Texture(a, dim=2)
+
+    # 3D
+    a = np.zeros((10,), np.float32)
+    with pytest.raises(ValueError):
+        gfx.Texture(a, dim=3)
+    a = np.zeros((10, 10), np.float32)
+    with pytest.raises(ValueError):
+        gfx.Texture(a, dim=3)
+    for i in [0, 5, 6]:
+        a = np.zeros((10, 10, 10, i), np.float32)
+        with pytest.raises(ValueError):
+            gfx.Texture(a, dim=3)