From 95e0e9fa332252546101aecf0587a9920cd8f199 Mon Sep 17 00:00:00 2001
From: Vivacious Box <jon@rouillard.org>
Date: Sun, 18 May 2025 19:49:29 +0200
Subject: [PATCH] Fix regression: put sprite clearing in a distinct method to
 make the intent clearer (#800)

## Description:
Fix regression on unite sprites drawing, the old code which used 2 for
loops on the same object looked like there was an oversight.
By splitting it in 2 function calls and adding a comment the intent
should be clearer.


Before:

![image](https://github.com/user-attachments/assets/b396f35c-1ca0-4622-a93c-1ad2f728b5b9)

After:

![image](https://github.com/user-attachments/assets/17aa2e0a-4627-42a7-90d4-c6685b22a380)


## Please complete the following:

- [x] I have added screenshots for all UI updates
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced
- [x] I understand that submitting code with bugs that could have been
caught through manual testing blocks releases and new features for all
contributors

## Please put your Discord username so you can be contacted if a bug or
regression is found:

<DISCORD USERNAME>
---
 src/client/graphics/layers/UnitLayer.ts | 43 +++++++++++++++----------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/src/client/graphics/layers/UnitLayer.ts b/src/client/graphics/layers/UnitLayer.ts
index 05fca583f..5ec98850c 100644
--- a/src/client/graphics/layers/UnitLayer.ts
+++ b/src/client/graphics/layers/UnitLayer.ts
@@ -222,26 +222,35 @@ export class UnitLayer implements Layer {
     const unitsToUpdate = this.game
       .updatesSinceLastTick()
       ?.[GameUpdateType.Unit]?.map((unit) => this.game.unit(unit.id))
-      ?.forEach((unitView) => {
-        if (unitView === undefined) return;
-        const ready = isSpriteReady(unitView.type());
-        if (ready) this.clearUnitCells(unitView);
-        this.onUnitEvent(unitView);
+      .filter((unit) => unit !== undefined);
+
+    if (unitsToUpdate) {
+      // the clearing and drawing of unit sprites need to be done in 2 passes
+      // otherwise the sprite of a unit can be drawn on top of another unit
+      this.clearUnitsCells(unitsToUpdate);
+      this.drawUnitsCells(unitsToUpdate);
+    }
+  }
+
+  private clearUnitsCells(unitViews: UnitView[]) {
+    unitViews
+      .filter((unitView) => isSpriteReady(unitView.type()))
+      .forEach((unitView) => {
+        const sprite = getColoredSprite(unitView, this.theme);
+        const clearsize = sprite.width + 1;
+        const lastX = this.game.x(unitView.lastTile());
+        const lastY = this.game.y(unitView.lastTile());
+        this.context.clearRect(
+          lastX - clearsize / 2,
+          lastY - clearsize / 2,
+          clearsize,
+          clearsize,
+        );
       });
   }
 
-  private clearUnitCells(unit: UnitView) {
-    const sprite = getColoredSprite(unit, this.theme);
-    const clearsize = sprite.width + 1;
-
-    const lastX = this.game.x(unit.lastTile());
-    const lastY = this.game.y(unit.lastTile());
-    this.context.clearRect(
-      lastX - clearsize / 2,
-      lastY - clearsize / 2,
-      clearsize,
-      clearsize,
-    );
+  private drawUnitsCells(unitViews: UnitView[]) {
+    unitViews.forEach((unitView) => this.onUnitEvent(unitView));
   }
 
   private relationship(unit: UnitView): Relationship {