Merge pull request #1912 from kchadha/executable-targets-fix

Populate executable targets when runtime.targets is populated

src/blocks/scratch3_control.js

@@ -165,7 +165,10 @@ class Scratch3ControlBlocks {
         // Create clone
         const newClone = cloneTarget.makeClone();
         if (newClone) {
-            this.runtime.targets.push(newClone);
+            this.runtime.addTarget(newClone);
+
+            // Place behind the original target.
+            newClone.goBehindOther(cloneTarget);
         }
     }
 

src/engine/runtime.js

@@ -1560,11 +1560,14 @@ class Runtime extends EventEmitter {
     }
 
     /**
-     * Add a target to the execution order.
-     * @param {Target} executableTarget target to add
+     * Add a target to the runtime. This tracks the sprite pane
+     * ordering of the target. The target still needs to be put
+     * into the correct execution order after calling this function.
+     * @param {Target} target target to add
      */
-    addExecutable (executableTarget) {
-        this.executableTargets.push(executableTarget);
+    addTarget (target) {
+        this.targets.push(target);
+        this.executableTargets.push(target);
     }
 
     /**

src/engine/target.js

@@ -69,10 +69,6 @@ class Target extends EventEmitter {
          * @type {Object.<string, *>}
          */
         this._edgeActivatedHatValues = {};
-
-        if (this.runtime) {
-            this.runtime.addExecutable(this);
-        }
     }
 
     /**

src/serialization/sb2.js

@@ -781,14 +781,17 @@ const reorderParsedTargets = function (targets) {
     // Reorder parsed targets based on the temporary targetPaneOrder property
     // and then delete it.
 
-    targets.sort((a, b) => a.targetPaneOrder - b.targetPaneOrder);
+    const reordered = targets.map((t, index) => {
+        t.layerOrder = index;
+        return t;
+    }).sort((a, b) => a.targetPaneOrder - b.targetPaneOrder);
 
     // Delete the temporary target pane ordering since we shouldn't need it anymore.
-    targets.forEach(t => {
+    reordered.forEach(t => {
         delete t.targetPaneOrder;
     });
 
-    return targets;
+    return reordered;
 };
 
 

src/serialization/sb3.js

@@ -1157,12 +1157,18 @@ const deserialize = function (json, runtime, zip, isSingleSprite) {
             parseScratchObject(target, runtime, extensions, zip))
     )
         .then(targets => targets // Re-sort targets back into original sprite-pane ordering
+            .map((t, i) => {
+                // Add layer order property to deserialized targets.
+                // This property is used to initialize executable targets in
+                // the correct order and is deleted in VM's installTargets function
+                t.layerOrder = i;
+                return t;
+            })
             .sort((a, b) => a.targetPaneOrder - b.targetPaneOrder)
             .map(t => {
                 // Delete the temporary properties used for
                 // sprite pane ordering and stage layer ordering
                 delete t.targetPaneOrder;
-                delete t.layerOrder;
                 return t;
             }))
         .then(targets => {

src/sprites/rendered-target.js

@@ -1021,8 +1021,6 @@ class RenderedTarget extends Target {
         newClone._edgeActivatedHatValues = Clone.simple(this._edgeActivatedHatValues);
         newClone.initDrawable(StageLayering.SPRITE_LAYER);
         newClone.updateAllDrawableProperties();
-        // Place behind the current target.
-        newClone.goBehindOther(this);
         return newClone;
     }
 
@@ -1046,7 +1044,6 @@ class RenderedTarget extends Target {
             newTarget.effects = JSON.parse(JSON.stringify(this.effects));
             newTarget.variables = this.duplicateVariables(newTarget.blocks);
             newTarget.updateAllDrawableProperties();
-            newTarget.goBehindOther(this);
             return newTarget;
         });
     }

src/virtual-machine.js

@@ -495,11 +495,18 @@ class VirtualMachine extends EventEmitter {
 
         return Promise.all(extensionPromises).then(() => {
             targets.forEach(target => {
-                this.runtime.targets.push(target);
+                this.runtime.addTarget(target);
                 (/** @type RenderedTarget */ target).updateAllDrawableProperties();
                 // Ensure unique sprite name
                 if (target.isSprite()) this.renameSprite(target.id, target.getName());
             });
+            // Sort the executable targets by layerOrder.
+            // Remove layerOrder property after use.
+            this.runtime.executableTargets.sort((a, b) => a.layerOrder - b.layerOrder);
+            targets.forEach(target => {
+                delete target.layerOrder;
+            });
+
             // Select the first target for editing, e.g., the first sprite.
             if (wholeProject && (targets.length > 1)) {
                 this.editingTarget = targets[1];
@@ -1016,7 +1023,8 @@ class VirtualMachine extends EventEmitter {
             throw new Error('No sprite associated with this target.');
         }
         return target.duplicate().then(newTarget => {
-            this.runtime.targets.push(newTarget);
+            this.runtime.addTarget(newTarget);
+            newTarget.goBehindOther(target);
             this.setEditingTarget(newTarget.id);
         });
     }

test/integration/hat-threads-run-every-frame.js

@@ -192,7 +192,8 @@ test('edge activated hat should trigger for both sprites when sprite is cloned',
                 val + Object.keys(target._edgeActivatedHatValues).length, 0);
             t.equal(numTargetEdgeHats, 1);
 
