Prevent desync of currentCostume

src/sprites/rendered-target.js

@@ -505,18 +505,28 @@ class RenderedTarget extends Target {
             return null;
         }
 
-        const deletedCostume = this.sprite.deleteCostumeAt(index);
+        const deletedCostume = this.sprite.deleteCostumeAt(index, true); // Update current costume
 
-        if (index === this.currentCostume && index === originalCostumeCount - 1) {
+        this.runtime.requestTargetsUpdate(this);
+        return deletedCostume;
+    }
+
+    /**
+     * Shift current costume index if needed and re-render this target.
+     * This needs to be called on all clones to prevent desync crash. (#2661)
+     * @param {number} index Index of deleted costume
+     * @param {boolean} isLastCostume Whether the deleted costume was the last costume or not
+     */
+    shiftCurrentCostume (index, isLastCostume) {
+        if (index === this.currentCostume && isLastCostume) {
             this.setCostume(index - 1);
         } else if (index < this.currentCostume) {
             this.setCostume(this.currentCostume - 1);
         } else {
+            // Index is still the same; however, the costume itself may have changed.
+            // This causes the renderer to re-render.
             this.setCostume(this.currentCostume);
         }
-
-        this.runtime.requestTargetsUpdate(this);
-        return deletedCostume;
     }
 
     /**
@@ -630,14 +640,15 @@ class RenderedTarget extends Target {
 
         if (newIndex === costumeIndex) return false;
 
-        const currentCostume = this.getCurrentCostume();
+        this.sprite.saveCurrentCostumeName();
         const costume = this.sprite.costumes[costumeIndex];
 
         // Use the sprite method for deleting costumes because setCostume is handled manually
         this.sprite.deleteCostumeAt(costumeIndex);
 
         this.addCostume(costume, newIndex);
-        this.currentCostume = this.getCostumeIndexByName(currentCostume.name);
+        // This sets currentCostume
+        this.sprite.restoreCurrentCostume();
         return true;
     }
 

src/sprites/sprite.js

@@ -92,12 +92,43 @@ class Sprite {
     }
 
     /**
-     * Delete a costume by index.
+     * Delete a costume by index, and optionally update current costume for all clones.
      * @param {number} index Costume index to be deleted
+     * @param {boolean} [optUpdateCurrentCostume] Whether or not to update current costume for all clones
      * @return {?object} The deleted costume
      */
