Speed up SpriteList swap, insert, setitem, and buffer uploads

CHANGELOG.md

@@ -27,6 +27,7 @@ Arcade [PyPi Release History](https://pypi.org/project/arcade/#history) page.
 - Fixed `SpriteList.rescale()` moving the list's center while rescaling, so sprites after the first were scaled around the wrong point.
 - Fixed `SpriteList.preload_textures()` raising `AttributeError` on a lazy sprite list that hadn't been drawn yet. It now preloads into the atlas the list will use.
 - Fixed GPU collision checks (`CollisionMethod.GPU`, also used automatically for sprite lists over 1500 sprites without a spatial hash) missing sprites flipped with a negative scale, whose negative width or height made them look smaller than they are.
+- Fixed `SpriteList.__setitem__` raising when setting a negative index to the sprite already there, such as `sprite_list[-1] = sprite_list[-1]`.
 
 ### New Features
 - Added `HitBox.get_adjusted_bounds()`, which returns the cached `(left, right, bottom, top)` bounds of the adjusted hit box points.
@@ -35,6 +36,7 @@ Arcade [PyPi Release History](https://pypi.org/project/arcade/#history) page.
 - Added `arcade.check_for_collision_between_lists(list_a, list_b)`, which returns every colliding `(sprite_a, sprite_b)` pair between two lists, such as bullets and enemies. Passing the same list twice returns each pair once.
 
 ### Misc Changes
+- Sped up several `SpriteList` operations: `swap()` no longer searches the draw order (about 1000x faster at the end of a 10,000 sprite list), `insert()` and item assignment check membership with a dictionary instead of scanning the list, and drawing uploads only the buffer slots in use instead of the whole capacity (moving one sprite and drawing is 1.6-2.3x faster).
 - The platformer and simple physics engines and `AStarBarrierList` now use `has_collision_with_list(s)` where they only need to know whether there's a collision. Building an `AStarBarrierList` is about 7-9% faster.
 - Sped up sprite collision checks. Sprites that pass the quick distance check are now compared by cached hit box bounds before the polygon test, and the polygon test skips horizontal and vertical edges, which the bounds check already covers. Checks that reach the polygon test are about 2-4x faster, e.g. 8.0 to 2.3 µs for two box hit boxes and 21.6 to 10.2 µs for two default octagon hit boxes. `are_polygons_intersecting` is also faster (7.3 to 1.6 µs for two rectangles).
 - Sped up collision checks further by caching each hit box's distinct edge directions. Parallel edges (such as opposite sides of the default octagon hit boxes, or matching edges on two sprites with the same angle) are only tested once, and the cache is kept when a sprite moves. Two unrotated octagon hit boxes go from 12.0 to 4.6 µs, and two rotated 30° from 20.8 to 6.9 µs.

arcade/sprite_list/sprite_list.py

@@ -367,15 +367,13 @@ def __getitem__(self, i: int) -> SpriteType:
 
     def __setitem__(self, index: int, sprite: SpriteType) -> None:
         """Replace a sprite at a specific index"""
-        try:
-            existing_index = self.sprite_list.index(sprite)  # raise ValueError
-            if existing_index == index:
+        sprite_to_be_removed = self.sprite_list[index]  # Raises IndexError
+        if sprite in self.sprite_slot:
+            if sprite is sprite_to_be_removed:
                 return
+            existing_index = self.sprite_list.index(sprite)
             raise Exception(f"Sprite is already in the list (index {existing_index})")
-        except ValueError:
-            pass
 
-        sprite_to_be_removed = self.sprite_list[index]
         sprite_to_be_removed._unregister_sprite_list(self)
         self.sprite_list[index] = sprite  # Replace sprite
         sprite.register_sprite_list(self)
@@ -680,13 +678,16 @@ def swap(self, index_1: int, index_2: int) -> None:
         self.sprite_list[index_1] = sprite_2
         self.sprite_list[index_2] = sprite_1
 
-        # Swap order in index buffer to change rendering order
-        slot_1 = self.sprite_slot[sprite_1]
-        slot_2 = self.sprite_slot[sprite_2]
-        i1 = self._sprite_index_data.index(slot_1)
-        i2 = self._sprite_index_data.index(slot_2)
-        self._sprite_index_data[i1] = slot_2
-        self._sprite_index_data[i2] = slot_1
+        # Swap order in index buffer to change rendering order. It's in the
+        # same order as the sprite list, but longer (it has spare capacity at
+        # the end), so negative indexes must be made positive first.
+        sprite_count = len(self.sprite_list)
+        if index_1 < 0:
+            index_1 += sprite_count
+        if index_2 < 0:
+            index_2 += sprite_count
+        index_data = self._sprite_index_data
+        index_data[index_1], index_data[index_2] = index_data[index_2], index_data[index_1]
 
         self._sprite_index_changed = True
 
@@ -739,7 +740,7 @@ def insert(self, index: int, sprite: SpriteType) -> None:
             index: The index at which to insert
             sprite: The sprite to insert
         """
-        if sprite in self.sprite_list:
+        if sprite in self.sprite_slot:
             raise ValueError("Sprite is already in list")
 
         index = max(min(len(self.sprite_list), index), 0)
@@ -953,6 +954,9 @@ def _write_sprite_buffers_to_gpu(self) -> None:
             self._sprite_color_changed,
             self._sprite_texture_changed,
             self._sprite_index_changed,
+            # Only the slots in use need writing, not the spare capacity
+            slot_count=self._sprite_buffer_slots,
+            index_count=self._sprite_index_slots,
         )
         self._sprite_pos_angle_changed = False
         self._sprite_size_changed = False
@@ -1353,6 +1357,8 @@ def write_sprite_buffers_to_gpu(
         sprite_color_changed: bool = True,
         sprite_texture_changed: bool = True,
         sprite_index_changed: bool = True,
+        slot_count: int | None = None,
+        index_count: int | None = None,
     ) -> None:
         """
         Write the sprite buffers to the GPU.
@@ -1368,6 +1374,10 @@ def write_sprite_buffers_to_gpu(
             sprite_color_changed: Whether the color data has changed.
             sprite_texture_changed: Whether the texture data has changed.
             sprite_index_changed: Whether the index data has changed.
+            slot_count: How many sprite buffer slots are in use. Only these
+                are written if given, instead of the whole arrays.
+            index_count: How many entries of the index data are in use. Only
+                these are written if given, instead of the whole array.
         """
         raise NotImplementedError("This method should be implemented in subclasses.")
 
@@ -1567,6 +1577,8 @@ def write_sprite_buffers_to_gpu(
         sprite_color_changed: bool = True,
         sprite_texture_changed: bool = True,
         sprite_index_changed: bool = True,
+        slot_count: int | None = None,
+        index_count: int | None = None,
     ) -> None:
         """
         Write the sprite buffers to the GPU.
@@ -1581,7 +1593,21 @@ def write_sprite_buffers_to_gpu(
             sprite_color_changed: Whether the color data has changed.
             sprite_texture_changed: Whether the texture data has changed.
             sprite_index_changed: Whether the index data has changed.
-        """
+            slot_count: How many sprite buffer slots are in use. Only these
+                are written if given, instead of the whole arrays.
+            index_count: How many entries of the index data are in use. Only
+                these are written if given, instead of the whole array.
+        """
+        # Orphaning leaves the rest of each buffer undefined, which is fine:
+        # the index buffer only refers to slots below slot_count.
+        if slot_count is not None:
+            sprite_pos_angle_data = memoryview(sprite_pos_angle_data)[: slot_count * 4]
+            sprite_size_data = memoryview(sprite_size_data)[: slot_count * 2]
+            sprite_color_data = memoryview(sprite_color_data)[: slot_count * 4]
+            sprite_texture_data = memoryview(sprite_texture_data)[:slot_count]
+        if index_count is not None:
+            sprite_index_data = memoryview(sprite_index_data)[:index_count]
+
         if sprite_pos_angle_changed:
             self._storage_pos_angle.orphan()
             self._storage_pos_angle.write(sprite_pos_angle_data)
@@ -1777,6 +1803,8 @@ def write_sprite_buffers_to_gpu(
         sprite_color_changed: bool = True,
         sprite_texture_changed: bool = True,
         sprite_index_changed: bool = True,
+        slot_count: int | None = None,
+        index_count: int | None = None,
     ) -> None:
         """
         Write the sprite buffers to the GPU.
@@ -1792,6 +1820,10 @@ def write_sprite_buffers_to_gpu(
             sprite_color_changed: Whether the color data has changed.
             sprite_texture_changed: Whether the texture data has changed.
             sprite_index_changed: Whether the index data has changed.
+            slot_count: How many sprite buffer slots are in use. Only these
+                are written if given, instead of the whole arrays.
+            index_count: How many entries of the index data are in use. Only
+                these are written if given, instead of the whole array.
         """
         if sprite_pos_angle_changed:
             self._storage_pos_angle.write(sprite_pos_angle_data)

tests/unit/spritelist/test_spritelist.py

@@ -421,3 +421,99 @@ def test_index_buffer_type(ctx):
     assert spritelist._sprite_index_data.typecode == "I"
     spritelist.clear()
     assert spritelist._sprite_index_data.typecode == "I"
+
+
+@pytest.mark.parametrize(
+    "index_1, index_2", [(0, 1), (0, 4), (1, 3), (-1, -2), (-1, 0), (2, -1), (-5, -1), (3, 3)]
+)
+def test_swap_draw_order(ctx, index_1, index_2):
+    """swap() keeps the GPU draw order in sync, including with negative indexes"""
+    spritelist = make_named_sprites(5)
+    expected = list(spritelist)
+    spritelist.draw()
+
+    spritelist.swap(index_1, index_2)
+    expected[index_1], expected[index_2] = expected[index_2], expected[index_1]
+    assert list(spritelist) == expected
+    assert _drawn_sprites(spritelist) == expected
+
+
+@pytest.mark.parametrize("index_1, index_2", [(0, 5), (-6, 0), (100, 1)])
+def test_swap_out_of_range(ctx, index_1, index_2):
+    spritelist = make_named_sprites(5)
+    sprites = list(spritelist)
+    with pytest.raises(IndexError):
+        spritelist.swap(index_1, index_2)
+    assert list(spritelist) == sprites
+    assert _drawn_sprites(spritelist) == sprites
+
+
+def test_setitem_negative_index(ctx):
+    spritelist = make_named_sprites(3)
+    sprites = list(spritelist)
+    # Setting a sprite to the position it's already at does nothing
+    spritelist[-1] = sprites[2]
+    assert list(spritelist) == sprites
+    # A sprite already elsewhere in the list can't be added again
+    with pytest.raises(Exception):
+        spritelist[-1] = sprites[0]
+    with pytest.raises(IndexError):
+        spritelist[3] = arcade.SpriteSolidColor(16, 16)
+
+    new_sprite = arcade.SpriteSolidColor(16, 16)
+    spritelist[-2] = new_sprite
+    assert list(spritelist) == [sprites[0], new_sprite, sprites[2]]
+    assert _drawn_sprites(spritelist) == [sprites[0], new_sprite, sprites[2]]
+
+
+def test_insert_already_in_list(ctx):
+    spritelist = make_named_sprites(3)
+    with pytest.raises(ValueError):
+        spritelist.insert(0, spritelist[2])
+    assert len(spritelist) == 3
+
+
+def _gpu_floats(buffer, count):
+    return list(struct.unpack(f"{count}f", buffer.read()[: count * 4]))
+
+
+def test_gpu_buffers_match_after_changes(ctx):
+    """Only the slots in use are uploaded, so check the GPU data still matches"""
+    import random
+
+    rng = random.Random(5)
+    # Start small so the buffers have to grow
+    spritelist = arcade.SpriteList(capacity=256)
+    for i in range(300):
+        spritelist.append(arcade.SpriteSolidColor(8, 8, center_x=i, center_y=-i))
+
+    for step in range(200):
+        action = rng.random()
+        if action < 0.3 and len(spritelist) > 1:
+            spritelist.pop(rng.randrange(-len(spritelist), len(spritelist)))
+        elif action < 0.5:
+            # Reuses a freed slot if there is one
+            spritelist.append(arcade.SpriteSolidColor(8, 8, center_x=rng.uniform(0, 500)))
+        elif action < 0.6:
+            spritelist.insert(rng.randrange(len(spritelist)), arcade.SpriteSolidColor(4, 4))
+        elif action < 0.7:
+            spritelist.swap(rng.randrange(len(spritelist)), -rng.randrange(1, len(spritelist)))
+        else:
+            sprite = spritelist[rng.randrange(len(spritelist))]
+            sprite.position = rng.uniform(-100, 100), rng.uniform(-100, 100)
+            sprite.angle = rng.uniform(0, 360)
+            sprite.width = rng.uniform(1, 50)
+        if step % 10 == 0:
+            spritelist.draw()
+
+    assert _drawn_sprites(spritelist) == list(spritelist)
+    data = spritelist.data
+    slot_count = spritelist._sprite_buffer_slots
+    pos = _gpu_floats(data.storage_positions_angle, slot_count * 4)
+    size = _gpu_floats(data.storage_size, slot_count * 2)
+    for sprite in spritelist:
+        slot = spritelist.sprite_slot[sprite]
+        assert pos[slot * 4 : slot * 4 + 4] == pytest.approx(
+            [sprite.center_x, sprite.center_y, sprite.depth, sprite.angle]
+        )
+        assert size[slot * 2 : slot * 2 + 2] == pytest.approx([sprite.width, sprite.height])