-            vm.runtime.targets.push(vm.runtime.targets[1].makeClone());
+            const cloneTarget = vm.runtime.targets[1].makeClone();
+            vm.runtime.addTarget(cloneTarget);
 
             vm.runtime._step();
             // Check that the runtime's _edgeActivatedHatValues object has two separate keys

test/unit/blocks_event.js

@@ -54,7 +54,8 @@ test('#760 - broadcastAndWait', t => {
     const tgt = new Target(rt, b);
     tgt.isStage = true;
     tgt.createVariable('testBroadcastID', 'message', Variable.BROADCAST_MESSAGE_TYPE);
-    rt.targets.push(tgt);
+
+    rt.addTarget(tgt);
 
     let th = rt._pushThread('broadcastAndWaitBlock', t);
     const util = new BlockUtility();

test/unit/blocks_looks.js

@@ -48,7 +48,7 @@ const testCostume = (costumes, arg, currentCostume = 1, isStage = false) => {
 
     if (isStage) {
         target.isStage = true;
-        rt.targets.push(target);
+        rt.addTarget(target);
         looks.switchBackdrop({BACKDROP: arg}, {target});
     } else {
         looks.switchCostume({COSTUME: arg}, {target});

test/unit/engine_target.js

@@ -188,7 +188,7 @@ test('renameVariable calls cloud io device\'s requestRenameVariable function', t
     target.isStage = true;
     const mockCloudVar = new Variable('foo', 'bar', Variable.SCALAR_TYPE, true);
     target.variables[mockCloudVar.id] = mockCloudVar;
-    runtime.targets.push(target);
+    runtime.addTarget(target);
 
     target.renameVariable('foo', 'bar2');
 
@@ -215,7 +215,7 @@ test('renameVariable does not call cloud io device\'s requestRenameVariable func
     const target = new Target(runtime);
     const mockCloudVar = new Variable('foo', 'bar', Variable.SCALAR_TYPE, true);
     target.variables[mockCloudVar.id] = mockCloudVar;
-    runtime.targets.push(target);
+    runtime.addTarget(target);
 
     target.renameVariable('foo', 'bar2');
 
@@ -266,7 +266,7 @@ test('deleteVariable calls cloud io device\'s requestRenameVariable function', t
     target.isStage = true;
     const mockCloudVar = new Variable('foo', 'bar', Variable.SCALAR_TYPE, true);
     target.variables[mockCloudVar.id] = mockCloudVar;
-    runtime.targets.push(target);
+    runtime.addTarget(target);
 
     target.deleteVariable('foo');
 
@@ -288,7 +288,7 @@ test('deleteVariable calls cloud io device\'s requestRenameVariable function', t
     const target = new Target(runtime);
     const mockCloudVar = new Variable('foo', 'bar', Variable.SCALAR_TYPE, true);
     target.variables[mockCloudVar.id] = mockCloudVar;
-    runtime.targets.push(target);
+    runtime.addTarget(target);
 
     target.deleteVariable('foo');
 

test/unit/serialization_sb3.js

@@ -1,6 +1,7 @@
 const test = require('tap').test;
 const path = require('path');
 const VirtualMachine = require('../../src/index');
+const Runtime = require('../../src/engine/runtime');
 const sb3 = require('../../src/serialization/sb3');
 const readFileToBuffer = require('../fixtures/readProjectFile').readFileToBuffer;
 const exampleProjectPath = path.resolve(__dirname, '../fixtures/clone-cleanup.sb2');
@@ -147,10 +148,10 @@ test('deserialize sb3 project with comments - no duplicate id serialization', t
         });
 });
 
-test('serialize sb3 preserves sprite layer order', t => {
+test('serializing and deserializing sb3 preserves sprite layer order', t => {
     const vm = new VirtualMachine();
     vm.attachRenderer(new FakeRenderer());
-    vm.loadProject(readFileToBuffer(path.resolve(__dirname, '../fixtures/ordering.sb2')))
+    return vm.loadProject(readFileToBuffer(path.resolve(__dirname, '../fixtures/ordering.sb2')))
         .then(() => {
             // Target get layer order needs a renderer,
             // fake the numbers we would get back from the
@@ -182,8 +183,28 @@ test('serialize sb3 preserves sprite layer order', t => {
             t.equal(result.targets[2].layerOrder, 1);
             t.equal(result.targets[3].layerOrder, 3);
 
-            t.end();
-        });
+            return result;
+        })
+        .then(serializedObject =>
+            sb3.deserialize(
+                JSON.parse(JSON.stringify(serializedObject)), new Runtime(), null, false)
+                .then(({targets}) => {
+                    // First check that the sprites are ordered correctly (as they would
+                    // appear in the target pane)
+                    t.equal(targets[0].sprite.name, 'Stage');
+                    t.equal(targets[1].sprite.name, 'First');
+                    t.equal(targets[2].sprite.name, 'Second');
+                    t.equal(targets[3].sprite.name, 'Third');
+
+                    // Check that they are in the correct layer order (as they would render
+                    // back to front on the stage)
+                    t.equal(targets[0].layerOrder, 0);
+                    t.equal(targets[1].layerOrder, 2);
+                    t.equal(targets[2].layerOrder, 1);
+                    t.equal(targets[3].layerOrder, 3);
+
+                    t.end();
+                }));
 });
 
 test('serializeBlocks', t => {