From 729fc3d3037ac7aafaa1cd4a7e69f32c03b3ad53 Mon Sep 17 00:00:00 2001 From: DD Date: Wed, 21 Feb 2018 19:59:35 -0500 Subject: [PATCH 1/3] Make sprite.costumes private so that I can enforce that when you add costumes, they get assigned a unique ID --- src/blocks/scratch3_looks.js | 6 +-- src/blocks/scratch3_sensing.js | 4 +- src/serialization/sb2.js | 2 +- src/serialization/sb3.js | 2 +- src/sprites/rendered-target.js | 39 ++++++++------- src/sprites/sprite.js | 71 ++++++++++++++++++++++++++-- src/virtual-machine.js | 16 +++++-- test/integration/sb3-roundtrip.js | 8 ++-- test/unit/sprites_rendered-target.js | 46 +++++++++--------- 9 files changed, 135 insertions(+), 59 deletions(-) diff --git a/src/blocks/scratch3_looks.js b/src/blocks/scratch3_looks.js index 8c17bb1b0..6b9059c09 100644 --- a/src/blocks/scratch3_looks.js +++ b/src/blocks/scratch3_looks.js @@ -332,7 +332,7 @@ class Scratch3LooksBlocks { } if (target === this.runtime.getTargetForStage()) { // Target is the stage - start hats. - const newName = target.sprite.costumes[target.currentCostume].name; + const newName = target.getCostumes()[target.currentCostume].name; return this.runtime.startHats('event_whenbackdropswitchesto', { BACKDROP: newName }); @@ -442,7 +442,7 @@ class Scratch3LooksBlocks { return stage.currentCostume + 1; } // Else return name - return stage.sprite.costumes[stage.currentCostume].name; + return stage.getCostumes()[stage.currentCostume].name; } getCostumeNumberName (args, util) { @@ -450,7 +450,7 @@ class Scratch3LooksBlocks { return util.target.currentCostume + 1; } // Else return name - return util.target.sprite.costumes[util.target.currentCostume].name; + return util.target.getCostumes()[util.target.currentCostume].name; } } diff --git a/src/blocks/scratch3_sensing.js b/src/blocks/scratch3_sensing.js index f7a39b6b9..602452669 100644 --- a/src/blocks/scratch3_sensing.js +++ b/src/blocks/scratch3_sensing.js @@ -237,7 +237,7 @@ class Scratch3SensingBlocks { case 'backdrop #': return attrTarget.currentCostume + 1; case 'backdrop name': - return attrTarget.sprite.costumes[attrTarget.currentCostume].name; + return attrTarget.getCostumes()[attrTarget.currentCostume].name; case 'volume': return; // @todo: Keep this in mind for sound blocks! } } else { @@ -247,7 +247,7 @@ class Scratch3SensingBlocks { case 'direction': return attrTarget.direction; case 'costume #': return attrTarget.currentCostume + 1; case 'costume name': - return attrTarget.sprite.costumes[attrTarget.currentCostume].name; + return attrTarget.getCostumes()[attrTarget.currentCostume].name; case 'size': return attrTarget.size; case 'volume': return; // @todo: above, keep in mind for sound blocks.. } diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index 1102c005e..0cb3e6ae3 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -324,7 +324,7 @@ const parseScratchObject = function (object, runtime, extensions, topLevel) { target.isStage = topLevel; Promise.all(costumePromises).then(costumes => { - sprite.costumes = costumes; + sprite.addCostumes(costumes); }); Promise.all(soundPromises).then(sounds => { diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index 9e8bcbf4e..a9ba94f77 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -156,7 +156,7 @@ const parseScratchObject = function (object, runtime, extensions) { target.isStage = object.isStage; } Promise.all(costumePromises).then(costumes => { - sprite.costumes = costumes; + sprite.addCostumes(costumes); }); Promise.all(soundPromises).then(sounds => { sprite.sounds = sounds; diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 2a6a0f7d3..5742e15dc 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -372,10 +372,10 @@ class RenderedTarget extends Target { // Keep the costume index within possible values. index = Math.round(index); this.currentCostume = MathUtil.wrapClamp( - index, 0, this.sprite.costumes.length - 1 + index, 0, this.getCostumes().length - 1 ); if (this.renderer) { - const costume = this.sprite.costumes[this.currentCostume]; + const costume = this.sprite.getCostumeByIndex(this.currentCostume); const drawableProperties = { skinId: costume.skinId, costumeResolution: costume.bitmapResolution @@ -403,9 +403,16 @@ class RenderedTarget extends Target { * @param {!object} costumeObject Object representing the costume. */ addCostume (costumeObject) { - const usedNames = this.sprite.costumes.map(costume => costume.name); - costumeObject.name = StringUtil.unusedName(costumeObject.name, usedNames); - this.sprite.costumes.push(costumeObject); + this.sprite.addCostumeAt(costumeObject, this.getCostumes().length); + } + + /** + * Add a costume at the given index, taking care to avoid duplicate names. + * @param {!object} costumeObject Object representing the costume. + * @param {!int} index Index at which to add costume + */ + addCostumeAt (costumeObject, index) { + this.sprite.addCostumeAt(costumeObject, index); } /** @@ -414,12 +421,12 @@ class RenderedTarget extends Target { * @param {string} newName - the desired new name of the costume (will be modified if already in use). */ renameCostume (costumeIndex, newName) { - const usedNames = this.sprite.costumes + const usedNames = this.getCostumes() .filter((costume, index) => costumeIndex !== index) .map(costume => costume.name); - const oldName = this.sprite.costumes[costumeIndex].name; + const oldName = this.sprite.getCostumeByIndex(costumeIndex).name; const newUnusedName = StringUtil.unusedName(newName, usedNames); - this.sprite.costumes[costumeIndex].name = newUnusedName; + this.sprite.getCostumeByIndex(costumeIndex).name = newUnusedName; if (this.isStage) { // Since this is a backdrop, go through all targets and @@ -440,12 +447,10 @@ class RenderedTarget extends Target { * @param {number} index Costume index to be deleted */ deleteCostume (index) { - const originalCostumeCount = this.sprite.costumes.length; + const originalCostumeCount = this.getCostumes().length; if (originalCostumeCount === 1) return; - this.sprite.costumes = this.sprite.costumes - .slice(0, index) - .concat(this.sprite.costumes.slice(index + 1)); + this.sprite.deleteCostumeByIndex(index); if (index === this.currentCostume && index === originalCostumeCount - 1) { this.setCostume(index - 1); @@ -525,8 +530,8 @@ class RenderedTarget extends Target { * @return {number} Index of the named costume, or -1 if not present. */ getCostumeIndexByName (costumeName) { - for (let i = 0; i < this.sprite.costumes.length; i++) { - if (this.sprite.costumes[i].name === costumeName) { + for (let i = 0; i < this.getCostumes().length; i++) { + if (this.sprite.getCostumeByIndex(i).name === costumeName) { return i; } } @@ -538,7 +543,7 @@ class RenderedTarget extends Target { * @return {object} current costume */ getCurrentCostume () { - return this.sprite.costumes[this.currentCostume]; + return this.sprite.getCostumeByIndex(this.currentCostume); } /** @@ -546,7 +551,7 @@ class RenderedTarget extends Target { * @return {object[]} list of costumes */ getCostumes () { - return this.sprite.costumes; + return this.sprite.getCostumes(); } /** @@ -564,7 +569,7 @@ class RenderedTarget extends Target { updateAllDrawableProperties () { if (this.renderer) { const renderedDirectionScale = this._getRenderedDirectionAndScale(); - const costume = this.sprite.costumes[this.currentCostume]; + const costume = this.sprite.getCostumeByIndex(this.currentCostume); const bitmapResolution = costume.bitmapResolution || 1; const props = { position: [this.x, this.y], diff --git a/src/sprites/sprite.js b/src/sprites/sprite.js index bf68c98d1..3c30a08da 100644 --- a/src/sprites/sprite.js +++ b/src/sprites/sprite.js @@ -32,11 +32,12 @@ class Sprite { * name: "Costume Name", * bitmapResolution: 2, * rotationCenterX: 0, - * rotationCenterY: 0 + * rotationCenterY: 0, + * costumeId: 0 * } * @type {Array.} */ - this.costumes = []; + this.costumes_ = []; /** * List of sounds for this sprite. */ @@ -46,6 +47,68 @@ class Sprite { * @type {Array.} */ this.clones = []; + /** + * A costume ID generator. Costumes within a sprite should each have a unique ID. + */ + this.nextCostumeId = 0; + } + + /** + * Get full costume list + * @return {object[]} list of costumes + */ + getCostumes () { + return this.costumes_; + } + + /** + * Get costume at index + * @param {int} index Index + * @return {object} costumes + */ + getCostumeByIndex (index) { + return this.costumes_[index]; + } + + /** + * Add a costume, taking care to avoid duplicate names. + * @param {!object} costumeObject Object representing the costume. + */ + addCostume (costumeObject) { + this.addCostumeAt(costumeObject, this.costumes_.length); + } + + /** + * Add an array of costumes, taking care to avoid duplicate names. + * @param {!Array} costumes Array of objects representing costumes. + */ + addCostumes (costumes) { + for (const costume of costumes) { + this.addCostumeAt(costume, this.costumes_.length); + } + } + + /** + * Add a costume at the given index, taking care to avoid duplicate names. + * @param {!object} costumeObject Object representing the costume. + * @param {!int} index Index at which to add costume + */ + addCostumeAt (costumeObject, index) { + const usedNames = this.costumes_.map(costume => costume.name); + costumeObject.name = StringUtil.unusedName(costumeObject.name, usedNames); + costumeObject.costumeId = this.nextCostumeId; + this.nextCostumeId++; + this.costumes_.splice(index, 0, costumeObject); + } + + /** + * Delete a costume by index. + * @param {number} index Costume index to be deleted + */ + deleteCostumeByIndex (index) { + this.costumes_ = this.costumes_ + .slice(0, index) + .concat(this.costumes_.slice(index + 1)); } /** @@ -89,12 +152,12 @@ class Sprite { const assetPromises = []; - newSprite.costumes = this.costumes.map(costume => { + newSprite.addCostumes(this.costumes_.map(costume => { const newCostume = Object.assign({}, costume); const costumeAsset = this.runtime.storage.get(costume.assetId); assetPromises.push(loadCostumeFromAsset(newCostume, costumeAsset, this.runtime)); return newCostume; - }); + })); newSprite.sounds = this.sounds.map(sound => { const newSound = Object.assign({}, sound); diff --git a/src/virtual-machine.js b/src/virtual-machine.js index d930ad534..9d4b21ea8 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -322,11 +322,19 @@ class VirtualMachine extends EventEmitter { return loadCostume(md5ext, costumeObject, this.runtime).then(() => { this.editingTarget.addCostume(costumeObject); this.editingTarget.setCostume( - this.editingTarget.sprite.costumes.length - 1 + this.editingTarget.getCostumes().length - 1 ); }); } + duplicateCostume (costumeIndex) { + const currentCostume = this.editingTarget.getCostumes()[costumeIndex]; + const clone = Object.assign({}, currentCostume); + this.editingTarget.addCostumeAt(clone, costumeIndex + 1); + this.editingTarget.setCostume(costumeIndex + 1); + this.emitTargetsUpdate(); + } + /** * Rename a costume on the current editing target. * @param {int} costumeIndex - the index of the costume to be renamed. @@ -407,7 +415,7 @@ class VirtualMachine extends EventEmitter { * @return {string} the costume's SVG string, or null if it's not an SVG costume. */ getCostumeSvg (costumeIndex) { - const id = this.editingTarget.sprite.costumes[costumeIndex].assetId; + const id = this.editingTarget.getCostumes()[costumeIndex].assetId; if (id && this.runtime && this.runtime.storage && this.runtime.storage.get(id).dataFormat === 'svg') { return this.runtime.storage.get(id).decodeText(); @@ -423,7 +431,7 @@ class VirtualMachine extends EventEmitter { * @param {number} rotationCenterY y of point about which the costume rotates, relative to its upper left corner */ updateSvg (costumeIndex, svg, rotationCenterX, rotationCenterY) { - const costume = this.editingTarget.sprite.costumes[costumeIndex]; + const costume = this.editingTarget.getCostumes()[costumeIndex]; if (costume && this.runtime && this.runtime.renderer) { costume.rotationCenterX = rotationCenterX; costume.rotationCenterY = rotationCenterY; @@ -452,7 +460,7 @@ class VirtualMachine extends EventEmitter { return loadCostume(md5ext, backdropObject, this.runtime).then(() => { const stage = this.runtime.getTargetForStage(); stage.addCostume(backdropObject); - stage.setCostume(stage.sprite.costumes.length - 1); + stage.setCostume(stage.getCostumes().length - 1); }); } diff --git a/test/integration/sb3-roundtrip.js b/test/integration/sb3-roundtrip.js index 5704e33b7..6233502fa 100644 --- a/test/integration/sb3-roundtrip.js +++ b/test/integration/sb3-roundtrip.js @@ -52,8 +52,8 @@ test('sb3-roundtrip', t => { t.strictEqual(sprite.clones.length, 1); t.strictEqual(sprite.clones[0], spriteClone); - t.strictEqual(sprite.costumes.length, 2); - const [cat, squirrel] = sprite.costumes; + t.strictEqual(sprite.getCostumes().length, 2); + const [cat, squirrel] = sprite.getCostumes(); t.strictEqual(cat.assetId, 'f88bf1935daea28f8ca098462a31dbb0'); t.strictEqual(cat.dataFormat, 'svg'); t.strictEqual(squirrel.assetId, '7e24c99c1b853e52f8e7f9004416fa34'); @@ -77,7 +77,7 @@ test('sb3-roundtrip', t => { const stageBlocks = new Blocks(); const stage = new Sprite(stageBlocks, runtime1); stage.name = 'Stage'; - stage.costumes = [building]; + stage.addCostume(building); stage.sounds = []; const stageClone = stage.createClone(); stageClone.isStage = true; @@ -85,7 +85,7 @@ test('sb3-roundtrip', t => { const spriteBlocks = new Blocks(); const sprite = new Sprite(spriteBlocks, runtime1); sprite.name = 'Sprite'; - sprite.costumes = [cat, squirrel]; + sprite.addCostumes([cat, squirrel]); sprite.sounds = [meow]; const spriteClone = sprite.createClone(); diff --git a/test/unit/sprites_rendered-target.js b/test/unit/sprites_rendered-target.js index 332a78111..262a834a5 100644 --- a/test/unit/sprites_rendered-target.js +++ b/test/unit/sprites_rendered-target.js @@ -90,7 +90,7 @@ test('setCostume', t => { const o = new Object(); const s = new Sprite(); const r = new Runtime(); - s.costumes = [o]; + s.addCostume(o); const a = new RenderedTarget(s, r); const renderer = new FakeRenderer(); a.renderer = renderer; @@ -107,7 +107,7 @@ test('deleteCostume', t => { const s = new Sprite(); const r = new Runtime(); - s.costumes = [o1, o2, o3]; + s.addCostumes([o1, o2, o3]); const a = new RenderedTarget(s, r); const renderer = new FakeRenderer(); a.renderer = renderer; @@ -117,32 +117,32 @@ test('deleteCostume', t => { // Costume 3 a.setCostume(0); a.deleteCostume(0); - t.deepEqual(a.sprite.costumes, [o2, o3]); + t.deepEqual(a.sprite.getCostumes(), [o2, o3]); t.equals(a.currentCostume, 0); // Costume 1 Costume 1 // x* Costume 2 => * Costume 3 // Costume 3 - a.sprite.costumes = [o1, o2, o3]; + a.sprite.addCostumes([o1, o2, o3]); a.setCostume(1); a.deleteCostume(1); - t.deepEqual(a.sprite.costumes, [o1, o3]); + t.deepEqual(a.sprite.getCostumes(), [o1, o3]); t.equals(a.currentCostume, 1); // Costume 1 Costume 1 // Costume 2 => * Costume 2 // x* Costume 3 - a.sprite.costumes = [o1, o2, o3]; + a.sprite.addCostumes([o1, o2, o3]); a.setCostume(2); a.deleteCostume(2); - t.deepEqual(a.sprite.costumes, [o1, o2]); + t.deepEqual(a.sprite.getCostumes(), [o1, o2]); t.equals(a.currentCostume, 1); // Refuses to delete only costume - a.sprite.costumes = [o1]; + a.sprite.addCostume(o1); a.setCostume(0); a.deleteCostume(0); - t.deepEqual(a.sprite.costumes, [o1]); + t.deepEqual(a.sprite.getCostumes(), [o1]); t.equals(a.currentCostume, 0); // Costume 1 Costume 1 @@ -150,10 +150,10 @@ test('deleteCostume', t => { // Costume 3 => * Costume 4 // * Costume 4 Costume 5 // Costume 5 - a.sprite.costumes = [o1, o2, o3, o4, o5]; + a.sprite.addCostumes([o1, o2, o3, o4, o5]); a.setCostume(3); a.deleteCostume(1); - t.deepEqual(a.sprite.costumes, [o1, o3, o4, o5]); + t.deepEqual(a.sprite.getCostumes(), [o1, o3, o4, o5]); t.equals(a.currentCostume, 2); // Costume 1 Costume 1 @@ -161,10 +161,10 @@ test('deleteCostume', t => { // Costume 3 => Costume 3 // x Costume 4 Costume 5 // Costume 5 - a.sprite.costumes = [o1, o2, o3, o4, o5]; + a.sprite.addCostumes([o1, o2, o3, o4, o5]); a.setCostume(1); a.deleteCostume(3); - t.deepEqual(a.sprite.costumes, [o1, o2, o3, o5]); + t.deepEqual(a.sprite.getCostumes(), [o1, o2, o3, o5]); t.equals(a.currentCostume, 1); // Costume 1 Costume 1 @@ -172,10 +172,10 @@ test('deleteCostume', t => { // Costume 3 => Costume 3 // Costume 4 Costume 4 // x Costume 5 - a.sprite.costumes = [o1, o2, o3, o4, o5]; + a.sprite.addCostumes([o1, o2, o3, o4, o5]); a.setCostume(1); a.deleteCostume(4); - t.deepEqual(a.sprite.costumes, [o1, o2, o3, o4]); + t.deepEqual(a.sprite.getCostumes(), [o1, o2, o3, o4]); t.equals(a.currentCostume, 1); t.end(); }); @@ -334,7 +334,7 @@ test('#getCostumes returns the costumes', t => { const spr = new Sprite(); const a = new RenderedTarget(spr, null); const costumes = [1, 2, 3]; - a.sprite.costumes = costumes; + a.sprite.getCostumes() = costumes; t.equals(a.getCostumes(), costumes); t.end(); }); @@ -354,9 +354,9 @@ test('#toJSON returns the sounds and costumes', t => { const sounds = [1, 2, 3]; const costumes = ['a', 'b', 'c']; a.sprite.sounds = sounds; - a.sprite.costumes = costumes; + a.sprite.getCostumes() = costumes; t.same(a.toJSON().sounds, sounds); - t.same(a.toJSON().costumes, costumes); + t.same(a.toJSON().getCostumes(), costumes); t.end(); }); @@ -372,9 +372,9 @@ test('#addSound does not duplicate names', t => { test('#addCostume does not duplicate names', t => { const spr = new Sprite(); const a = new RenderedTarget(spr, null); - a.sprite.costumes = [{name: 'first'}]; + a.sprite.addCostumes([{name: 'first'}]); a.addCostume({name: 'first'}); - t.deepEqual(a.sprite.costumes, [{name: 'first'}, {name: 'first2'}]); + t.deepEqual(a.sprite.getCostumes(), [{name: 'first'}, {name: 'first2'}]); t.end(); }); @@ -392,10 +392,10 @@ test('#renameSound does not duplicate names', t => { test('#renameCostume does not duplicate names', t => { const spr = new Sprite(); const a = new RenderedTarget(spr, null); - a.sprite.costumes = [{name: 'first'}, {name: 'second'}]; + a.sprite.addCostumes([{name: 'first'}, {name: 'second'}]); a.renameCostume(0, 'first'); // Shouldn't increment the name, noop - t.deepEqual(a.sprite.costumes, [{name: 'first'}, {name: 'second'}]); + t.deepEqual(a.sprite.getCostumes(), [{name: 'first'}, {name: 'second'}]); a.renameCostume(1, 'first'); - t.deepEqual(a.sprite.costumes, [{name: 'first'}, {name: 'first2'}]); + t.deepEqual(a.sprite.getCostumes(), [{name: 'first'}, {name: 'first2'}]); t.end(); }); From b4d6db0ad9c6c187d426309ad837a2524af94588 Mon Sep 17 00:00:00 2001 From: DD Date: Thu, 22 Feb 2018 15:42:35 -0500 Subject: [PATCH 2/3] Fix tests --- src/serialization/sb2.js | 2 +- src/serialization/sb3.js | 2 +- src/sprites/rendered-target.js | 26 +++++----- src/sprites/sprite.js | 47 +++++++---------- test/integration/sb3-roundtrip.js | 8 +-- test/unit/blocks_looks.js | 3 ++ test/unit/sprites_rendered-target.js | 78 ++++++++++++++++++---------- 7 files changed, 92 insertions(+), 74 deletions(-) diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index 0cb3e6ae3..1102c005e 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -324,7 +324,7 @@ const parseScratchObject = function (object, runtime, extensions, topLevel) { target.isStage = topLevel; Promise.all(costumePromises).then(costumes => { - sprite.addCostumes(costumes); + sprite.costumes = costumes; }); Promise.all(soundPromises).then(sounds => { diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index a9ba94f77..9e8bcbf4e 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -156,7 +156,7 @@ const parseScratchObject = function (object, runtime, extensions) { target.isStage = object.isStage; } Promise.all(costumePromises).then(costumes => { - sprite.addCostumes(costumes); + sprite.costumes = costumes; }); Promise.all(soundPromises).then(sounds => { sprite.sounds = sounds; diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 5742e15dc..c2802117d 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -372,10 +372,10 @@ class RenderedTarget extends Target { // Keep the costume index within possible values. index = Math.round(index); this.currentCostume = MathUtil.wrapClamp( - index, 0, this.getCostumes().length - 1 + index, 0, this.sprite.costumes.length - 1 ); if (this.renderer) { - const costume = this.sprite.getCostumeByIndex(this.currentCostume); + const costume = this.getCostumes()[this.currentCostume]; const drawableProperties = { skinId: costume.skinId, costumeResolution: costume.bitmapResolution @@ -403,7 +403,7 @@ class RenderedTarget extends Target { * @param {!object} costumeObject Object representing the costume. */ addCostume (costumeObject) { - this.sprite.addCostumeAt(costumeObject, this.getCostumes().length); + this.sprite.addCostumeAt(costumeObject, this.sprite.costumes.length); } /** @@ -421,12 +421,12 @@ class RenderedTarget extends Target { * @param {string} newName - the desired new name of the costume (will be modified if already in use). */ renameCostume (costumeIndex, newName) { - const usedNames = this.getCostumes() + const usedNames = this.sprite.costumes .filter((costume, index) => costumeIndex !== index) .map(costume => costume.name); - const oldName = this.sprite.getCostumeByIndex(costumeIndex).name; + const oldName = this.getCostumes()[costumeIndex].name; const newUnusedName = StringUtil.unusedName(newName, usedNames); - this.sprite.getCostumeByIndex(costumeIndex).name = newUnusedName; + this.getCostumes()[costumeIndex].name = newUnusedName; if (this.isStage) { // Since this is a backdrop, go through all targets and @@ -447,10 +447,10 @@ class RenderedTarget extends Target { * @param {number} index Costume index to be deleted */ deleteCostume (index) { - const originalCostumeCount = this.getCostumes().length; + const originalCostumeCount = this.sprite.costumes.length; if (originalCostumeCount === 1) return; - this.sprite.deleteCostumeByIndex(index); + this.sprite.deleteCostumeAt(index); if (index === this.currentCostume && index === originalCostumeCount - 1) { this.setCostume(index - 1); @@ -530,8 +530,8 @@ class RenderedTarget extends Target { * @return {number} Index of the named costume, or -1 if not present. */ getCostumeIndexByName (costumeName) { - for (let i = 0; i < this.getCostumes().length; i++) { - if (this.sprite.getCostumeByIndex(i).name === costumeName) { + for (let i = 0; i < this.sprite.costumes.length; i++) { + if (this.getCostumes()[i].name === costumeName) { return i; } } @@ -543,7 +543,7 @@ class RenderedTarget extends Target { * @return {object} current costume */ getCurrentCostume () { - return this.sprite.getCostumeByIndex(this.currentCostume); + return this.getCostumes()[this.currentCostume]; } /** @@ -551,7 +551,7 @@ class RenderedTarget extends Target { * @return {object[]} list of costumes */ getCostumes () { - return this.sprite.getCostumes(); + return this.sprite.costumes; } /** @@ -569,7 +569,7 @@ class RenderedTarget extends Target { updateAllDrawableProperties () { if (this.renderer) { const renderedDirectionScale = this._getRenderedDirectionAndScale(); - const costume = this.sprite.getCostumeByIndex(this.currentCostume); + const costume = this.getCostumes()[this.currentCostume]; const bitmapResolution = costume.bitmapResolution || 1; const props = { position: [this.x, this.y], diff --git a/src/sprites/sprite.js b/src/sprites/sprite.js index 3c30a08da..d3e53b8db 100644 --- a/src/sprites/sprite.js +++ b/src/sprites/sprite.js @@ -52,48 +52,37 @@ class Sprite { */ this.nextCostumeId = 0; } - - /** - * Get full costume list - * @return {object[]} list of costumes - */ - getCostumes () { - return this.costumes_; - } - - /** - * Get costume at index - * @param {int} index Index - * @return {object} costumes - */ - getCostumeByIndex (index) { - return this.costumes_[index]; - } - /** - * Add a costume, taking care to avoid duplicate names. - * @param {!object} costumeObject Object representing the costume. - */ - addCostume (costumeObject) { - this.addCostumeAt(costumeObject, this.costumes_.length); - } - /** * Add an array of costumes, taking care to avoid duplicate names. * @param {!Array} costumes Array of objects representing costumes. */ - addCostumes (costumes) { + set costumes (costumes) { + this.costumes_ = []; for (const costume of costumes) { this.addCostumeAt(costume, this.costumes_.length); } } + /** + * Get full costume list + * @return {object[]} list of costumes. Note that mutating the returned list will not + * mutate the list on the sprite. The sprite list should be mutated by calling + * addCostumeAt, deleteCostumeAt, or setting costumes. + */ + get costumes () { + return this.costumes_.slice(0); + } + /** * Add a costume at the given index, taking care to avoid duplicate names. * @param {!object} costumeObject Object representing the costume. * @param {!int} index Index at which to add costume */ addCostumeAt (costumeObject, index) { + if (!costumeObject.name) { + costumeObject.name = ''; + } const usedNames = this.costumes_.map(costume => costume.name); costumeObject.name = StringUtil.unusedName(costumeObject.name, usedNames); costumeObject.costumeId = this.nextCostumeId; @@ -105,7 +94,7 @@ class Sprite { * Delete a costume by index. * @param {number} index Costume index to be deleted */ - deleteCostumeByIndex (index) { + deleteCostumeAt (index) { this.costumes_ = this.costumes_ .slice(0, index) .concat(this.costumes_.slice(index + 1)); @@ -152,12 +141,12 @@ class Sprite { const assetPromises = []; - newSprite.addCostumes(this.costumes_.map(costume => { + newSprite.costumes = this.costumes_.map(costume => { const newCostume = Object.assign({}, costume); const costumeAsset = this.runtime.storage.get(costume.assetId); assetPromises.push(loadCostumeFromAsset(newCostume, costumeAsset, this.runtime)); return newCostume; - })); + }); newSprite.sounds = this.sounds.map(sound => { const newSound = Object.assign({}, sound); diff --git a/test/integration/sb3-roundtrip.js b/test/integration/sb3-roundtrip.js index 6233502fa..5704e33b7 100644 --- a/test/integration/sb3-roundtrip.js +++ b/test/integration/sb3-roundtrip.js @@ -52,8 +52,8 @@ test('sb3-roundtrip', t => { t.strictEqual(sprite.clones.length, 1); t.strictEqual(sprite.clones[0], spriteClone); - t.strictEqual(sprite.getCostumes().length, 2); - const [cat, squirrel] = sprite.getCostumes(); + t.strictEqual(sprite.costumes.length, 2); + const [cat, squirrel] = sprite.costumes; t.strictEqual(cat.assetId, 'f88bf1935daea28f8ca098462a31dbb0'); t.strictEqual(cat.dataFormat, 'svg'); t.strictEqual(squirrel.assetId, '7e24c99c1b853e52f8e7f9004416fa34'); @@ -77,7 +77,7 @@ test('sb3-roundtrip', t => { const stageBlocks = new Blocks(); const stage = new Sprite(stageBlocks, runtime1); stage.name = 'Stage'; - stage.addCostume(building); + stage.costumes = [building]; stage.sounds = []; const stageClone = stage.createClone(); stageClone.isStage = true; @@ -85,7 +85,7 @@ test('sb3-roundtrip', t => { const spriteBlocks = new Blocks(); const sprite = new Sprite(spriteBlocks, runtime1); sprite.name = 'Sprite'; - sprite.addCostumes([cat, squirrel]); + sprite.costumes = [cat, squirrel]; sprite.sounds = [meow]; const spriteClone = sprite.createClone(); diff --git a/test/unit/blocks_looks.js b/test/unit/blocks_looks.js index 6c1877c8a..c7b4c3d7b 100644 --- a/test/unit/blocks_looks.js +++ b/test/unit/blocks_looks.js @@ -4,6 +4,9 @@ const Runtime = require('../../src/engine/runtime'); const util = { target: { currentCostume: 0, // Internally, current costume is 0 indexed + getCostumes: function () { + return this.sprite.costumes; + }, sprite: { costumes: [ {name: 'first name'}, diff --git a/test/unit/sprites_rendered-target.js b/test/unit/sprites_rendered-target.js index 262a834a5..51c670dc9 100644 --- a/test/unit/sprites_rendered-target.js +++ b/test/unit/sprites_rendered-target.js @@ -90,7 +90,7 @@ test('setCostume', t => { const o = new Object(); const s = new Sprite(); const r = new Runtime(); - s.addCostume(o); + s.costumes = [o]; const a = new RenderedTarget(s, r); const renderer = new FakeRenderer(); a.renderer = renderer; @@ -107,7 +107,7 @@ test('deleteCostume', t => { const s = new Sprite(); const r = new Runtime(); - s.addCostumes([o1, o2, o3]); + s.costumes = [o1, o2, o3]; const a = new RenderedTarget(s, r); const renderer = new FakeRenderer(); a.renderer = renderer; @@ -117,32 +117,39 @@ test('deleteCostume', t => { // Costume 3 a.setCostume(0); a.deleteCostume(0); - t.deepEqual(a.sprite.getCostumes(), [o2, o3]); + t.equals(a.sprite.costumes.length, 2); + t.equals(a.sprite.costumes[0].id, 2); + t.equals(a.sprite.costumes[1].id, 3); t.equals(a.currentCostume, 0); // Costume 1 Costume 1 // x* Costume 2 => * Costume 3 // Costume 3 - a.sprite.addCostumes([o1, o2, o3]); + a.sprite.costumes = [o1, o2, o3]; a.setCostume(1); a.deleteCostume(1); - t.deepEqual(a.sprite.getCostumes(), [o1, o3]); + t.equals(a.sprite.costumes.length, 2); + t.equals(a.sprite.costumes[0].id, 1); + t.equals(a.sprite.costumes[1].id, 3); t.equals(a.currentCostume, 1); // Costume 1 Costume 1 // Costume 2 => * Costume 2 // x* Costume 3 - a.sprite.addCostumes([o1, o2, o3]); + a.sprite.costumes = [o1, o2, o3]; a.setCostume(2); a.deleteCostume(2); - t.deepEqual(a.sprite.getCostumes(), [o1, o2]); + t.equals(a.sprite.costumes.length, 2); + t.equals(a.sprite.costumes[0].id, 1); + t.equals(a.sprite.costumes[1].id, 2); t.equals(a.currentCostume, 1); // Refuses to delete only costume - a.sprite.addCostume(o1); + a.sprite.costumes = [o1]; a.setCostume(0); a.deleteCostume(0); - t.deepEqual(a.sprite.getCostumes(), [o1]); + t.equals(a.sprite.costumes.length, 1); + t.equals(a.sprite.costumes[0].id, 1); t.equals(a.currentCostume, 0); // Costume 1 Costume 1 @@ -150,10 +157,14 @@ test('deleteCostume', t => { // Costume 3 => * Costume 4 // * Costume 4 Costume 5 // Costume 5 - a.sprite.addCostumes([o1, o2, o3, o4, o5]); + a.sprite.costumes = [o1, o2, o3, o4, o5]; a.setCostume(3); a.deleteCostume(1); - t.deepEqual(a.sprite.getCostumes(), [o1, o3, o4, o5]); + t.equals(a.sprite.costumes.length, 4); + t.equals(a.sprite.costumes[0].id, 1); + t.equals(a.sprite.costumes[1].id, 3); + t.equals(a.sprite.costumes[2].id, 4); + t.equals(a.sprite.costumes[3].id, 5); t.equals(a.currentCostume, 2); // Costume 1 Costume 1 @@ -161,10 +172,14 @@ test('deleteCostume', t => { // Costume 3 => Costume 3 // x Costume 4 Costume 5 // Costume 5 - a.sprite.addCostumes([o1, o2, o3, o4, o5]); + a.sprite.costumes = [o1, o2, o3, o4, o5]; a.setCostume(1); a.deleteCostume(3); - t.deepEqual(a.sprite.getCostumes(), [o1, o2, o3, o5]); + t.equals(a.sprite.costumes.length, 4); + t.equals(a.sprite.costumes[0].id, 1); + t.equals(a.sprite.costumes[1].id, 2); + t.equals(a.sprite.costumes[2].id, 3); + t.equals(a.sprite.costumes[3].id, 5); t.equals(a.currentCostume, 1); // Costume 1 Costume 1 @@ -172,10 +187,14 @@ test('deleteCostume', t => { // Costume 3 => Costume 3 // Costume 4 Costume 4 // x Costume 5 - a.sprite.addCostumes([o1, o2, o3, o4, o5]); + a.sprite.costumes = [o1, o2, o3, o4, o5]; a.setCostume(1); a.deleteCostume(4); - t.deepEqual(a.sprite.getCostumes(), [o1, o2, o3, o4]); + t.equals(a.sprite.costumes.length, 4); + t.equals(a.sprite.costumes[0].id, 1); + t.equals(a.sprite.costumes[1].id, 2); + t.equals(a.sprite.costumes[2].id, 3); + t.equals(a.sprite.costumes[3].id, 4); t.equals(a.currentCostume, 1); t.end(); }); @@ -333,9 +352,11 @@ test('#stopAll clears graphics effects', t => { test('#getCostumes returns the costumes', t => { const spr = new Sprite(); const a = new RenderedTarget(spr, null); - const costumes = [1, 2, 3]; - a.sprite.getCostumes() = costumes; - t.equals(a.getCostumes(), costumes); + a.sprite.costumes = [{id: 1}, {id: 2}, {id: 3}]; + t.equals(a.getCostumes().length, 3); + t.equals(a.getCostumes()[0].id, 1); + t.equals(a.getCostumes()[1].id, 2); + t.equals(a.getCostumes()[2].id, 3); t.end(); }); @@ -352,11 +373,10 @@ test('#toJSON returns the sounds and costumes', t => { const spr = new Sprite(); const a = new RenderedTarget(spr, null); const sounds = [1, 2, 3]; - const costumes = ['a', 'b', 'c']; a.sprite.sounds = sounds; - a.sprite.getCostumes() = costumes; + a.sprite.costumes = [{id: 1}, {id: 2}, {id: 3}]; t.same(a.toJSON().sounds, sounds); - t.same(a.toJSON().getCostumes(), costumes); + t.same(a.toJSON().costumes, a.sprite.costumes); t.end(); }); @@ -372,9 +392,11 @@ test('#addSound does not duplicate names', t => { test('#addCostume does not duplicate names', t => { const spr = new Sprite(); const a = new RenderedTarget(spr, null); - a.sprite.addCostumes([{name: 'first'}]); a.addCostume({name: 'first'}); - t.deepEqual(a.sprite.getCostumes(), [{name: 'first'}, {name: 'first2'}]); + a.addCostume({name: 'first'}); + t.equal(a.sprite.costumes.length, 2); + t.equal(a.sprite.costumes[0].name, 'first'); + t.equal(a.sprite.costumes[1].name, 'first2'); t.end(); }); @@ -392,10 +414,14 @@ test('#renameSound does not duplicate names', t => { test('#renameCostume does not duplicate names', t => { const spr = new Sprite(); const a = new RenderedTarget(spr, null); - a.sprite.addCostumes([{name: 'first'}, {name: 'second'}]); + a.sprite.costumes = [{name: 'first'}, {name: 'second'}]; a.renameCostume(0, 'first'); // Shouldn't increment the name, noop - t.deepEqual(a.sprite.getCostumes(), [{name: 'first'}, {name: 'second'}]); + t.equal(a.sprite.costumes.length, 2); + t.equal(a.sprite.costumes[0].name, 'first'); + t.equal(a.sprite.costumes[1].name, 'second'); a.renameCostume(1, 'first'); - t.deepEqual(a.sprite.getCostumes(), [{name: 'first'}, {name: 'first2'}]); + t.equal(a.sprite.costumes.length, 2); + t.equal(a.sprite.costumes[0].name, 'first'); + t.equal(a.sprite.costumes[1].name, 'first2'); t.end(); }); From d6bcfae526edd339fe056318a813d1c29b2984cc Mon Sep 17 00:00:00 2001 From: DD Date: Fri, 23 Feb 2018 10:21:29 -0500 Subject: [PATCH 3/3] Remove costume ID --- src/sprites/sprite.js | 9 +-------- src/virtual-machine.js | 8 -------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/src/sprites/sprite.js b/src/sprites/sprite.js index d3e53b8db..4e9bd2894 100644 --- a/src/sprites/sprite.js +++ b/src/sprites/sprite.js @@ -32,8 +32,7 @@ class Sprite { * name: "Costume Name", * bitmapResolution: 2, * rotationCenterX: 0, - * rotationCenterY: 0, - * costumeId: 0 + * rotationCenterY: 0 * } * @type {Array.} */ @@ -47,10 +46,6 @@ class Sprite { * @type {Array.} */ this.clones = []; - /** - * A costume ID generator. Costumes within a sprite should each have a unique ID. - */ - this.nextCostumeId = 0; } /** @@ -85,8 +80,6 @@ class Sprite { } const usedNames = this.costumes_.map(costume => costume.name); costumeObject.name = StringUtil.unusedName(costumeObject.name, usedNames); - costumeObject.costumeId = this.nextCostumeId; - this.nextCostumeId++; this.costumes_.splice(index, 0, costumeObject); } diff --git a/src/virtual-machine.js b/src/virtual-machine.js index 9d4b21ea8..af190b9de 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -327,14 +327,6 @@ class VirtualMachine extends EventEmitter { }); } - duplicateCostume (costumeIndex) { - const currentCostume = this.editingTarget.getCostumes()[costumeIndex]; - const clone = Object.assign({}, currentCostume); - this.editingTarget.addCostumeAt(clone, costumeIndex + 1); - this.editingTarget.setCostume(costumeIndex + 1); - this.emitTargetsUpdate(); - } - /** * Rename a costume on the current editing target. * @param {int} costumeIndex - the index of the costume to be renamed.