Merge pull request #1361 from kchadha/sprite-upload-global-local-fix

Fix sprite upload variable conflict

src/engine/target.js

@@ -535,6 +535,7 @@ class Target extends EventEmitter {
      * then the local variable is renamed to distinguish itself from the pre-existing variable.
      * All blocks that reference the local variable will be updated to use the new name.
      */
+    // TODO (#1360) This function is too long, add some helpers for the different chunks and cases...
     fixUpVariableReferences () {
         if (!this.runtime) return; // There's no runtime context to conflict with
         if (this.isStage) return; // Stage can't have variable conflicts with itself (and also can't be uploaded)
@@ -562,6 +563,18 @@ class Target extends EventEmitter {
             }
         }
         const conflictIdsToReplace = Object.create(null);
+        const conflictNamesToReplace = Object.create(null);
+
+        // Cache the list of all variable names by type so that we don't need to
+        // re-calculate this in every iteration of the following loop.
+        const varNamesByType = {};
+        const allVarNames = type => {
+            const namesOfType = varNamesByType[type];
+            if (namesOfType) return namesOfType;
+            varNamesByType[type] = this.runtime.getAllVarNamesOfType(type);
+            return varNamesByType[type];
+        };
+
         for (const varId in allReferences) {
             // We don't care about which var ref we get, they should all have the same var info
             const varRef = allReferences[varId][0];
@@ -588,10 +601,25 @@ class Target extends EventEmitter {
                 }
             } else {
                 // We didn't find the referenced variable id anywhere,
-                // look it up by name and type to make sure there's no conflict
-                const existingVar = this.lookupVariableByNameAndType(varName, varType);
-                if (existingVar && !conflictIdsToReplace[varId]) {
-                    conflictIdsToReplace[varId] = existingVar.id;
+                // Treat it as a reference to a global variable (from the original
+                // project this sprite was exported from).
+                // Check for whether a global variable of the same name and type exists,
+                // and if so, track it to merge with the existing global in a second pass of the blocks.
+                const existingVar = stage.lookupVariableByNameAndType(varName, varType);
+                if (existingVar) {
+                    if (!conflictIdsToReplace[varId]) {
+                        conflictIdsToReplace[varId] = existingVar.id;
+                    }
+                } else {
+                    // A global variable with the same name did not already exist,
+                    // create a new one such that it does not conflict with any
+                    // names of local variables of the same type.
+                    const allNames = allVarNames(varType);
+                    const freshName = StringUtil.unusedName(varName, allNames);
+                    stage.createVariable(varId, freshName, varType);
+                    if (!conflictNamesToReplace[varId]) {
+                        conflictNamesToReplace[varId] = freshName;
+                    }
                 }
             }
         }
@@ -603,7 +631,7 @@ class Target extends EventEmitter {
             const type = this.variables[varId].type;
             renameConflictingLocalVar(varId, name, type);
         }
-        // Finally, handle global var conflicts (e.g. a sprite is uploaded, and has
+        // Handle global var conflicts with existing global vars (e.g. a sprite is uploaded, and has
         // blocks referencing some variable that the sprite does not own, and this
         // variable conflicts with a global var)
         // In this case, we want to merge the new variable referenes with the
@@ -613,6 +641,20 @@ class Target extends EventEmitter {
             const referencesToUpdate = allReferences[conflictId];
             this.mergeVariables(conflictId, existingId, referencesToUpdate);
         }
+
+        // Handle global var conflicts existing local vars (e.g a sprite is uploaded,
+        // and has blocks referencing some variable that the sprite does not own, and this
+        // variable conflcits with another sprite's local var).
+        // In this case, we want to go through the variable references and update
+        // the name of the variable in that reference.
+        for (const conflictId in conflictNamesToReplace) {
+            const newName = conflictNamesToReplace[conflictId];
+            const referencesToUpdate = allReferences[conflictId];
+            referencesToUpdate.map(ref => {
+                ref.referencingField.value = newName;
+                return ref;
+            });
+        }
     }
 
 }

test/unit/engine_target.js

@@ -387,6 +387,51 @@ test('fixUpVariableReferences fixes conflicting sprite local var without blocks
     t.end();
 });
 
+test('fixUpVariableReferences fixes sprite global var conflicting with other sprite\'s local var', t => {
+    const runtime = new Runtime();
+
+    const stage = new Target(runtime);
+    stage.isStage = true;
+
+    const target = new Target(runtime);
+    target.isStage = false;
+
+    const existingTarget = new Target(runtime);
+    existingTarget.isStage = false;
+
+    runtime.targets = [stage, target, existingTarget];
+
+    // Create a local variable on the pre-existing target
+    existingTarget.createVariable('pre-existing local var id', 'a mock variable', Variable.SCALAR_TYPE);
+
+    target.blocks.createBlock(adapter(events.mockVariableBlock)[0]);
+
+    t.equal(Object.keys(existingTarget.variables).length, 1);
+    const existingVariable = Object.values(existingTarget.variables)[0];
+    t.equal(existingVariable.name, 'a mock variable');
+    t.equal(Object.keys(target.variables).length, 0);
+    t.equal(Object.keys(stage.variables).length, 0);
+    t.type(target.blocks.getBlock('a block'), 'object');
+    t.type(target.blocks.getBlock('a block').fields, 'object');
+    t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object');
+    t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id');
+
+    target.fixUpVariableReferences();
+
+    t.equal(Object.keys(existingTarget.variables).length, 1);
+    t.equal(existingVariable.name, 'a mock variable');
+    t.equal(Object.keys(target.variables).length, 0);
+    t.equal(Object.keys(stage.variables).length, 1);
+    t.type(target.blocks.getBlock('a block'), 'object');
+    t.type(target.blocks.getBlock('a block').fields, 'object');
+    t.type(target.blocks.getBlock('a block').fields.VARIABLE, 'object');
+    t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id');
+    const newGlobal = stage.variables[Object.keys(stage.variables)[0]];
+    t.equal(newGlobal.name, 'a mock variable2');
+
+    t.end();
+});
+
 test('fixUpVariableReferences does not change variable name if there is no variable conflict', t => {
     const runtime = new Runtime();