Maintain list of targets in layer order

This matches how Scratch behaves, and means we don't have to mess around
with _layerOrder anymore.

Author: adroitwhiz

src/Project.ts

@@ -3,7 +3,7 @@ import Renderer from "./Renderer";
 import Input from "./Input";
 import LoudnessHandler from "./Loudness";
 import Sound from "./Sound";
-import type { Stage, Sprite } from "./Sprite";
+import { Stage, Sprite } from "./Sprite";
 
 type TriggerWithTarget = {
   target: Sprite | Stage;
@@ -13,6 +13,13 @@ type TriggerWithTarget = {
 export default class Project {
   public stage: Stage;
   public sprites: Partial<Record<string, Sprite>>;
+  /**
+   * All rendered targets (the stage, sprites, and clones), in layer order.
+   * This is kept private so that nobody can improperly modify it. The only way
+   * to add or remove targets is via the appropriate methods, and iteration can
+   * be done with {@link forEachTarget}.
+   */
+  private targets: (Sprite | Stage)[];
   public renderer: Renderer;
   public input: Input;
 
@@ -34,12 +41,29 @@ export default class Project {
     this.stage = stage;
     this.sprites = sprites;
 
-    Object.freeze(sprites); // Prevent adding/removing sprites while project is running
+    this.targets = [
+      stage,
+      ...Object.values(this.sprites as Record<string, Sprite>),
+    ];
+    this.targets.sort((a, b) => {
+      // There should only ever be one stage, but it's best to maintain a total
+      // ordering to avoid weird sorting-algorithm stuff from happening if
+      // there's more than one
+      if (a instanceof Stage && !(b instanceof Stage)) {
+        return -1;
+      }
+      if (b instanceof Stage && !(a instanceof Stage)) {
+        return 1;
+      }
 
-    for (const sprite of this.spritesAndClones) {
-      sprite._project = this;
+      return a.getInitialLayerOrder() - b.getInitialLayerOrder();
+    });
+    for (const target of this.targets) {
+      target.clearInitialLayerOrder();
+      target._project = this;
     }
-    this.stage._project = this;
+
+    Object.freeze(sprites); // Prevent adding/removing sprites while project is running
 
     this.renderer = new Renderer(this, null);
     this.input = new Input(this.stage, this.renderer.stage, (key) => {
@@ -77,7 +101,7 @@ export default class Project {
         void Sound.audioContext.resume();
       }
 
-      let clickedSprite = this.renderer.pick(this.spritesAndClones, {
+      let clickedSprite = this.renderer.pick(this.targets, {
         x: this.input.mouse.x,
         y: this.input.mouse.y,
       });
@@ -113,8 +137,7 @@ export default class Project {
     triggerMatches: (tr: Trigger, target: Sprite | Stage) => boolean
   ): TriggerWithTarget[] {
     const matchingTriggers: TriggerWithTarget[] = [];
-    const targets = this.spritesAndStage;
-    for (const target of targets) {
+    for (const target of this.targets) {
       const matchingTargetTriggers = target.triggers.filter((tr) =>
         triggerMatches(tr, target)
       );
@@ -198,15 +221,13 @@ export default class Project {
       this.stopAllSounds();
       this.runningTriggers = [];
 
-      for (const spriteName in this.sprites) {
-        const sprite = this.sprites[spriteName]!;
-        sprite.clones = [];
-      }
+      this.filterSprites((sprite) => {
+        if (!sprite.isOriginal) return false;
 
-      for (const sprite of this.spritesAndStage) {
         sprite.effects.clear();
         sprite.audioEffects.clear();
-      }
+        return true;
+      });
     }
 
     const matchingTriggers = this._matchingTriggers((tr, target) =>
@@ -237,22 +258,49 @@ export default class Project {
     );
   }
 
-  public get spritesAndClones(): Sprite[] {
-    return Object.values(this.sprites)
-      .flatMap((sprite) => sprite!.andClones())
-      .sort((a, b) => a._layerOrder - b._layerOrder);
+  public addSprite(sprite: Sprite, behind?: Sprite): void {
+    if (behind) {
+      const currentIndex = this.targets.indexOf(behind);
+      this.targets.splice(currentIndex, 0, sprite);
+    } else {
+      this.targets.push(sprite);
+    }
+  }
+
+  public removeSprite(sprite: Sprite): void {
+    const index = this.targets.indexOf(sprite);
+    if (index === -1) return;
+
+    this.targets.splice(index, 1);
+    this.cleanupSprite(sprite);
+  }
+
+  public filterSprites(predicate: (sprite: Sprite) => boolean): void {
+    let nextKeptSpriteIndex = 0;
+    for (let i = 0; i < this.targets.length; i++) {
+      const target = this.targets[i];
+      if (target instanceof Stage || predicate(target)) {
+        this.targets[nextKeptSpriteIndex] = target;
+        nextKeptSpriteIndex++;
+      } else {
+        this.cleanupSprite(target);
+      }
+    }
+    this.targets.length = nextKeptSpriteIndex;
   }
 
-  public get spritesAndStage(): (Sprite | Stage)[] {
-    return [...this.spritesAndClones, this.stage];
+  private cleanupSprite(sprite: Sprite): void {
+    this.runningTriggers = this.runningTriggers.filter(
+      ({ target }) => target !== sprite
+    );
   }
 
   public changeSpriteLayer(
     sprite: Sprite,
     layerDelta: number,
     relativeToSprite = sprite
   ): void {
-    const spritesArray = this.spritesAndClones;
+    const spritesArray = this.targets;
 
     const originalIndex = spritesArray.indexOf(sprite);
     const relativeToIndex = spritesArray.indexOf(relativeToSprite);
@@ -264,17 +312,16 @@ export default class Project {
     // Remove sprite from originalIndex and insert at newIndex
     spritesArray.splice(originalIndex, 1);
     spritesArray.splice(newIndex, 0, sprite);
+  }
 
-    // spritesArray is sorted correctly, but to influence
-    // the actual order of the sprites we need to update
-    // each one's _layerOrder property.
-    spritesArray.forEach((sprite, index) => {
-      sprite._layerOrder = index + 1;
-    });
+  public forEachTarget(callback: (target: Sprite | Stage) => void): void {
+    for (const target of this.targets) {
+      callback(target);
+    }
   }
 
   public stopAllSounds(): void {
-    for (const target of this.spritesAndStage) {
+    for (const target of this.targets) {
       target.stopAllOfMySounds();
     }
   }

src/Renderer.ts

@@ -303,37 +303,33 @@ export default class Renderer {
         (filter && !filter(layer))
       );
 
-    // Stage
-    if (shouldIncludeLayer(this.project.stage)) {
-      this.renderSprite(this.project.stage, options);
-    }
+    this.project.forEachTarget((target) => {
+      // TODO: just make a `visible` getter for Stage to avoid this rigmarole
+      const visible = "visible" in target ? target.visible : true;
 
-    // Pen layer
-    if (shouldIncludeLayer(this._penSkin)) {
-      const penMatrix = Matrix.create();
-      Matrix.scale(
-        penMatrix,
-        penMatrix,
-        this._penSkin.width,
-        -this._penSkin.height
-      );
-      Matrix.translate(penMatrix, penMatrix, -0.5, -0.5);
+      if (shouldIncludeLayer(target) && visible) {
+        this.renderSprite(target, options);
+      }
 
-      this._renderSkin(
-        this._penSkin,
-        options.drawMode,
-        penMatrix,
-        1 /* scale */
-      );
-    }
+      // Draw the pen layer in front of the stage
+      if (target instanceof Stage && shouldIncludeLayer(this._penSkin)) {
+        const penMatrix = Matrix.create();
+        Matrix.scale(
+          penMatrix,
+          penMatrix,
+          this._penSkin.width,
+          -this._penSkin.height
+        );
+        Matrix.translate(penMatrix, penMatrix, -0.5, -0.5);
 
-    // Sprites + clones
-    for (const sprite of this.project.spritesAndClones) {
-      // Stage doesn't have "visible" defined, so check if it's strictly false
-      if (shouldIncludeLayer(sprite) && sprite.visible !== false) {
-        this.renderSprite(sprite, options);
+        this._renderSkin(
+          this._penSkin,
+          options.drawMode,
+          penMatrix,
+          1 /* scale */
+        );
       }
-    }
+    });
   }
 
   private _updateStageSize(): void {

src/Sprite.ts

@@ -102,7 +102,7 @@ abstract class SpriteBase {
 
   protected _costumeNumber: number;
   // TODO: remove this and just store the sprites in layer order, as Scratch does.
-  public _layerOrder: number;
+  private _initialLayerOrder: number | null;
   public triggers: Trigger[];
   public watchers: Partial<Record<string, Watcher>>;
   protected costumes: Costume[];
@@ -118,7 +118,7 @@ abstract class SpriteBase {
     // TODO: pass project in here, ideally
     const { costumeNumber, layerOrder = 0 } = initialConditions;
     this._costumeNumber = costumeNumber;
-    this._layerOrder = layerOrder;
+    this._initialLayerOrder = layerOrder;
 
     this.triggers = [];
     this.watchers = {};
@@ -136,6 +136,20 @@ abstract class SpriteBase {
     this._vars = vars;
   }
 
+  public getInitialLayerOrder(): number {
+    const order = this._initialLayerOrder;
+    if (order === null) {
+      throw new Error(
+        "getInitialLayerOrder should only be called once, when the project is initialized"
+      );
+    }
+    return order;
+  }
+
+  public clearInitialLayerOrder(): void {
+    this._initialLayerOrder = null;
+  }
+
   protected getSoundsPlayedByMe(): Sound[] {
     return this.sounds.filter((sound) => this.effectChain.isTargetOf(sound));
   }
@@ -523,8 +537,7 @@ export class Sprite extends SpriteBase {
   public size: number;
   public visible: boolean;
 
-  private parent: this | null;
-  public clones: this[];
+  private original: this;
 
   private _penDown: boolean;
   public penSize: number;
@@ -555,8 +568,7 @@ export class Sprite extends SpriteBase {
     this.size = size;
     this.visible = visible;
 
-    this.parent = null;
-    this.clones = [];
+    this.original = this;
 
     this._penDown = penDown || false;
     this.penSize = penSize || 1;
@@ -569,6 +581,10 @@ export class Sprite extends SpriteBase {
     };
   }
 
+  public get isOriginal(): boolean {
+    return this.original === this;
+  }
+
   public *askAndWait(question: string): Yielding<void> {
     if (this._speechBubble) {
       this.say("");
@@ -599,21 +615,16 @@ export class Sprite extends SpriteBase {
 
     // Clones inherit audio effects from the original sprite, for some reason.
     // Couldn't explain it, but that's the behavior in Scratch 3.0.
-    // eslint-disable-next-line @typescript-eslint/no-this-alias
-    let original = this;
-    while (original.parent) {
-      original = original.parent;
-    }
-    clone.effectChain = original.effectChain.clone({
+    clone.effectChain = this.original.effectChain.clone({
       getNonPatchSoundList: clone.getSoundsPlayedByMe.bind(clone),
     });
 
     // Make a new audioEffects interface which acts on the cloned effect chain.
     clone.audioEffects = new AudioEffectMap(clone.effectChain);
 
-    clone.clones = [];
-    clone.parent = this;
-    this.clones.push(clone);
+    clone.original = this.original;
+
+    this._project.addSprite(clone, this);
 
     // Trigger CLONE_START:
     const triggers = clone.triggers.filter((tr) =>
@@ -625,17 +636,9 @@ export class Sprite extends SpriteBase {
   }
 
   public deleteThisClone(): void {
-    if (this.parent === null) return;
-
-    this.parent.clones = this.parent.clones.filter((clone) => clone !== this);
-
-    this._project.runningTriggers = this._project.runningTriggers.filter(
-      ({ target }) => target !== this
-    );
-  }
+    if (this.isOriginal) return;
 
-  public andClones(): this[] {
-    return [this, ...this.clones.flatMap((clone) => clone.andClones())];
+    this._project.removeSprite(this);
   }
 
   public get direction(): number {