-    deleteCostumeAt (index) {
-        return this.costumes.splice(index, 1)[0];
+    deleteCostumeAt (index, optUpdateCurrentCostume) {
+        const isLastCostume = index === this.costumes.length - 1;
+        const deletedCostume = this.costumes.splice(index, 1)[0];
+        if (optUpdateCurrentCostume) {
+            for (const target of this.clones) {
+                target.shiftCurrentCostume(index, isLastCostume);
+            }
+        }
+        return deletedCostume;
+    }
+
+    /**
+     * Save current costume temporarliy during reordering, for all clones.
+     */
+    saveCurrentCostumeName () {
+        for (const target of this.clones) {
+            target._currentCostumeName = target.getCurrentCostume().name;
+        }
+    }
+
+    /**
+     * Restore current costume from saved name.
+     */
+    restoreCurrentCostume () {
+        for (const target of this.clones) {
+            if (typeof target._currentCostumeName === 'undefined') continue;
+            const costumeIndex = this.costumes.findIndex(c => c.name === target._currentCostumeName);
+            delete target._currentCostumeName;
+            if (costumeIndex !== target.currentCostume) {
+                target.setCostume(costumeIndex);
+            }
+        }
     }
 
     /**

test/unit/sprites_rendered-target.js

@@ -119,6 +119,7 @@ test('deleteCostume', t => {
     const a = new RenderedTarget(s, r);
     const renderer = new FakeRenderer();
     a.renderer = renderer;
+    s.clones = [a];
 
     // x* Costume 1        * Costume 2
     //    Costume 2   =>     Costume 3
@@ -211,6 +212,70 @@ test('deleteCostume', t => {
     t.end();
 });
 
+test('deleteCostume shifts current costume (1/2)', t => {
+    const o1 = {id: 1};
+    const o2 = {id: 2};
+    const o3 = {id: 3};
+
+    const r = new Runtime();
+    const s = new Sprite(null, r);
+    s.costumes = [o1, o2, o3];
+    const renderer = new FakeRenderer();
+
+    const c1 = new RenderedTarget(s, r);
+    c1.renderer = renderer;
+    c1.currentCostume = 0;
+
+    const c2 = new RenderedTarget(s, r);
+    c2.renderer = renderer;
+    c2.currentCostume = 1;
+
+    const c3 = new RenderedTarget(s, r);
+    c3.renderer = renderer;
+    c3.currentCostume = 2;
+
+    s.clones = [c1, c2, c3];
+
+    c1.deleteCostume(0);
+    t.equals(c1.currentCostume, 0);
+    t.equals(c2.currentCostume, 0);
+    t.equals(c3.currentCostume, 1);
+
+    t.end();
+});
+
+test('deleteCostume shifts current costume (2/2)', t => {
+    const o1 = {id: 1};
+    const o2 = {id: 2};
+    const o3 = {id: 3};
+
+    const r = new Runtime();
+    const s = new Sprite(null, r);
+    s.costumes = [o1, o2, o3];
+    const renderer = new FakeRenderer();
+
+    const c1 = new RenderedTarget(s, r);
+    c1.renderer = renderer;
+    c1.currentCostume = 0;
+
+    const c2 = new RenderedTarget(s, r);
+    c2.renderer = renderer;
+    c2.currentCostume = 1;
+
+    const c3 = new RenderedTarget(s, r);
+    c3.renderer = renderer;
+    c3.currentCostume = 2;
+
+    s.clones = [c1, c2, c3];
+
+    c1.deleteCostume(2);
+    t.equals(c1.currentCostume, 0);
+    t.equals(c2.currentCostume, 1);
+    t.equals(c3.currentCostume, 1);
+
+    t.end();
+});
+
 test('deleteSound', t => {
     const o1 = {id: 1};
     const o2 = {id: 2};
@@ -474,12 +539,16 @@ test('#reorderCostume', t => {
     const r = new Runtime();
     const s = new Sprite(null, r);
     s.costumes = [o1, o2, o3, o4, o5];
-    const a = new RenderedTarget(s, r);
     const renderer = new FakeRenderer();
+    const a = new RenderedTarget(s, r);
     a.renderer = renderer;
+    const b = new RenderedTarget(s, r);
+    b.renderer = renderer;
+    s.clones = [a, b];
 
     const resetCostumes = () => {
         a.setCostume(0);
+        b.setCostume(0);
         s.costumes = [o1, o2, o3, o4, o5];
     };
     const costumeIds = () => a.sprite.costumes.map(c => c.id);
@@ -487,6 +556,7 @@ test('#reorderCostume', t => {
     resetCostumes();
     t.deepEquals(costumeIds(), [0, 1, 2, 3, 4]);
     t.equals(a.currentCostume, 0);
+    t.equals(b.currentCostume, 0);
 
     // Returns false if the costumes are the same and no change occurred
     t.equal(a.reorderCostume(3, 3), false);
@@ -495,15 +565,19 @@ test('#reorderCostume', t => {
 
     // Make sure reordering up and down works and current costume follows
     resetCostumes();
+    b.setCostume(4);
     t.equal(a.reorderCostume(0, 3), true);
     t.deepEquals(costumeIds(), [1, 2, 3, 0, 4]);
     t.equals(a.currentCostume, 3); // Index of id=0
+    t.equals(b.currentCostume, 4); // Index of id=4
 
     resetCostumes();
     a.setCostume(1);
+    b.setCostume(3);
     t.equal(a.reorderCostume(3, 1), true);
     t.deepEquals(costumeIds(), [0, 3, 1, 2, 4]);
     t.equals(a.currentCostume, 2); // Index of id=1
+    t.equals(b.currentCostume, 1); // Index of id=3
 
     // Out of bounds indices get clamped
     resetCostumes();