From 1e3907e92213cb55599807176830a1f34af0bb7d Mon Sep 17 00:00:00 2001 From: chrisgarrity Date: Mon, 18 Jun 2018 11:37:10 -0400 Subject: [PATCH 01/22] Localize category labels Added formatMessage to the name attribute for: * music * pen * translate * video motion Skipped speak extension - trademarked name Skipped extensions that are not yet localized at all. --- src/extensions/scratch3_music/index.js | 6 +++++- src/extensions/scratch3_pen/index.js | 6 +++++- src/extensions/scratch3_translate/index.js | 6 +++++- src/extensions/scratch3_video_sensing/index.js | 6 +++++- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/extensions/scratch3_music/index.js b/src/extensions/scratch3_music/index.js index 4450a69e8..852c9261a 100644 --- a/src/extensions/scratch3_music/index.js +++ b/src/extensions/scratch3_music/index.js @@ -623,7 +623,11 @@ class Scratch3MusicBlocks { getInfo () { return { id: 'music', - name: 'Music', + name: formatMessage({ + id: 'music.categoryName', + default: 'Music', + description: 'Label for the Music extension category' + }), menuIconURI: menuIconURI, blockIconURI: blockIconURI, blocks: [ diff --git a/src/extensions/scratch3_pen/index.js b/src/extensions/scratch3_pen/index.js index e4211b328..4c46ce370 100644 --- a/src/extensions/scratch3_pen/index.js +++ b/src/extensions/scratch3_pen/index.js @@ -282,7 +282,11 @@ class Scratch3PenBlocks { getInfo () { return { id: 'pen', - name: 'Pen', + name: formatMessage({ + id: 'pen.categoryName', + default: 'Pen', + description: 'Label for the pen extension category' + }), blockIconURI: blockIconURI, blocks: [ { diff --git a/src/extensions/scratch3_translate/index.js b/src/extensions/scratch3_translate/index.js index 1d0c2ec05..787b8545f 100644 --- a/src/extensions/scratch3_translate/index.js +++ b/src/extensions/scratch3_translate/index.js @@ -86,7 +86,11 @@ class Scratch3TranslateBlocks { getInfo () { return { id: 'translate', - name: 'Translate', + name: formatMessage({ + id: 'translate.categoryName', + default: 'Translate', + description: 'Label for the translate extension category' + }), menuIconURI: '', // TODO: Add the final icons. blockIconURI: '', blocks: [ diff --git a/src/extensions/scratch3_video_sensing/index.js b/src/extensions/scratch3_video_sensing/index.js index c6e29f85b..afe8f44a1 100644 --- a/src/extensions/scratch3_video_sensing/index.js +++ b/src/extensions/scratch3_video_sensing/index.js @@ -372,7 +372,11 @@ class Scratch3VideoSensingBlocks { // Return extension definition return { id: 'videoSensing', - name: 'Video Motion', + name: formatMessage({ + id: 'videoSensing.categoryName', + default: 'Video Motion', + description: 'Label for the video motion extension category' + }), blocks: [ { // @todo this hat needs to be set itself to restart existing From 0527fcde640eee2e50910526e0e7361a0d144916 Mon Sep 17 00:00:00 2001 From: Florrie Date: Mon, 18 Jun 2018 17:20:59 -0300 Subject: [PATCH 02/22] Fix new sprites only showing up in the top-right quadrant --- src/sprites/rendered-target.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 37c1b5615..0cbbceddf 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -1004,8 +1004,8 @@ class RenderedTarget extends Target { const newTarget = newSprite.createClone(); // Copy all properties. // @todo refactor with clone methods - newTarget.x = Math.random() * 400 / 2; - newTarget.y = Math.random() * 300 / 2; + newTarget.x = (Math.random() - 0.5) * 400 / 2; + newTarget.y = (Math.random() - 0.5) * 300 / 2; newTarget.direction = this.direction; newTarget.draggable = this.draggable; newTarget.visible = this.visible; From 86a8d93eb6323a02b86e41d6340e55a37ecf9a96 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Tue, 19 Jun 2018 08:51:16 -0400 Subject: [PATCH 03/22] Sprite3 Export/Import. --- src/serialization/sb3.js | 18 ++++++++++---- src/serialization/serialize-assets.js | 20 +++++++++------- src/virtual-machine.js | 34 ++++++++++++++++++++------- 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index 4f129c171..1c02c2aec 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -446,15 +446,25 @@ const serializeTarget = function (target) { /** * Serializes the specified VM runtime. - * @param {!Runtime} runtime VM runtime instance to be serialized. + * @param {!Runtime} runtime VM runtime instance to be serialized. + * @param {string=} targetId Optional target id if serializing only a single target * @return {object} Serialized runtime instance. */ -const serialize = function (runtime) { +const serialize = function (runtime, targetId) { // Fetch targets const obj = Object.create(null); - const flattenedOriginalTargets = JSON.parse(JSON.stringify( + const flattenedOriginalTargets = JSON.parse(JSON.stringify(targetId ? + [runtime.getTargetById(targetId)] : runtime.targets.filter(target => target.isOriginal))); - obj.targets = flattenedOriginalTargets.map(t => serializeTarget(t, runtime)); + + const serializedTargets = flattenedOriginalTargets.map(t => serializeTarget(t, runtime)); + + if (targetId) { + return serializedTargets[0]; + } + + obj.targets = serializedTargets; + // TODO Serialize monitors diff --git a/src/serialization/serialize-assets.js b/src/serialization/serialize-assets.js index 9dc7bf0d9..ac444f11a 100644 --- a/src/serialization/serialize-assets.js +++ b/src/serialization/serialize-assets.js @@ -5,10 +5,11 @@ * to be written and the contents of the file, the serialized asset. * @param {Runtime} runtime The runtime with the assets to be serialized * @param {string} assetType The type of assets to be serialized: 'sounds' | 'costumes' + * @param {string=} optTargetId Optional target id to serialize assets for * @returns {Array} An array of file descriptors for each asset */ -const serializeAssets = function (runtime, assetType) { - const targets = runtime.targets; +const serializeAssets = function (runtime, assetType, optTargetId) { + const targets = optTargetId ? [runtime.getTargetById(optTargetId)] : runtime.targets; const assetDescs = []; for (let i = 0; i < targets.length; i++) { const currTarget = targets[i]; @@ -27,14 +28,16 @@ const serializeAssets = function (runtime, assetType) { }; /** - * Serialize all the sounds in the provided runtime into an array of file - * descriptors. A file descriptor is an object containing the name of the file + * Serialize all the sounds in the provided runtime or, if a target id is provided, + * in the specified target into an array of file descriptors. + * A file descriptor is an object containing the name of the file * to be written and the contents of the file, the serialized sound. * @param {Runtime} runtime The runtime with the sounds to be serialized + * @param {string=} optTargetId Optional targetid for serializing sounds of a single target * @returns {Array} An array of file descriptors for each sound */ -const serializeSounds = function (runtime) { - return serializeAssets(runtime, 'sounds'); +const serializeSounds = function (runtime, optTargetId) { + return serializeAssets(runtime, 'sounds', optTargetId); }; /** @@ -42,10 +45,11 @@ const serializeSounds = function (runtime) { * descriptors. A file descriptor is an object containing the name of the file * to be written and the contents of the file, the serialized costume. * @param {Runtime} runtime The runtime with the costumes to be serialized + * @param {string} optTargetId Optional targetid for serializing costumes of a single target * @returns {Array} An array of file descriptors for each costume */ -const serializeCostumes = function (runtime) { - return serializeAssets(runtime, 'costumes'); +const serializeCostumes = function (runtime, optTargetId) { + return serializeAssets(runtime, 'costumes', optTargetId); }; module.exports = { diff --git a/src/virtual-machine.js b/src/virtual-machine.js index 4ff368faa..378f9021a 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -263,14 +263,7 @@ class VirtualMachine extends EventEmitter { // Put everything in a zip file zip.file('project.json', projectJson); - for (let i = 0; i < soundDescs.length; i++) { - const currSound = soundDescs[i]; - zip.file(currSound.fileName, currSound.fileContent); - } - for (let i = 0; i < costumeDescs.length; i++) { - const currCostume = costumeDescs[i]; - zip.file(currCostume.fileName, currCostume.fileContent); - } + this._addFileDescsToZip(soundDescs.concat(costumeDescs), zip); return zip.generateAsync({ type: 'blob', @@ -281,6 +274,31 @@ class VirtualMachine extends EventEmitter { }); } + _addFileDescsToZip (fileDescs, zip) { + for (let i = 0; i < fileDescs.length; i++) { + const currFileDesc = fileDescs[i]; + zip.file(currFileDesc.fileName, currFileDesc.fileContent); + } + } + + exportSprite (targetId) { + const soundDescs = serializeSounds(this.runtime, targetId); + const costumeDescs = serializeCostumes(this.runtime, targetId); + const spriteJson = JSON.stringify(sb3.serialize(this.runtime, targetId)); + + const zip = new JSZip(); + zip.file('sprite.json', spriteJson); + this._addFileDescsToZip(soundDescs.concat(costumeDescs), zip); + + return zip.generateAsync({ + type: 'blob', + compression: 'DEFLATE', + compressionOptions: { + level: 6 + } + }); + } + /** * Export project as a Scratch 3.0 JSON representation. * @return {string} Serialized state of the runtime. From 5cc48b6ef368bcc9f118cd64767e716810c6852a Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Tue, 19 Jun 2018 08:52:22 -0400 Subject: [PATCH 04/22] Provide global vars to sb2 import when deserializing a single sprite. --- src/serialization/sb2.js | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index 9ca378d90..768267682 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -189,14 +189,19 @@ const parseScripts = function (scripts, blocks, addBroadcastMsg, getVariableId, * Create a callback for assigning fixed IDs to imported variables * Generator stores the global variable mapping in a closure * @param {!string} targetId the id of the target to scope the variable to + * @param {object=} initialVars Initial vars in scope if parsing a sprite * @return {string} variable ID */ const generateVariableIdGetter = (function () { let globalVariableNameMap = {}; const namer = (targetId, name) => `${targetId}-${name}`; - return function (targetId, topLevel) { + return function (targetId, topLevel, initialVars) { // Reset the global variable map if topLevel - if (topLevel) globalVariableNameMap = {}; + if (topLevel) { + globalVariableNameMap = {}; + } else if (initialVars) { + globalVariableNameMap = initialVars; + } return function (name) { if (topLevel) { // Store the name/id pair in the globalVariableNameMap globalVariableNameMap[name] = namer(targetId, name); @@ -334,9 +339,10 @@ const parseMonitorObject = (object, runtime, targets, extensions) => { * @param {ImportedExtensionsInfo} extensions - (in/out) parsed extension information will be stored here. * @param {boolean} topLevel - Whether this is the top-level object (stage). * @param {?object} zip - Optional zipped assets for local file import + * @param {object=} initialVarScope - Initial set of variables in scope if parsing a single sprite * @return {!Promise.>} Promise for the loaded targets when ready, or null for unsupported objects. */ -const parseScratchObject = function (object, runtime, extensions, topLevel, zip) { +const parseScratchObject = function (object, runtime, extensions, topLevel, zip, initialVarScope) { if (!object.hasOwnProperty('objName')) { if (object.hasOwnProperty('listName')) { // Shim these objects so they can be processed as monitors @@ -427,7 +433,7 @@ const parseScratchObject = function (object, runtime, extensions, topLevel, zip) // Create the first clone, and load its run-state from JSON. const target = sprite.createClone(topLevel ? StageLayering.BACKGROUND_LAYER : StageLayering.SPRITE_LAYER); - const getVariableId = generateVariableIdGetter(target.id, topLevel); + const getVariableId = generateVariableIdGetter(target.id, topLevel, initialVarScope); const globalBroadcastMsgObj = globalBroadcastMsgStateGenerator(topLevel); const addBroadcastMsg = globalBroadcastMsgObj.broadcastMsgMapUpdater; @@ -666,7 +672,19 @@ const sb2import = function (json, runtime, optForceSprite, zip) { extensionIDs: new Set(), extensionURLs: new Map() }; - return parseScratchObject(json, runtime, extensions, !optForceSprite, zip) + + const initialGlobalVars = {}; + if (optForceSprite && runtime.targets && runtime.getTargetForStage()) { + const stageVars = runtime.getTargetForStage().variables; + for (const varId in stageVars) { + const currVar = stageVars[varId]; + initialGlobalVars[currVar.name] = varId; + } + } + + // If parsing a sprite, use the current global vars as parent scope for the sprite + return parseScratchObject(json, runtime, extensions, !optForceSprite, zip, + (optForceSprite ? initialGlobalVars : null)) .then(targets => ({ targets, extensions From f2aacbc79c1f30808d325db06c35ac8d1409d7fa Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Tue, 19 Jun 2018 14:02:54 -0400 Subject: [PATCH 05/22] Fix up variable references after deserializing a .sprite2 or .sprite3 to avoid conflicts with pre-existing variables. --- src/engine/blocks.js | 64 +++++++++++++++++++++++++++++++ src/engine/target.js | 81 ++++++++++++++++++++++++++++++++++++---- src/serialization/sb2.js | 34 ++++++----------- src/serialization/sb3.js | 16 ++++++-- 4 files changed, 162 insertions(+), 33 deletions(-) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index 13122c1f1..30f43a2af 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -6,6 +6,8 @@ const Clone = require('../util/clone'); const {Map} = require('immutable'); const BlocksExecuteCache = require('./blocks-execute-cache'); const log = require('../util/log'); +const Variable = require('./variable'); +const StringUtil = require('../util/string-util'); /** * @fileoverview @@ -670,6 +672,68 @@ class Blocks { this.resetCache(); } + /** + * Fixes up variable references in this blocks container avoiding conflicts with + * pre-existing variables in the same scope. + * This is used when uploading a new sprite (the given target) + * into an existing project, where the new sprite may contain references + * to variable names that already exist as global variables in the project + * (and thus are in scope for variable references in the given sprite). + * + * If the given target has a block that references an existing global variable and that + * variable *does not* exist in the target itself (e.g. it was a global variable in the + * project the sprite was originally exported from), fix the variable references in this sprite + * to reference the id of the pre-existing global variable. + * If the given target has a block that references an existing global variable and that + * variable does exist in the target itself (e.g. it's a local variable in the sprite being uploaded), + * then the 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. + * @param {Target} target The new target being uploaded, with potential variable conflicts + * @param {Runtime} runtime The runtime context with any pre-existing variables + */ + fixUpVariableReferences (target, runtime) { + const blocks = this._blocks; + for (const blockId in blocks) { + let varOrListField = null; + let varType = null; + if (blocks[blockId].fields.VARIABLE) { + varOrListField = blocks[blockId].fields.VARIABLE; + varType = Variable.SCALAR_TYPE; + } else if (blocks[blockId].fields.LIST) { + varOrListField = blocks[blockId].fields.LIST; + varType = Variable.LIST_TYPE; + } + if (varOrListField) { + const currVarId = varOrListField.id; + const currVarName = varOrListField.value; + if (target.lookupVariableById(currVarId)) { + // Found a variable with the id in either the target or the stage, + // figure out which one. + if (target.variables.hasOwnProperty(currVarId)) { + // If the target has the variable, then check whether the stage + // has one with the same name and type. If it does, then rename + // this target specific variable so that there is a distinction. + const stage = runtime.getTargetForStage(); + if (stage && stage.variables && + stage.lookupVariableByNameAndType(currVarName, varType)) { + // TODO what should the new name be? + const newName = StringUtil.unusedName( + `${target.getName()}_${currVarName}`, + target.getAllVariableNamesInScopeByType(varType)); + target.renameVariable(currVarId, newName); + this.updateBlocksAfterVarRename(currVarId, newName); + } + } + } else { + const existingVar = target.lookupVariableByNameAndType(currVarName, varType); + if (existingVar) { + varOrListField.id = existingVar.id; + } + } + } + } + } + /** * Keep blocks up to date after a variable gets renamed. * @param {string} varId The id of the variable that was renamed diff --git a/src/engine/target.js b/src/engine/target.js index a8c6828ee..c62eb373b 100644 --- a/src/engine/target.js +++ b/src/engine/target.js @@ -80,14 +80,40 @@ class Target extends EventEmitter { } /** - * Look up a variable object, and create it if one doesn't exist. + * Get the names of all the variables of the given type that are in scope for this target. + * For targets that are not the stage, this includes any target-specific + * variables as well as any stage variables. + * For the stage, this is all stage variables. + * @param {string} type The variable type to search for; defaults to Variable.SCALAR_TYPE + * @return {Array} A list of variable names + */ + getAllVariableNamesInScopeByType (type) { + if (typeof type !== 'string') type = Variable.SCALAR_TYPE; + const targetVariables = Object.values(this.variables) + .filter(v => v.type === type) + .map(variable => variable.name); + if (this.isStage || !this.runtime) { + return targetVariables; + } + const stage = this.runtime.getTargetForStage(); + const stageVariables = stage.getAllVariableNamesInScopeByType(type); + return targetVariables.concat(stageVariables); + } + + /** + * Look up a variable object, first by id, and then by name if the id is not found. + * Create a new variable if both lookups fail. * @param {string} id Id of the variable. * @param {string} name Name of the variable. * @return {!Variable} Variable object. */ lookupOrCreateVariable (id, name) { - const variable = this.lookupVariableById(id); + let variable = this.lookupVariableById(id); if (variable) return variable; + + variable = this.lookupVariableByNameAndType(name, Variable.SCALAR_TYPE); + if (variable) return variable; + // No variable with this name exists - create it locally. const newVariable = new Variable(id, name, Variable.SCALAR_TYPE, false); this.variables[id] = newVariable; @@ -161,6 +187,40 @@ class Target extends EventEmitter { } } + /** + * Look up a variable object by its name and variable type. + * Search begins with local variables; then global variables if a local one + * was not found. + * @param {string} name Name of the variable. + * @param {string} type Type of the variable. Defaults to Variable.SCALAR_TYPE. + * @return {?Variable} Variable object if found, or null if not. + */ + lookupVariableByNameAndType (name, type) { + if (typeof name !== 'string') return; + if (typeof type !== 'string') type = Variable.SCALAR_TYPE; + + for (const varId in this.variables) { + const currVar = this.variables[varId]; + if (currVar.name === name && currVar.type === type) { + return currVar; + } + } + + if (this.runtime && !this.isStage) { + const stage = this.runtime.getTargetForStage(); + if (stage) { + for (const varId in stage.variables) { + const currVar = stage.variables[varId]; + if (currVar.name === name && currVar.type === type) { + return currVar; + } + } + } + } + + return null; + } + /** * Look up a list object for this target, and create it if one doesn't exist. * Search begins for local lists; then look for globals. @@ -169,8 +229,12 @@ class Target extends EventEmitter { * @return {!Varible} Variable object representing the found/created list. */ lookupOrCreateList (id, name) { - const list = this.lookupVariableById(id); + let list = this.lookupVariableById(id); if (list) return list; + + list = this.lookupVariableByNameAndType(name, Variable.LIST_TYPE); + if (list) return list; + // No variable with this name exists - create it locally. const newList = new Variable(id, name, Variable.LIST_TYPE, false); this.variables[id] = newList; @@ -240,10 +304,13 @@ class Target extends EventEmitter { name: 'VARIABLE', value: id }, this.runtime); - this.runtime.requestUpdateMonitor(Map({ - id: id, - params: blocks._getBlockParams(blocks.getBlock(variable.id)) - })); + const monitorBlock = blocks.getBlock(variable.id); + if (monitorBlock) { + this.runtime.requestUpdateMonitor(Map({ + id: id, + params: blocks._getBlockParams(monitorBlock) + })); + } } } diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index 768267682..afe615dfc 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -189,19 +189,14 @@ const parseScripts = function (scripts, blocks, addBroadcastMsg, getVariableId, * Create a callback for assigning fixed IDs to imported variables * Generator stores the global variable mapping in a closure * @param {!string} targetId the id of the target to scope the variable to - * @param {object=} initialVars Initial vars in scope if parsing a sprite * @return {string} variable ID */ const generateVariableIdGetter = (function () { let globalVariableNameMap = {}; const namer = (targetId, name) => `${targetId}-${name}`; - return function (targetId, topLevel, initialVars) { + return function (targetId, topLevel) { // Reset the global variable map if topLevel - if (topLevel) { - globalVariableNameMap = {}; - } else if (initialVars) { - globalVariableNameMap = initialVars; - } + if (topLevel) globalVariableNameMap = {}; return function (name) { if (topLevel) { // Store the name/id pair in the globalVariableNameMap globalVariableNameMap[name] = namer(targetId, name); @@ -339,10 +334,9 @@ const parseMonitorObject = (object, runtime, targets, extensions) => { * @param {ImportedExtensionsInfo} extensions - (in/out) parsed extension information will be stored here. * @param {boolean} topLevel - Whether this is the top-level object (stage). * @param {?object} zip - Optional zipped assets for local file import - * @param {object=} initialVarScope - Initial set of variables in scope if parsing a single sprite * @return {!Promise.>} Promise for the loaded targets when ready, or null for unsupported objects. */ -const parseScratchObject = function (object, runtime, extensions, topLevel, zip, initialVarScope) { +const parseScratchObject = function (object, runtime, extensions, topLevel, zip) { if (!object.hasOwnProperty('objName')) { if (object.hasOwnProperty('listName')) { // Shim these objects so they can be processed as monitors @@ -433,7 +427,7 @@ const parseScratchObject = function (object, runtime, extensions, topLevel, zip, // Create the first clone, and load its run-state from JSON. const target = sprite.createClone(topLevel ? StageLayering.BACKGROUND_LAYER : StageLayering.SPRITE_LAYER); - const getVariableId = generateVariableIdGetter(target.id, topLevel, initialVarScope); + const getVariableId = generateVariableIdGetter(target.id, topLevel); const globalBroadcastMsgObj = globalBroadcastMsgStateGenerator(topLevel); const addBroadcastMsg = globalBroadcastMsgObj.broadcastMsgMapUpdater; @@ -673,18 +667,14 @@ const sb2import = function (json, runtime, optForceSprite, zip) { extensionURLs: new Map() }; - const initialGlobalVars = {}; - if (optForceSprite && runtime.targets && runtime.getTargetForStage()) { - const stageVars = runtime.getTargetForStage().variables; - for (const varId in stageVars) { - const currVar = stageVars[varId]; - initialGlobalVars[currVar.name] = varId; - } - } - - // If parsing a sprite, use the current global vars as parent scope for the sprite - return parseScratchObject(json, runtime, extensions, !optForceSprite, zip, - (optForceSprite ? initialGlobalVars : null)) + return parseScratchObject(json, runtime, extensions, !optForceSprite, zip) + .then(targets => { + if (optForceSprite && targets.length === 1) { + const target = targets[0]; + target.blocks.fixUpVariableReferences(target, runtime); + } + return targets; + }) .then(targets => ({ targets, extensions diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index 1c02c2aec..d63fa2049 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -936,10 +936,18 @@ const deserialize = function (json, runtime, zip, isSingleSprite) { return Promise.all( ((isSingleSprite ? [json] : json.targets) || []).map(target => parseScratchObject(target, runtime, extensions, zip)) - ).then(targets => ({ - targets, - extensions - })); + ) + .then(targets => { + if (isSingleSprite && targets.length === 1) { + const target = targets[0]; + target.blocks.fixUpVariableReferences(target, runtime); + } + return targets; + }) + .then(targets => ({ + targets, + extensions + })); }; module.exports = { From 9646e3d11e363e5db08777dc4ce14f62ddbe87f8 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Tue, 19 Jun 2018 15:31:08 -0400 Subject: [PATCH 06/22] Refactor and move variable reference fixup function into target. --- src/engine/blocks.js | 59 +++++++++++------------------------- src/engine/target.js | 65 ++++++++++++++++++++++++++++++++++++++++ src/serialization/sb2.js | 2 +- src/serialization/sb3.js | 2 +- 4 files changed, 84 insertions(+), 44 deletions(-) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index 30f43a2af..0a6750224 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -7,7 +7,6 @@ const {Map} = require('immutable'); const BlocksExecuteCache = require('./blocks-execute-cache'); const log = require('../util/log'); const Variable = require('./variable'); -const StringUtil = require('../util/string-util'); /** * @fileoverview @@ -673,26 +672,15 @@ class Blocks { } /** - * Fixes up variable references in this blocks container avoiding conflicts with - * pre-existing variables in the same scope. - * This is used when uploading a new sprite (the given target) - * into an existing project, where the new sprite may contain references - * to variable names that already exist as global variables in the project - * (and thus are in scope for variable references in the given sprite). - * - * If the given target has a block that references an existing global variable and that - * variable *does not* exist in the target itself (e.g. it was a global variable in the - * project the sprite was originally exported from), fix the variable references in this sprite - * to reference the id of the pre-existing global variable. - * If the given target has a block that references an existing global variable and that - * variable does exist in the target itself (e.g. it's a local variable in the sprite being uploaded), - * then the 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. - * @param {Target} target The new target being uploaded, with potential variable conflicts - * @param {Runtime} runtime The runtime context with any pre-existing variables + * Returns a map of all references to variables or lists from blocks + * in this block container. + * @return {object} A map of variable ID to a list of all variable references + * for that ID. A variable reference contains the field referencing that variable + * and also the type of the variable being referenced. */ - fixUpVariableReferences (target, runtime) { + getAllVariableAndListReferences () { const blocks = this._blocks; + const allReferences = Object.create(null); for (const blockId in blocks) { let varOrListField = null; let varType = null; @@ -705,33 +693,20 @@ class Blocks { } if (varOrListField) { const currVarId = varOrListField.id; - const currVarName = varOrListField.value; - if (target.lookupVariableById(currVarId)) { - // Found a variable with the id in either the target or the stage, - // figure out which one. - if (target.variables.hasOwnProperty(currVarId)) { - // If the target has the variable, then check whether the stage - // has one with the same name and type. If it does, then rename - // this target specific variable so that there is a distinction. - const stage = runtime.getTargetForStage(); - if (stage && stage.variables && - stage.lookupVariableByNameAndType(currVarName, varType)) { - // TODO what should the new name be? - const newName = StringUtil.unusedName( - `${target.getName()}_${currVarName}`, - target.getAllVariableNamesInScopeByType(varType)); - target.renameVariable(currVarId, newName); - this.updateBlocksAfterVarRename(currVarId, newName); - } - } + if (allReferences[currVarId]) { + allReferences[currVarId].push({ + referencingField: varOrListField, + type: varType + }); } else { - const existingVar = target.lookupVariableByNameAndType(currVarName, varType); - if (existingVar) { - varOrListField.id = existingVar.id; - } + allReferences[currVarId] = [{ + referencingField: varOrListField, + type: varType + }]; } } } + return allReferences; } /** diff --git a/src/engine/target.js b/src/engine/target.js index c62eb373b..e456a88b8 100644 --- a/src/engine/target.js +++ b/src/engine/target.js @@ -6,6 +6,7 @@ const Comment = require('../engine/comment'); const uid = require('../util/uid'); const {Map} = require('immutable'); const log = require('../util/log'); +const StringUtil = require('../util/string-util'); /** * @fileoverview @@ -331,6 +332,70 @@ class Target extends EventEmitter { } } + /** + * Fixes up variable references in this target avoiding conflicts with + * pre-existing variables in the same scope. + * This is used when uploading this target as a new sprite into an existing + * project, where the new sprite may contain references + * to variable names that already exist as global variables in the project + * (and thus are in scope for variable references in the given sprite). + * + * If the given target has a block that references an existing global variable and that + * variable *does not* exist in the target itself (e.g. it was a global variable in the + * project the sprite was originally exported from), fix the variable references in this sprite + * to reference the id of the pre-existing global variable. + * If the given target has a block that references an existing global variable and that + * variable does exist in the target itself (e.g. it's a local variable in the sprite being uploaded), + * then the 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. + // * @param {Target} target The new target being uploaded, with potential variable conflicts + // * @param {Runtime} runtime The runtime context with any pre-existing variables + */ + 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) + + const allReferences = this.blocks.getAllVariableAndListReferences(); + const conflictIdsToReplace = Object.create(null); + 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]; + const varName = varRef.referencingField.value; + const varType = varRef.type; + if (this.lookupVariableById(varId)) { + // Found a variable with the id in either the target or the stage, + // figure out which one. + if (this.variables.hasOwnProperty(varId)) { + // If the target has the variable, then check whether the stage + // has one with the same name and type. If it does, then rename + // this target specific variable so that there is a distinction. + const stage = this.runtime.getTargetForStage(); + if (stage && stage.variables && + stage.lookupVariableByNameAndType(varName, varType)) { + // TODO what should the new name be? + const newName = StringUtil.unusedName( + `${this.getName()}_${varName}`, + this.getAllVariableNamesInScopeByType(varType)); + this.renameVariable(varId, newName); + this.blocks.updateBlocksAfterVarRename(varId, newName); + } + } + } else { + const existingVar = this.lookupVariableByNameAndType(varName, varType); + if (existingVar && !conflictIdsToReplace[varId]) { + conflictIdsToReplace[varId] = existingVar.id; + } + } + } + for (const conflictId in conflictIdsToReplace) { + const existingId = conflictIdsToReplace[conflictId]; + allReferences[conflictId].map(varRef => { + varRef.referencingField.id = existingId; + return varRef; + }); + } + } + /** * Post/edit sprite info. * @param {object} data An object with sprite info data to set. diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index afe615dfc..60978eea3 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -671,7 +671,7 @@ const sb2import = function (json, runtime, optForceSprite, zip) { .then(targets => { if (optForceSprite && targets.length === 1) { const target = targets[0]; - target.blocks.fixUpVariableReferences(target, runtime); + target.fixUpVariableReferences(); } return targets; }) diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index d63fa2049..fb4cbeffd 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -940,7 +940,7 @@ const deserialize = function (json, runtime, zip, isSingleSprite) { .then(targets => { if (isSingleSprite && targets.length === 1) { const target = targets[0]; - target.blocks.fixUpVariableReferences(target, runtime); + target.fixUpVariableReferences(); } return targets; }) From 073bb37c30ac13b655a3dbe449a4565f789d9695 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 20 Jun 2018 10:32:36 -0400 Subject: [PATCH 07/22] Add a test for 'getAllVariableAndListReferences' function. --- test/fixtures/events.json | 12 ++++++++++++ test/unit/engine_blocks.js | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/test/fixtures/events.json b/test/fixtures/events.json index a5a126f07..dbddd2777 100644 --- a/test/fixtures/events.json +++ b/test/fixtures/events.json @@ -90,5 +90,17 @@ "type": "comment_create", "commentId": "a comment", "xy": {"x": 10, "y": 20} + }, + "mockVariableBlock": { + "name": "block", + "xml": { + "outerHTML": "a mock variable" + } + }, + "mockListBlock": { + "name": "block", + "xml": { + "outerHTML": "a mock list" + } } } diff --git a/test/unit/engine_blocks.js b/test/unit/engine_blocks.js index 08b0b414e..5659b6878 100644 --- a/test/unit/engine_blocks.js +++ b/test/unit/engine_blocks.js @@ -1,5 +1,8 @@ const test = require('tap').test; const Blocks = require('../../src/engine/blocks'); +const Variable = require('../../src/engine/variable'); +const adapter = require('../../src/engine/adapter'); +const events = require('../fixtures/events.json'); test('spec', t => { const b = new Blocks(); @@ -776,3 +779,32 @@ test('updateTargetSpecificBlocks changes sprite clicked hat to stage clicked for t.end(); }); + +test('getAllVariableAndListReferences returns an empty map references when variable blocks do not exist', t => { + const b = new Blocks(); + t.equal(Object.keys(b.getAllVariableAndListReferences()).length, 0); + t.end(); +}); + +test('getAllVariableAndListReferences returns references when variable blocks exist', t => { + const b = new Blocks(); + + let varListRefs = b.getAllVariableAndListReferences(); + t.equal(Object.keys(varListRefs).length, 0); + + b.createBlock(adapter(events.mockVariableBlock)[0]); + b.createBlock(adapter(events.mockListBlock)[0]); + + varListRefs = b.getAllVariableAndListReferences(); + t.equal(Object.keys(varListRefs).length, 2); + t.equal(Array.isArray(varListRefs['mock var id']), true); + t.equal(varListRefs['mock var id'].length, 1); + t.equal(varListRefs['mock var id'][0].type, Variable.SCALAR_TYPE); + t.equal(varListRefs['mock var id'][0].referencingField.value, 'a mock variable'); + t.equal(Array.isArray(varListRefs['mock list id']), true); + t.equal(varListRefs['mock list id'].length, 1); + t.equal(varListRefs['mock list id'][0].type, Variable.LIST_TYPE); + t.equal(varListRefs['mock list id'][0].referencingField.value, 'a mock list'); + + t.end(); +}); From 889443fcef313f4dfa053085f403b3f28024fb2a Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 20 Jun 2018 12:07:42 -0400 Subject: [PATCH 08/22] fixUpVariableReferences should handle conflicting local variables that don't have blocks referencing them. Add unit tests for the function. --- src/engine/target.js | 51 ++++++++++-- test/unit/engine_target.js | 165 ++++++++++++++++++++++++++++++++++++- 2 files changed, 206 insertions(+), 10 deletions(-) diff --git a/src/engine/target.js b/src/engine/target.js index e456a88b8..b5d94eb5a 100644 --- a/src/engine/target.js +++ b/src/engine/target.js @@ -354,8 +354,29 @@ class Target extends EventEmitter { 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) + const stage = this.runtime.getTargetForStage(); + if (!stage || !stage.variables) return; + + const renameConflictingLocalVar = (id, name, type) => { + const conflict = stage.lookupVariableByNameAndType(name, type); + if (conflict) { + const newName = StringUtil.unusedName( + `${this.getName()}: ${name}`, + this.getAllVariableNamesInScopeByType(type)); + this.renameVariable(id, newName); + return newName; + } + return null; + }; const allReferences = this.blocks.getAllVariableAndListReferences(); + const unreferencedLocalVarIds = []; + if (Object.keys(this.variables).length > 0) { + for (const localVarId in this.variables) { + if (!this.variables.hasOwnProperty(localVarId)) continue; + if (!allReferences[localVarId]) unreferencedLocalVarIds.push(localVarId); + } + } const conflictIdsToReplace = Object.create(null); for (const varId in allReferences) { // We don't care about which var ref we get, they should all have the same var info @@ -369,15 +390,16 @@ class Target extends EventEmitter { // If the target has the variable, then check whether the stage // has one with the same name and type. If it does, then rename // this target specific variable so that there is a distinction. - const stage = this.runtime.getTargetForStage(); - if (stage && stage.variables && - stage.lookupVariableByNameAndType(varName, varType)) { - // TODO what should the new name be? - const newName = StringUtil.unusedName( - `${this.getName()}_${varName}`, - this.getAllVariableNamesInScopeByType(varType)); - this.renameVariable(varId, newName); - this.blocks.updateBlocksAfterVarRename(varId, newName); + const newVarName = renameConflictingLocalVar(varId, varName, varType); + + if (newVarName) { + // We are not calling this.blocks.updateBlocksAfterVarRename + // here because it will search through all the blocks. We already + // have access to all the references for this var id. + allReferences[varId].map(ref => { + ref.referencingField.value = newVarName; + return ref; + }); } } } else { @@ -387,6 +409,17 @@ class Target extends EventEmitter { } } } + // Rename any local variables that were missed above because they aren't + // referenced by any blocks + for (const id in unreferencedLocalVarIds) { + const varId = unreferencedLocalVarIds[id]; + const name = this.variables[varId].name; + const type = this.variables[varId].type; + renameConflictingLocalVar(varId, name, type); + } + // Finally, handle global var conflicts (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) for (const conflictId in conflictIdsToReplace) { const existingId = conflictIdsToReplace[conflictId]; allReferences[conflictId].map(varRef => { diff --git a/test/unit/engine_target.js b/test/unit/engine_target.js index a92dd5e61..c39154cd2 100644 --- a/test/unit/engine_target.js +++ b/test/unit/engine_target.js @@ -1,6 +1,9 @@ const test = require('tap').test; const Target = require('../../src/engine/target'); const Variable = require('../../src/engine/variable'); +const adapter = require('../../src/engine/adapter'); +const Runtime = require('../../src/engine/runtime'); +const events = require('../fixtures/events.json'); test('spec', t => { const target = new Target(); @@ -145,7 +148,7 @@ test('deleteVariable2', t => { t.end(); }); -test('lookupOrCreateList creates a list if var with given id does not exist', t => { +test('lookupOrCreateList creates a list if var with given id or var with given name does not exist', t => { const target = new Target(); const variables = target.variables; @@ -174,6 +177,22 @@ test('lookupOrCreateList returns list if one with given id exists', t => { t.end(); }); +test('lookupOrCreateList succeeds in finding list if id is incorrect but name matches', t => { + const target = new Target(); + const variables = target.variables; + + t.equal(Object.keys(variables).length, 0); + target.createVariable('foo', 'bar', Variable.LIST_TYPE); + t.equal(Object.keys(variables).length, 1); + + const listVar = target.lookupOrCreateList('not foo', 'bar'); + t.equal(Object.keys(variables).length, 1); + t.equal(listVar.id, 'foo'); + t.equal(listVar.name, 'bar'); + + t.end(); +}); + test('lookupBroadcastMsg returns the var with given id if exists', t => { const target = new Target(); const variables = target.variables; @@ -263,3 +282,147 @@ test('creating a comment with a blockId also updates the comment property on the t.end(); }); + +test('fixUpVariableReferences fixes sprite global var conflicting with project global var', t => { + const runtime = new Runtime(); + + const stage = new Target(runtime); + stage.isStage = true; + + const target = new Target(runtime); + target.isStage = false; + + runtime.targets = [stage, target]; + + // Create a global variable + stage.createVariable('pre-existing global var id', 'a mock variable', Variable.SCALAR_TYPE); + + target.blocks.createBlock(adapter(events.mockVariableBlock)[0]); + + 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'); + + target.fixUpVariableReferences(target, runtime); + + 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, 'pre-existing global var id'); + + t.end(); +}); + +test('fixUpVariableReferences fixes sprite local var conflicting with project global var', t => { + const runtime = new Runtime(); + + const stage = new Target(runtime); + stage.isStage = true; + + const target = new Target(runtime); + target.isStage = false; + target.getName = () => 'Target'; + + runtime.targets = [stage, target]; + + // Create a global variable + stage.createVariable('pre-existing global var id', 'a mock variable', Variable.SCALAR_TYPE); + target.createVariable('mock var id', 'a mock variable', Variable.SCALAR_TYPE); + + target.blocks.createBlock(adapter(events.mockVariableBlock)[0]); + + t.equal(Object.keys(target.variables).length, 1); + 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'); + t.equal(target.variables['mock var id'].name, 'a mock variable'); + + target.fixUpVariableReferences(target, runtime); + + t.equal(Object.keys(target.variables).length, 1); + 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'); + t.equal(target.variables['mock var id'].name, 'Target: a mock variable'); + + t.end(); +}); + +test('fixUpVariableReferences fixes conflicting sprite local var without blocks referencing var', t => { + const runtime = new Runtime(); + + const stage = new Target(runtime); + stage.isStage = true; + + const target = new Target(runtime); + target.isStage = false; + target.getName = () => 'Target'; + + runtime.targets = [stage, target]; + + // Create a global variable + stage.createVariable('pre-existing global var id', 'a mock variable', Variable.SCALAR_TYPE); + target.createVariable('mock var id', 'a mock variable', Variable.SCALAR_TYPE); + + + t.equal(Object.keys(target.variables).length, 1); + t.equal(Object.keys(stage.variables).length, 1); + t.equal(target.variables['mock var id'].name, 'a mock variable'); + + target.fixUpVariableReferences(target, runtime); + + t.equal(Object.keys(target.variables).length, 1); + t.equal(Object.keys(stage.variables).length, 1); + t.equal(target.variables['mock var id'].name, 'Target: a mock variable'); + + t.end(); +}); + +test('fixUpVariableReferences does not change variable name if there is no variable conflict', t => { + const runtime = new Runtime(); + + const stage = new Target(runtime); + stage.isStage = true; + + const target = new Target(runtime); + target.isStage = false; + target.getName = () => 'Target'; + + runtime.targets = [stage, target]; + + // Create a global variable + stage.createVariable('pre-existing global var id', 'a variable', Variable.SCALAR_TYPE); + stage.createVariable('pre-existing global list id', 'a mock variable', Variable.LIST_TYPE); + target.createVariable('mock var id', 'a mock variable', Variable.SCALAR_TYPE); + + target.blocks.createBlock(adapter(events.mockVariableBlock)[0]); + + t.equal(Object.keys(target.variables).length, 1); + t.equal(Object.keys(stage.variables).length, 2); + 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'); + t.equal(target.variables['mock var id'].name, 'a mock variable'); + + target.fixUpVariableReferences(target, runtime); + + t.equal(Object.keys(target.variables).length, 1); + t.equal(Object.keys(stage.variables).length, 2); + 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'); + t.equal(target.variables['mock var id'].name, 'a mock variable'); + + t.end(); +}); From b75a77954040b24e99acfe1f9938b479fdaaba5f Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 20 Jun 2018 12:12:33 -0400 Subject: [PATCH 09/22] Call fixUpVariableReferences in installTargets, before emitting the workspace update. --- src/serialization/sb2.js | 7 ------- src/serialization/sb3.js | 7 ------- src/virtual-machine.js | 4 ++++ 3 files changed, 4 insertions(+), 14 deletions(-) diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index 60978eea3..a1533b67f 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -668,13 +668,6 @@ const sb2import = function (json, runtime, optForceSprite, zip) { }; return parseScratchObject(json, runtime, extensions, !optForceSprite, zip) - .then(targets => { - if (optForceSprite && targets.length === 1) { - const target = targets[0]; - target.fixUpVariableReferences(); - } - return targets; - }) .then(targets => ({ targets, extensions diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index fb4cbeffd..0ecff4ed5 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -937,13 +937,6 @@ const deserialize = function (json, runtime, zip, isSingleSprite) { ((isSingleSprite ? [json] : json.targets) || []).map(target => parseScratchObject(target, runtime, extensions, zip)) ) - .then(targets => { - if (isSingleSprite && targets.length === 1) { - const target = targets[0]; - target.fixUpVariableReferences(); - } - return targets; - }) .then(targets => ({ targets, extensions diff --git a/src/virtual-machine.js b/src/virtual-machine.js index 378f9021a..b0571e52c 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -386,6 +386,10 @@ class VirtualMachine extends EventEmitter { this.editingTarget = targets[0]; } + if (!wholeProject) { + this.editingTarget.fixUpVariableReferences(); + } + // Update the VM user's knowledge of targets and blocks on the workspace. this.emitTargetsUpdate(); this.emitWorkspaceUpdate(); From b40ac54fdd5b0f862fade0c5741b8d5276b6638e Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 20 Jun 2018 13:37:31 -0400 Subject: [PATCH 10/22] Remove stray newline. --- src/serialization/sb2.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index a1533b67f..9ca378d90 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -666,7 +666,6 @@ const sb2import = function (json, runtime, optForceSprite, zip) { extensionIDs: new Set(), extensionURLs: new Map() }; - return parseScratchObject(json, runtime, extensions, !optForceSprite, zip) .then(targets => ({ targets, From 7e48bed0abbd04d2c6e8f1f1d583f7cb76d89849 Mon Sep 17 00:00:00 2001 From: "Michael \"Z\" Goddard" Date: Wed, 20 Jun 2018 14:33:39 -0400 Subject: [PATCH 11/22] use scratch-audio EffectChain and SoundPlayer in music extension - Use AudioEngine to decode sounds - Store players instead of buffers - Use SoundPlayer stop event to track concurrency --- src/extensions/scratch3_music/index.js | 151 ++++++++++++++----------- 1 file changed, 87 insertions(+), 64 deletions(-) diff --git a/src/extensions/scratch3_music/index.js b/src/extensions/scratch3_music/index.js index 4450a69e8..8d73067b3 100644 --- a/src/extensions/scratch3_music/index.js +++ b/src/extensions/scratch3_music/index.js @@ -52,18 +52,25 @@ class Scratch3MusicBlocks { this._concurrencyCounter = 0; /** - * An array of audio buffers, one for each drum sound. + * An array of sound players, one for each drum sound. * @type {Array} * @private */ - this._drumBuffers = []; + this._drumPlayers = []; /** - * An array of arrays of audio buffers. Each instrument has one or more audio buffers. + * An array of arrays of sound players. Each instrument has one or more audio players. * @type {Array[]} * @private */ - this._instrumentBufferArrays = []; + this._instrumentPlayerArrays = []; + + /** + * An array of arrays of sound players. Each instrument mya have an audio player for each playable note. + * @type {Array[]} + * @private + */ + this._instrumentPlayerNoteArrays = []; /** * An array of audio bufferSourceNodes. Each time you play an instrument or drum sound, @@ -87,14 +94,15 @@ class Scratch3MusicBlocks { const loadingPromises = []; this.DRUM_INFO.forEach((drumInfo, index) => { const filePath = `drums/${drumInfo.fileName}`; - const promise = this._storeSound(filePath, index, this._drumBuffers); + const promise = this._storeSound(filePath, index, this._drumPlayers); loadingPromises.push(promise); }); this.INSTRUMENT_INFO.forEach((instrumentInfo, instrumentIndex) => { - this._instrumentBufferArrays[instrumentIndex] = []; + this._instrumentPlayerArrays[instrumentIndex] = []; + this._instrumentPlayerNoteArrays[instrumentIndex] = []; instrumentInfo.samples.forEach((sample, noteIndex) => { const filePath = `instruments/${instrumentInfo.dirName}/${sample}`; - const promise = this._storeSound(filePath, noteIndex, this._instrumentBufferArrays[instrumentIndex]); + const promise = this._storeSound(filePath, noteIndex, this._instrumentPlayerArrays[instrumentIndex]); loadingPromises.push(promise); }); }); @@ -104,22 +112,22 @@ class Scratch3MusicBlocks { } /** - * Decode a sound and store the buffer in an array. + * Decode a sound and store the player in an array. * @param {string} filePath - the audio file name. - * @param {number} index - the index at which to store the audio buffer. - * @param {array} bufferArray - the array of buffers in which to store it. + * @param {number} index - the index at which to store the audio player. + * @param {array} playerArray - the array of players in which to store it. * @return {Promise} - a promise which will resolve once the sound has been stored. */ - _storeSound (filePath, index, bufferArray) { + _storeSound (filePath, index, playerArray) { const fullPath = `${filePath}.mp3`; if (!assetData[fullPath]) return; - // The sound buffer has already been downloaded via the manifest file required above. + // The sound player has already been downloaded via the manifest file required above. const soundBuffer = assetData[fullPath]; - return this._decodeSound(soundBuffer).then(buffer => { - bufferArray[index] = buffer; + return this._decodeSound(soundBuffer).then(player => { + playerArray[index] = player; }); } @@ -129,24 +137,14 @@ class Scratch3MusicBlocks { * @return {Promise} - a promise which will resolve once the sound has decoded. */ _decodeSound (soundBuffer) { - const context = this.runtime.audioEngine && this.runtime.audioEngine.audioContext; + const engine = this.runtime.audioEngine; - if (!context) { + if (!engine) { return Promise.reject(new Error('No Audio Context Detected')); } // Check for newer promise-based API - if (context.decodeAudioData.length === 1) { - return context.decodeAudioData(soundBuffer); - } else { // eslint-disable-line no-else-return - // Fall back to callback API - return new Promise((resolve, reject) => - context.decodeAudioData(soundBuffer, - buffer => resolve(buffer), - error => reject(error) - ) - ); - } + return engine.decodeSoundPlayer({data: {buffer: soundBuffer}}); } /** @@ -774,26 +772,34 @@ class Scratch3MusicBlocks { */ _playDrumNum (util, drumNum) { if (util.runtime.audioEngine === null) return; - if (util.target.audioPlayer === null) return; + if (util.target.sprite.soundBank === null) return; // If we're playing too many sounds, do not play the drum sound. if (this._concurrencyCounter > Scratch3MusicBlocks.CONCURRENCY_LIMIT) { return; } - const outputNode = util.target.audioPlayer.getInputNode(); - const context = util.runtime.audioEngine.audioContext; - const bufferSource = context.createBufferSource(); - bufferSource.buffer = this._drumBuffers[drumNum]; - bufferSource.connect(outputNode); - bufferSource.start(); - const bufferSourceIndex = this._bufferSources.length; - this._bufferSources.push(bufferSource); + const player = this._drumPlayers[drumNum]; + + if (typeof player === 'undefined') return; + + if (player.isPlaying) { + // Take the internal player state and create a new player with it. + // `.play` does this internally but then instructs the sound to + // stop. + player.take(); + } + + const engine = util.runtime.audioEngine; + const chain = engine.createEffectChain(); + chain.setEffectsFromTarget(util.target); + player.connect(chain); this._concurrencyCounter++; - bufferSource.onended = () => { + player.once('stop', () => { this._concurrencyCounter--; - delete this._bufferSources[bufferSourceIndex]; - }; + }); + + player.play(); } /** @@ -852,7 +858,7 @@ class Scratch3MusicBlocks { */ _playNote (util, note, durationSec) { if (util.runtime.audioEngine === null) return; - if (util.target.audioPlayer === null) return; + if (util.target.sprite.soundBank === null) return; // If we're playing too many sounds, do not play the note. if (this._concurrencyCounter > Scratch3MusicBlocks.CONCURRENCY_LIMIT) { @@ -867,28 +873,37 @@ class Scratch3MusicBlocks { const sampleIndex = this._selectSampleIndexForNote(note, sampleArray); // If the audio sample has not loaded yet, bail out - if (typeof this._instrumentBufferArrays[inst] === 'undefined') return; - if (typeof this._instrumentBufferArrays[inst][sampleIndex] === 'undefined') return; + if (typeof this._instrumentPlayerArrays[inst] === 'undefined') return; + if (typeof this._instrumentPlayerArrays[inst][sampleIndex] === 'undefined') return; - // Create the audio buffer to play the note, and set its pitch - const context = util.runtime.audioEngine.audioContext; - const bufferSource = context.createBufferSource(); + // Fetch the sound player to play the note. + const engine = util.runtime.audioEngine; - const bufferSourceIndex = this._bufferSources.length; - this._bufferSources.push(bufferSource); + if (!this._instrumentPlayerNoteArrays[inst][note]) { + this._instrumentPlayerNoteArrays[inst][note] = this._instrumentPlayerArrays[inst][sampleIndex].take(); + } - bufferSource.buffer = this._instrumentBufferArrays[inst][sampleIndex]; + const player = this._instrumentPlayerNoteArrays[inst][note]; + + if (player.isPlaying) { + // Take the internal player state and create a new player with it. + // `.play` does this internally but then instructs the sound to + // stop. + player.take(); + } + + const chain = engine.createEffectChain(); + chain.setEffectsFromTarget(util.target); + + // Set its pitch. const sampleNote = sampleArray[sampleIndex]; - bufferSource.playbackRate.value = this._ratioForPitchInterval(note - sampleNote); + const notePitchInterval = this._ratioForPitchInterval(note - sampleNote); - // Create a gain node for this note, and connect it to the sprite's audioPlayer. - const gainNode = context.createGain(); - bufferSource.connect(gainNode); - const outputNode = util.target.audioPlayer.getInputNode(); - gainNode.connect(outputNode); - - // Start playing the note - bufferSource.start(); + // Create a gain node for this note, and connect it to the sprite's + // simulated effectChain. + const context = engine.audioContext; + const releaseGain = context.createGain(); + releaseGain.connect(chain.getInputNode()); // Schedule the release of the note, ramping its gain down to zero, // and then stopping the sound. @@ -898,16 +913,24 @@ class Scratch3MusicBlocks { } const releaseStart = context.currentTime + durationSec; const releaseEnd = releaseStart + releaseDuration; - gainNode.gain.setValueAtTime(1, releaseStart); - gainNode.gain.linearRampToValueAtTime(0.0001, releaseEnd); - bufferSource.stop(releaseEnd); + releaseGain.gain.setValueAtTime(1, releaseStart); + releaseGain.gain.linearRampToValueAtTime(0.0001, releaseEnd); - // Update the concurrency counter this._concurrencyCounter++; - bufferSource.onended = () => { + player.once('stop', () => { this._concurrencyCounter--; - delete this._bufferSources[bufferSourceIndex]; - }; + }); + + // Start playing the note + player.play(); + // Connect the player to the gain node. + player.connect({getInputNode () { + return releaseGain; + }}); + // Set playback now after play creates the outputNode. + player.outputNode.playbackRate.value = notePitchInterval; + // Schedule playback to stop. + player.outputNode.stop(releaseEnd); } /** From c087cf326a7866d89d42486a9b23a609c4292d69 Mon Sep 17 00:00:00 2001 From: Corey Frang Date: Fri, 15 Jun 2018 12:22:49 -0400 Subject: [PATCH 12/22] soundbank tests --- src/blocks/scratch3_sound.js | 47 +++++++++++++++------------------- src/sprites/rendered-target.js | 40 ++++++++++++++++------------- src/sprites/sprite.js | 14 +++++++++- test/unit/blocks_sounds.js | 8 +++--- 4 files changed, 60 insertions(+), 49 deletions(-) diff --git a/src/blocks/scratch3_sound.js b/src/blocks/scratch3_sound.js index a599b314c..16b041bf8 100644 --- a/src/blocks/scratch3_sound.js +++ b/src/blocks/scratch3_sound.js @@ -88,6 +88,7 @@ class Scratch3SoundBlocks { if (!soundState) { soundState = Clone.simple(Scratch3SoundBlocks.DEFAULT_SOUND_STATE); target.setCustomState(Scratch3SoundBlocks.STATE_KEY, soundState); + target.soundEffects = soundState.effects; } return soundState; } @@ -139,20 +140,19 @@ class Scratch3SoundBlocks { } playSound (args, util) { - const index = this._getSoundIndex(args.SOUND_MENU, util); - if (index >= 0) { - const soundId = util.target.sprite.sounds[index].soundId; - if (util.target.audioPlayer === null) return; - util.target.audioPlayer.playSound(soundId); - } + // Don't return the promise, it's the only difference for AndWait + this.playSoundAndWait(args, util); } playSoundAndWait (args, util) { const index = this._getSoundIndex(args.SOUND_MENU, util); if (index >= 0) { - const soundId = util.target.sprite.sounds[index].soundId; - if (util.target.audioPlayer === null) return; - return util.target.audioPlayer.playSound(soundId); + const {target} = util; + const {sprite} = target; + const {soundId} = sprite.sounds[index]; + if (sprite.soundBank) { + return sprite.soundBank.playSound(target, soundId); + } } } @@ -199,8 +199,9 @@ class Scratch3SoundBlocks { } _stopAllSoundsForTarget (target) { - if (target.audioPlayer === null) return; - target.audioPlayer.stopAllSounds(); + if (target.sprite.soundBank) { + target.sprite.soundBank.stopAllSounds(target); + } } setEffect (args, util) { @@ -224,23 +225,19 @@ class Scratch3SoundBlocks { soundState.effects[effect] = value; } - const effectRange = Scratch3SoundBlocks.EFFECT_RANGE[effect]; - soundState.effects[effect] = MathUtil.clamp(soundState.effects[effect], effectRange.min, effectRange.max); - - if (util.target.audioPlayer === null) return; - util.target.audioPlayer.setEffect(effect, soundState.effects[effect]); + const {min, max} = Scratch3SoundBlocks.EFFECT_RANGE[effect]; + soundState.effects[effect] = MathUtil.clamp(soundState.effects[effect], min, max); + this._syncEffectsForTarget(util.target); // Yield until the next tick. return Promise.resolve(); } _syncEffectsForTarget (target) { - if (!target || !target.audioPlayer) return; - const soundState = this._getSoundState(target); - for (const effect in soundState.effects) { - if (!soundState.effects.hasOwnProperty(effect)) continue; - target.audioPlayer.setEffect(effect, soundState.effects[effect]); - } + if (!target || !target.sprite.soundBank) return; + target.soundEffects = this._getSoundState(target).effects; + + target.sprite.soundBank.setEffects(target); } clearEffects (args, util) { @@ -253,8 +250,7 @@ class Scratch3SoundBlocks { if (!soundState.effects.hasOwnProperty(effect)) continue; soundState.effects[effect] = 0; } - if (target.audioPlayer === null) return; - target.audioPlayer.clearEffects(); + this._syncEffectsForTarget(target); } _clearEffectsForAllTargets () { @@ -278,8 +274,7 @@ class Scratch3SoundBlocks { _updateVolume (volume, util) { volume = MathUtil.clamp(volume, 0, 100); util.target.volume = volume; - if (util.target.audioPlayer === null) return; - util.target.audioPlayer.setVolume(util.target.volume); + this._syncEffectsForTarget(util.target); // Yield until the next tick. return Promise.resolve(); diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index aac0b059a..293666acf 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -170,21 +170,30 @@ class RenderedTarget extends Target { } } + get audioPlayer () { + /* eslint-disable no-console */ + console.warn('get audioPlayer deprecated, please update to use .sprite.soundBank methods'); + console.warn(new Error('stack for debug').stack); + /* eslint-enable no-console */ + const bank = this.sprite.soundBank; + const audioPlayerProxy = { + playSound: soundId => bank.play(this, soundId) + }; + + Object.defineProperty(this, 'audioPlayer', { + configurable: false, + enumerable: true, + writable: false, + value: audioPlayerProxy + }); + + return audioPlayerProxy; + } + /** * Initialize the audio player for this sprite or clone. */ initAudio () { - this.audioPlayer = null; - if (this.runtime && this.runtime.audioEngine) { - this.audioPlayer = this.runtime.audioEngine.createPlayer(); - // If this is a clone, it gets a reference to its parent's activeSoundPlayers object. - if (!this.isOriginal) { - const parent = this.sprite.clones[0]; - if (parent && parent.audioPlayer) { - this.audioPlayer.activeSoundPlayers = parent.audioPlayer.activeSoundPlayers; - } - } - } } /** @@ -1034,9 +1043,8 @@ class RenderedTarget extends Target { */ onStopAll () { this.clearEffects(); - if (this.audioPlayer) { - this.audioPlayer.stopAllSounds(); - this.audioPlayer.clearEffects(); + if (this.sprite.soundBank) { + this.sprite.soundBank.stopAllSounds(this); } } @@ -1132,10 +1140,6 @@ class RenderedTarget extends Target { this.runtime.requestRedraw(); } } - if (this.audioPlayer) { - this.audioPlayer.stopAllSounds(); - this.audioPlayer.dispose(); - } } } diff --git a/src/sprites/sprite.js b/src/sprites/sprite.js index de735ecc3..c7c35c9e0 100644 --- a/src/sprites/sprite.js +++ b/src/sprites/sprite.js @@ -8,7 +8,8 @@ const StageLayering = require('../engine/stage-layering'); class Sprite { /** * Sprite to be used on the Scratch stage. - * All clones of a sprite have shared blocks, shared costumes, shared variables. + * All clones of a sprite have shared blocks, shared costumes, shared variables, + * shared sounds, etc. * @param {?Blocks} blocks Shared blocks object for all clones of sprite. * @param {Runtime} runtime Reference to the runtime. * @constructor @@ -47,6 +48,11 @@ class Sprite { * @type {Array.} */ this.clones = []; + + this.soundBank = null; + if (this.runtime && this.runtime.audioEngine) { + this.soundBank = this.runtime.audioEngine.createBank(); + } } /** @@ -155,6 +161,12 @@ class Sprite { return Promise.all(assetPromises).then(() => newSprite); } + + dispose () { + if (this.soundBank) { + this.soundBank.dispose(); + } + } } module.exports = Sprite; diff --git a/test/unit/blocks_sounds.js b/test/unit/blocks_sounds.js index 9a44e7f9f..53acc8aff 100644 --- a/test/unit/blocks_sounds.js +++ b/test/unit/blocks_sounds.js @@ -11,10 +11,10 @@ const util = { {name: 'second name', soundId: 'second soundId'}, {name: 'third name', soundId: 'third soundId'}, {name: '6', soundId: 'fourth soundId'} - ] - }, - audioPlayer: { - playSound: soundId => (playedSound = soundId) + ], + soundBank: { + playSound: (target, soundId) => (playedSound = soundId) + } } } }; From 14f8c44ad9c6af52cfd8a2cb21d275617166fca3 Mon Sep 17 00:00:00 2001 From: Corey Frang Date: Fri, 15 Jun 2018 12:54:44 -0400 Subject: [PATCH 13/22] Stop all sounds by clones when disposed --- src/sprites/rendered-target.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 293666acf..93e7d8c7f 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -1130,6 +1130,9 @@ class RenderedTarget extends Target { dispose () { this.runtime.changeCloneCounter(-1); this.runtime.stopForTarget(this); + if (this.sprite.soundBank) { + this.sprite.soundBank.stopAllSounds(this); + } this.sprite.removeClone(this); if (this.renderer && this.drawableID !== null) { this.renderer.destroyDrawable(this.drawableID, this.isStage ? From c268bbae47ce5e86c47894b2d813645820793590 Mon Sep 17 00:00:00 2001 From: "Michael \"Z\" Goddard" Date: Thu, 21 Jun 2018 17:23:33 -0400 Subject: [PATCH 14/22] add decoded SoundPlayer's to a Sprite's SoundBank (#1260) --- src/import/load-sound.js | 20 +++++++++++++------- src/serialization/sb3.js | 2 +- src/virtual-machine.js | 6 +++--- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/import/load-sound.js b/src/import/load-sound.js index e13db14d2..8f4f7a1a6 100644 --- a/src/import/load-sound.js +++ b/src/import/load-sound.js @@ -8,27 +8,32 @@ const log = require('../util/log'); * @property {Buffer} data - sound data will be written here once loaded. * @param {!Asset} soundAsset - the asset loaded from storage. * @param {!Runtime} runtime - Scratch runtime, used to access the storage module. + * @param {Sprite} sprite - Scratch sprite to add sounds to. * @returns {!Promise} - a promise which will resolve to the sound when ready. */ -const loadSoundFromAsset = function (sound, soundAsset, runtime) { +const loadSoundFromAsset = function (sound, soundAsset, runtime, sprite) { sound.assetId = soundAsset.assetId; if (!runtime.audioEngine) { log.error('No audio engine present; cannot load sound asset: ', sound.md5); return Promise.resolve(sound); } - return runtime.audioEngine.decodeSound(Object.assign( + return runtime.audioEngine.decodeSoundPlayer(Object.assign( {}, sound, {data: soundAsset.data} - )).then(soundId => { - sound.soundId = soundId; + )).then(soundPlayer => { + sound.soundId = soundPlayer.id; // Set the sound sample rate and sample count based on the // the audio buffer from the audio engine since the sound // gets resampled by the audio engine - const soundBuffer = runtime.audioEngine.getSoundBuffer(soundId); + const soundBuffer = soundPlayer.buffer; sound.rate = soundBuffer.sampleRate; sound.sampleCount = soundBuffer.length; + if (sprite.soundBank !== null) { + sprite.soundBank.addSoundPlayer(soundPlayer); + } + return sound; }); }; @@ -39,9 +44,10 @@ const loadSoundFromAsset = function (sound, soundAsset, runtime) { * @property {string} md5 - the MD5 and extension of the sound to be loaded. * @property {Buffer} data - sound data will be written here once loaded. * @param {!Runtime} runtime - Scratch runtime, used to access the storage module. + * @param {Sprite} sprite - Scratch sprite to add sounds to. * @returns {!Promise} - a promise which will resolve to the sound when ready. */ -const loadSound = function (sound, runtime) { +const loadSound = function (sound, runtime, sprite) { if (!runtime.storage) { log.error('No storage module present; cannot load sound asset: ', sound.md5); return Promise.resolve(sound); @@ -52,7 +58,7 @@ const loadSound = function (sound, runtime) { return runtime.storage.load(runtime.storage.AssetType.Sound, md5, ext) .then(soundAsset => { sound.dataFormat = ext; - return loadSoundFromAsset(sound, soundAsset, runtime); + return loadSoundFromAsset(sound, soundAsset, runtime, sprite); }); }; diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index 4f129c171..0ab76a15d 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -800,7 +800,7 @@ const parseScratchObject = function (object, runtime, extensions, zip) { // any translation that needs to happen will happen in the process // of building up the costume object into an sb3 format return deserializeSound(sound, runtime, zip) - .then(() => loadSound(sound, runtime)); + .then(() => loadSound(sound, runtime, sprite)); // Only attempt to load the sound after the deserialization // process has been completed. }); diff --git a/src/virtual-machine.js b/src/virtual-machine.js index 767641eec..9e93b79d2 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -525,7 +525,7 @@ class VirtualMachine extends EventEmitter { * @returns {?Promise} - a promise that resolves when the sound has been decoded and added */ addSound (soundObject) { - return loadSound(soundObject, this.runtime).then(() => { + return loadSound(soundObject, this.runtime, this.editingTarget.sprite).then(() => { this.editingTarget.addSound(soundObject); this.emitTargetsUpdate(); }); @@ -549,7 +549,7 @@ class VirtualMachine extends EventEmitter { getSoundBuffer (soundIndex) { const id = this.editingTarget.sprite.sounds[soundIndex].soundId; if (id && this.runtime && this.runtime.audioEngine) { - return this.runtime.audioEngine.getSoundBuffer(id); + return this.editingTarget.sprite.soundBank.getSoundPlayer(id).buffer; } return null; } @@ -564,7 +564,7 @@ class VirtualMachine extends EventEmitter { const sound = this.editingTarget.sprite.sounds[soundIndex]; const id = sound ? sound.soundId : null; if (id && this.runtime && this.runtime.audioEngine) { - this.runtime.audioEngine.updateSoundBuffer(id, newBuffer); + this.editingTarget.sprite.soundBank.getSoundPlayer(id).buffer = newBuffer; } // Update sound in runtime if (soundEncoding) { From ea3a23e37c36527490b5a93418ff6996750a03a0 Mon Sep 17 00:00:00 2001 From: Mx Corey Frang Date: Thu, 21 Jun 2018 17:53:45 -0400 Subject: [PATCH 15/22] Fix before_deploy script to properly quote (#1261) --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 76950271a..50ae66370 100644 --- a/.travis.yml +++ b/.travis.yml @@ -28,8 +28,8 @@ jobs: env: NPM_SCRIPT=build before_deploy: - npm --no-git-tag-version version $($(npm bin)/json -f package.json version)-prerelease.$(date +%s) - - git config --global user.email $(git log --pretty=format:"%ae" -n1) - - git config --global user.name $(git log --pretty=format:"%an" -n1) + - git config --global user.email "$(git log --pretty=format:"%ae" -n1)" + - git config --global user.name "$(git log --pretty=format:"%an" -n1)" deploy: - provider: npm on: From 28f90648b04b652d68bede05b1d1992ffc1dea92 Mon Sep 17 00:00:00 2001 From: Ray Schamp Date: Fri, 22 Jun 2018 09:27:23 -0400 Subject: [PATCH 16/22] Revert #1260, #1258, #1239 This reverts commits c268bbae47ce5e86c47894b2d813645820793590 61dacfc915bf7a6b385c20516579bd19c13016ec 30b8cb8eb1ca1c7af5d1afb079d7176c6cac8273 --- src/blocks/scratch3_sound.js | 47 ++++---- src/extensions/scratch3_music/index.js | 151 +++++++++++-------------- src/import/load-sound.js | 20 ++-- src/serialization/sb3.js | 2 +- src/sprites/rendered-target.js | 43 +++---- src/sprites/sprite.js | 14 +-- src/virtual-machine.js | 6 +- test/unit/blocks_sounds.js | 8 +- 8 files changed, 124 insertions(+), 167 deletions(-) diff --git a/src/blocks/scratch3_sound.js b/src/blocks/scratch3_sound.js index 16b041bf8..a599b314c 100644 --- a/src/blocks/scratch3_sound.js +++ b/src/blocks/scratch3_sound.js @@ -88,7 +88,6 @@ class Scratch3SoundBlocks { if (!soundState) { soundState = Clone.simple(Scratch3SoundBlocks.DEFAULT_SOUND_STATE); target.setCustomState(Scratch3SoundBlocks.STATE_KEY, soundState); - target.soundEffects = soundState.effects; } return soundState; } @@ -140,19 +139,20 @@ class Scratch3SoundBlocks { } playSound (args, util) { - // Don't return the promise, it's the only difference for AndWait - this.playSoundAndWait(args, util); + const index = this._getSoundIndex(args.SOUND_MENU, util); + if (index >= 0) { + const soundId = util.target.sprite.sounds[index].soundId; + if (util.target.audioPlayer === null) return; + util.target.audioPlayer.playSound(soundId); + } } playSoundAndWait (args, util) { const index = this._getSoundIndex(args.SOUND_MENU, util); if (index >= 0) { - const {target} = util; - const {sprite} = target; - const {soundId} = sprite.sounds[index]; - if (sprite.soundBank) { - return sprite.soundBank.playSound(target, soundId); - } + const soundId = util.target.sprite.sounds[index].soundId; + if (util.target.audioPlayer === null) return; + return util.target.audioPlayer.playSound(soundId); } } @@ -199,9 +199,8 @@ class Scratch3SoundBlocks { } _stopAllSoundsForTarget (target) { - if (target.sprite.soundBank) { - target.sprite.soundBank.stopAllSounds(target); - } + if (target.audioPlayer === null) return; + target.audioPlayer.stopAllSounds(); } setEffect (args, util) { @@ -225,19 +224,23 @@ class Scratch3SoundBlocks { soundState.effects[effect] = value; } - const {min, max} = Scratch3SoundBlocks.EFFECT_RANGE[effect]; - soundState.effects[effect] = MathUtil.clamp(soundState.effects[effect], min, max); + const effectRange = Scratch3SoundBlocks.EFFECT_RANGE[effect]; + soundState.effects[effect] = MathUtil.clamp(soundState.effects[effect], effectRange.min, effectRange.max); + + if (util.target.audioPlayer === null) return; + util.target.audioPlayer.setEffect(effect, soundState.effects[effect]); - this._syncEffectsForTarget(util.target); // Yield until the next tick. return Promise.resolve(); } _syncEffectsForTarget (target) { - if (!target || !target.sprite.soundBank) return; - target.soundEffects = this._getSoundState(target).effects; - - target.sprite.soundBank.setEffects(target); + if (!target || !target.audioPlayer) return; + const soundState = this._getSoundState(target); + for (const effect in soundState.effects) { + if (!soundState.effects.hasOwnProperty(effect)) continue; + target.audioPlayer.setEffect(effect, soundState.effects[effect]); + } } clearEffects (args, util) { @@ -250,7 +253,8 @@ class Scratch3SoundBlocks { if (!soundState.effects.hasOwnProperty(effect)) continue; soundState.effects[effect] = 0; } - this._syncEffectsForTarget(target); + if (target.audioPlayer === null) return; + target.audioPlayer.clearEffects(); } _clearEffectsForAllTargets () { @@ -274,7 +278,8 @@ class Scratch3SoundBlocks { _updateVolume (volume, util) { volume = MathUtil.clamp(volume, 0, 100); util.target.volume = volume; - this._syncEffectsForTarget(util.target); + if (util.target.audioPlayer === null) return; + util.target.audioPlayer.setVolume(util.target.volume); // Yield until the next tick. return Promise.resolve(); diff --git a/src/extensions/scratch3_music/index.js b/src/extensions/scratch3_music/index.js index 9099dc98a..852c9261a 100644 --- a/src/extensions/scratch3_music/index.js +++ b/src/extensions/scratch3_music/index.js @@ -52,25 +52,18 @@ class Scratch3MusicBlocks { this._concurrencyCounter = 0; /** - * An array of sound players, one for each drum sound. + * An array of audio buffers, one for each drum sound. * @type {Array} * @private */ - this._drumPlayers = []; + this._drumBuffers = []; /** - * An array of arrays of sound players. Each instrument has one or more audio players. + * An array of arrays of audio buffers. Each instrument has one or more audio buffers. * @type {Array[]} * @private */ - this._instrumentPlayerArrays = []; - - /** - * An array of arrays of sound players. Each instrument mya have an audio player for each playable note. - * @type {Array[]} - * @private - */ - this._instrumentPlayerNoteArrays = []; + this._instrumentBufferArrays = []; /** * An array of audio bufferSourceNodes. Each time you play an instrument or drum sound, @@ -94,15 +87,14 @@ class Scratch3MusicBlocks { const loadingPromises = []; this.DRUM_INFO.forEach((drumInfo, index) => { const filePath = `drums/${drumInfo.fileName}`; - const promise = this._storeSound(filePath, index, this._drumPlayers); + const promise = this._storeSound(filePath, index, this._drumBuffers); loadingPromises.push(promise); }); this.INSTRUMENT_INFO.forEach((instrumentInfo, instrumentIndex) => { - this._instrumentPlayerArrays[instrumentIndex] = []; - this._instrumentPlayerNoteArrays[instrumentIndex] = []; + this._instrumentBufferArrays[instrumentIndex] = []; instrumentInfo.samples.forEach((sample, noteIndex) => { const filePath = `instruments/${instrumentInfo.dirName}/${sample}`; - const promise = this._storeSound(filePath, noteIndex, this._instrumentPlayerArrays[instrumentIndex]); + const promise = this._storeSound(filePath, noteIndex, this._instrumentBufferArrays[instrumentIndex]); loadingPromises.push(promise); }); }); @@ -112,22 +104,22 @@ class Scratch3MusicBlocks { } /** - * Decode a sound and store the player in an array. + * Decode a sound and store the buffer in an array. * @param {string} filePath - the audio file name. - * @param {number} index - the index at which to store the audio player. - * @param {array} playerArray - the array of players in which to store it. + * @param {number} index - the index at which to store the audio buffer. + * @param {array} bufferArray - the array of buffers in which to store it. * @return {Promise} - a promise which will resolve once the sound has been stored. */ - _storeSound (filePath, index, playerArray) { + _storeSound (filePath, index, bufferArray) { const fullPath = `${filePath}.mp3`; if (!assetData[fullPath]) return; - // The sound player has already been downloaded via the manifest file required above. + // The sound buffer has already been downloaded via the manifest file required above. const soundBuffer = assetData[fullPath]; - return this._decodeSound(soundBuffer).then(player => { - playerArray[index] = player; + return this._decodeSound(soundBuffer).then(buffer => { + bufferArray[index] = buffer; }); } @@ -137,14 +129,24 @@ class Scratch3MusicBlocks { * @return {Promise} - a promise which will resolve once the sound has decoded. */ _decodeSound (soundBuffer) { - const engine = this.runtime.audioEngine; + const context = this.runtime.audioEngine && this.runtime.audioEngine.audioContext; - if (!engine) { + if (!context) { return Promise.reject(new Error('No Audio Context Detected')); } // Check for newer promise-based API - return engine.decodeSoundPlayer({data: {buffer: soundBuffer}}); + if (context.decodeAudioData.length === 1) { + return context.decodeAudioData(soundBuffer); + } else { // eslint-disable-line no-else-return + // Fall back to callback API + return new Promise((resolve, reject) => + context.decodeAudioData(soundBuffer, + buffer => resolve(buffer), + error => reject(error) + ) + ); + } } /** @@ -776,34 +778,26 @@ class Scratch3MusicBlocks { */ _playDrumNum (util, drumNum) { if (util.runtime.audioEngine === null) return; - if (util.target.sprite.soundBank === null) return; + if (util.target.audioPlayer === null) return; // If we're playing too many sounds, do not play the drum sound. if (this._concurrencyCounter > Scratch3MusicBlocks.CONCURRENCY_LIMIT) { return; } + const outputNode = util.target.audioPlayer.getInputNode(); + const context = util.runtime.audioEngine.audioContext; + const bufferSource = context.createBufferSource(); + bufferSource.buffer = this._drumBuffers[drumNum]; + bufferSource.connect(outputNode); + bufferSource.start(); - const player = this._drumPlayers[drumNum]; - - if (typeof player === 'undefined') return; - - if (player.isPlaying) { - // Take the internal player state and create a new player with it. - // `.play` does this internally but then instructs the sound to - // stop. - player.take(); - } - - const engine = util.runtime.audioEngine; - const chain = engine.createEffectChain(); - chain.setEffectsFromTarget(util.target); - player.connect(chain); + const bufferSourceIndex = this._bufferSources.length; + this._bufferSources.push(bufferSource); this._concurrencyCounter++; - player.once('stop', () => { + bufferSource.onended = () => { this._concurrencyCounter--; - }); - - player.play(); + delete this._bufferSources[bufferSourceIndex]; + }; } /** @@ -862,7 +856,7 @@ class Scratch3MusicBlocks { */ _playNote (util, note, durationSec) { if (util.runtime.audioEngine === null) return; - if (util.target.sprite.soundBank === null) return; + if (util.target.audioPlayer === null) return; // If we're playing too many sounds, do not play the note. if (this._concurrencyCounter > Scratch3MusicBlocks.CONCURRENCY_LIMIT) { @@ -877,37 +871,28 @@ class Scratch3MusicBlocks { const sampleIndex = this._selectSampleIndexForNote(note, sampleArray); // If the audio sample has not loaded yet, bail out - if (typeof this._instrumentPlayerArrays[inst] === 'undefined') return; - if (typeof this._instrumentPlayerArrays[inst][sampleIndex] === 'undefined') return; + if (typeof this._instrumentBufferArrays[inst] === 'undefined') return; + if (typeof this._instrumentBufferArrays[inst][sampleIndex] === 'undefined') return; - // Fetch the sound player to play the note. - const engine = util.runtime.audioEngine; + // Create the audio buffer to play the note, and set its pitch + const context = util.runtime.audioEngine.audioContext; + const bufferSource = context.createBufferSource(); - if (!this._instrumentPlayerNoteArrays[inst][note]) { - this._instrumentPlayerNoteArrays[inst][note] = this._instrumentPlayerArrays[inst][sampleIndex].take(); - } + const bufferSourceIndex = this._bufferSources.length; + this._bufferSources.push(bufferSource); - const player = this._instrumentPlayerNoteArrays[inst][note]; - - if (player.isPlaying) { - // Take the internal player state and create a new player with it. - // `.play` does this internally but then instructs the sound to - // stop. - player.take(); - } - - const chain = engine.createEffectChain(); - chain.setEffectsFromTarget(util.target); - - // Set its pitch. + bufferSource.buffer = this._instrumentBufferArrays[inst][sampleIndex]; const sampleNote = sampleArray[sampleIndex]; - const notePitchInterval = this._ratioForPitchInterval(note - sampleNote); + bufferSource.playbackRate.value = this._ratioForPitchInterval(note - sampleNote); - // Create a gain node for this note, and connect it to the sprite's - // simulated effectChain. - const context = engine.audioContext; - const releaseGain = context.createGain(); - releaseGain.connect(chain.getInputNode()); + // Create a gain node for this note, and connect it to the sprite's audioPlayer. + const gainNode = context.createGain(); + bufferSource.connect(gainNode); + const outputNode = util.target.audioPlayer.getInputNode(); + gainNode.connect(outputNode); + + // Start playing the note + bufferSource.start(); // Schedule the release of the note, ramping its gain down to zero, // and then stopping the sound. @@ -917,24 +902,16 @@ class Scratch3MusicBlocks { } const releaseStart = context.currentTime + durationSec; const releaseEnd = releaseStart + releaseDuration; - releaseGain.gain.setValueAtTime(1, releaseStart); - releaseGain.gain.linearRampToValueAtTime(0.0001, releaseEnd); + gainNode.gain.setValueAtTime(1, releaseStart); + gainNode.gain.linearRampToValueAtTime(0.0001, releaseEnd); + bufferSource.stop(releaseEnd); + // Update the concurrency counter this._concurrencyCounter++; - player.once('stop', () => { + bufferSource.onended = () => { this._concurrencyCounter--; - }); - - // Start playing the note - player.play(); - // Connect the player to the gain node. - player.connect({getInputNode () { - return releaseGain; - }}); - // Set playback now after play creates the outputNode. - player.outputNode.playbackRate.value = notePitchInterval; - // Schedule playback to stop. - player.outputNode.stop(releaseEnd); + delete this._bufferSources[bufferSourceIndex]; + }; } /** diff --git a/src/import/load-sound.js b/src/import/load-sound.js index 8f4f7a1a6..e13db14d2 100644 --- a/src/import/load-sound.js +++ b/src/import/load-sound.js @@ -8,32 +8,27 @@ const log = require('../util/log'); * @property {Buffer} data - sound data will be written here once loaded. * @param {!Asset} soundAsset - the asset loaded from storage. * @param {!Runtime} runtime - Scratch runtime, used to access the storage module. - * @param {Sprite} sprite - Scratch sprite to add sounds to. * @returns {!Promise} - a promise which will resolve to the sound when ready. */ -const loadSoundFromAsset = function (sound, soundAsset, runtime, sprite) { +const loadSoundFromAsset = function (sound, soundAsset, runtime) { sound.assetId = soundAsset.assetId; if (!runtime.audioEngine) { log.error('No audio engine present; cannot load sound asset: ', sound.md5); return Promise.resolve(sound); } - return runtime.audioEngine.decodeSoundPlayer(Object.assign( + return runtime.audioEngine.decodeSound(Object.assign( {}, sound, {data: soundAsset.data} - )).then(soundPlayer => { - sound.soundId = soundPlayer.id; + )).then(soundId => { + sound.soundId = soundId; // Set the sound sample rate and sample count based on the // the audio buffer from the audio engine since the sound // gets resampled by the audio engine - const soundBuffer = soundPlayer.buffer; + const soundBuffer = runtime.audioEngine.getSoundBuffer(soundId); sound.rate = soundBuffer.sampleRate; sound.sampleCount = soundBuffer.length; - if (sprite.soundBank !== null) { - sprite.soundBank.addSoundPlayer(soundPlayer); - } - return sound; }); }; @@ -44,10 +39,9 @@ const loadSoundFromAsset = function (sound, soundAsset, runtime, sprite) { * @property {string} md5 - the MD5 and extension of the sound to be loaded. * @property {Buffer} data - sound data will be written here once loaded. * @param {!Runtime} runtime - Scratch runtime, used to access the storage module. - * @param {Sprite} sprite - Scratch sprite to add sounds to. * @returns {!Promise} - a promise which will resolve to the sound when ready. */ -const loadSound = function (sound, runtime, sprite) { +const loadSound = function (sound, runtime) { if (!runtime.storage) { log.error('No storage module present; cannot load sound asset: ', sound.md5); return Promise.resolve(sound); @@ -58,7 +52,7 @@ const loadSound = function (sound, runtime, sprite) { return runtime.storage.load(runtime.storage.AssetType.Sound, md5, ext) .then(soundAsset => { sound.dataFormat = ext; - return loadSoundFromAsset(sound, soundAsset, runtime, sprite); + return loadSoundFromAsset(sound, soundAsset, runtime); }); }; diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index 0ab76a15d..4f129c171 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -800,7 +800,7 @@ const parseScratchObject = function (object, runtime, extensions, zip) { // any translation that needs to happen will happen in the process // of building up the costume object into an sb3 format return deserializeSound(sound, runtime, zip) - .then(() => loadSound(sound, runtime, sprite)); + .then(() => loadSound(sound, runtime)); // Only attempt to load the sound after the deserialization // process has been completed. }); diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 93e7d8c7f..aac0b059a 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -170,30 +170,21 @@ class RenderedTarget extends Target { } } - get audioPlayer () { - /* eslint-disable no-console */ - console.warn('get audioPlayer deprecated, please update to use .sprite.soundBank methods'); - console.warn(new Error('stack for debug').stack); - /* eslint-enable no-console */ - const bank = this.sprite.soundBank; - const audioPlayerProxy = { - playSound: soundId => bank.play(this, soundId) - }; - - Object.defineProperty(this, 'audioPlayer', { - configurable: false, - enumerable: true, - writable: false, - value: audioPlayerProxy - }); - - return audioPlayerProxy; - } - /** * Initialize the audio player for this sprite or clone. */ initAudio () { + this.audioPlayer = null; + if (this.runtime && this.runtime.audioEngine) { + this.audioPlayer = this.runtime.audioEngine.createPlayer(); + // If this is a clone, it gets a reference to its parent's activeSoundPlayers object. + if (!this.isOriginal) { + const parent = this.sprite.clones[0]; + if (parent && parent.audioPlayer) { + this.audioPlayer.activeSoundPlayers = parent.audioPlayer.activeSoundPlayers; + } + } + } } /** @@ -1043,8 +1034,9 @@ class RenderedTarget extends Target { */ onStopAll () { this.clearEffects(); - if (this.sprite.soundBank) { - this.sprite.soundBank.stopAllSounds(this); + if (this.audioPlayer) { + this.audioPlayer.stopAllSounds(); + this.audioPlayer.clearEffects(); } } @@ -1130,9 +1122,6 @@ class RenderedTarget extends Target { dispose () { this.runtime.changeCloneCounter(-1); this.runtime.stopForTarget(this); - if (this.sprite.soundBank) { - this.sprite.soundBank.stopAllSounds(this); - } this.sprite.removeClone(this); if (this.renderer && this.drawableID !== null) { this.renderer.destroyDrawable(this.drawableID, this.isStage ? @@ -1143,6 +1132,10 @@ class RenderedTarget extends Target { this.runtime.requestRedraw(); } } + if (this.audioPlayer) { + this.audioPlayer.stopAllSounds(); + this.audioPlayer.dispose(); + } } } diff --git a/src/sprites/sprite.js b/src/sprites/sprite.js index c7c35c9e0..de735ecc3 100644 --- a/src/sprites/sprite.js +++ b/src/sprites/sprite.js @@ -8,8 +8,7 @@ const StageLayering = require('../engine/stage-layering'); class Sprite { /** * Sprite to be used on the Scratch stage. - * All clones of a sprite have shared blocks, shared costumes, shared variables, - * shared sounds, etc. + * All clones of a sprite have shared blocks, shared costumes, shared variables. * @param {?Blocks} blocks Shared blocks object for all clones of sprite. * @param {Runtime} runtime Reference to the runtime. * @constructor @@ -48,11 +47,6 @@ class Sprite { * @type {Array.} */ this.clones = []; - - this.soundBank = null; - if (this.runtime && this.runtime.audioEngine) { - this.soundBank = this.runtime.audioEngine.createBank(); - } } /** @@ -161,12 +155,6 @@ class Sprite { return Promise.all(assetPromises).then(() => newSprite); } - - dispose () { - if (this.soundBank) { - this.soundBank.dispose(); - } - } } module.exports = Sprite; diff --git a/src/virtual-machine.js b/src/virtual-machine.js index 9e93b79d2..767641eec 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -525,7 +525,7 @@ class VirtualMachine extends EventEmitter { * @returns {?Promise} - a promise that resolves when the sound has been decoded and added */ addSound (soundObject) { - return loadSound(soundObject, this.runtime, this.editingTarget.sprite).then(() => { + return loadSound(soundObject, this.runtime).then(() => { this.editingTarget.addSound(soundObject); this.emitTargetsUpdate(); }); @@ -549,7 +549,7 @@ class VirtualMachine extends EventEmitter { getSoundBuffer (soundIndex) { const id = this.editingTarget.sprite.sounds[soundIndex].soundId; if (id && this.runtime && this.runtime.audioEngine) { - return this.editingTarget.sprite.soundBank.getSoundPlayer(id).buffer; + return this.runtime.audioEngine.getSoundBuffer(id); } return null; } @@ -564,7 +564,7 @@ class VirtualMachine extends EventEmitter { const sound = this.editingTarget.sprite.sounds[soundIndex]; const id = sound ? sound.soundId : null; if (id && this.runtime && this.runtime.audioEngine) { - this.editingTarget.sprite.soundBank.getSoundPlayer(id).buffer = newBuffer; + this.runtime.audioEngine.updateSoundBuffer(id, newBuffer); } // Update sound in runtime if (soundEncoding) { diff --git a/test/unit/blocks_sounds.js b/test/unit/blocks_sounds.js index 53acc8aff..9a44e7f9f 100644 --- a/test/unit/blocks_sounds.js +++ b/test/unit/blocks_sounds.js @@ -11,10 +11,10 @@ const util = { {name: 'second name', soundId: 'second soundId'}, {name: 'third name', soundId: 'third soundId'}, {name: '6', soundId: 'fourth soundId'} - ], - soundBank: { - playSound: (target, soundId) => (playedSound = soundId) - } + ] + }, + audioPlayer: { + playSound: soundId => (playedSound = soundId) } } }; From 9afe401dcafcb199d1719228137fc276600684c7 Mon Sep 17 00:00:00 2001 From: Ray Schamp Date: Fri, 22 Jun 2018 09:33:08 -0400 Subject: [PATCH 17/22] Revert "Revert #1260, #1258, #1239" This reverts commit 28f90648b04b652d68bede05b1d1992ffc1dea92. --- src/blocks/scratch3_sound.js | 47 ++++---- src/extensions/scratch3_music/index.js | 151 ++++++++++++++----------- src/import/load-sound.js | 20 ++-- src/serialization/sb3.js | 2 +- src/sprites/rendered-target.js | 43 ++++--- src/sprites/sprite.js | 14 ++- src/virtual-machine.js | 6 +- test/unit/blocks_sounds.js | 8 +- 8 files changed, 167 insertions(+), 124 deletions(-) diff --git a/src/blocks/scratch3_sound.js b/src/blocks/scratch3_sound.js index a599b314c..16b041bf8 100644 --- a/src/blocks/scratch3_sound.js +++ b/src/blocks/scratch3_sound.js @@ -88,6 +88,7 @@ class Scratch3SoundBlocks { if (!soundState) { soundState = Clone.simple(Scratch3SoundBlocks.DEFAULT_SOUND_STATE); target.setCustomState(Scratch3SoundBlocks.STATE_KEY, soundState); + target.soundEffects = soundState.effects; } return soundState; } @@ -139,20 +140,19 @@ class Scratch3SoundBlocks { } playSound (args, util) { - const index = this._getSoundIndex(args.SOUND_MENU, util); - if (index >= 0) { - const soundId = util.target.sprite.sounds[index].soundId; - if (util.target.audioPlayer === null) return; - util.target.audioPlayer.playSound(soundId); - } + // Don't return the promise, it's the only difference for AndWait + this.playSoundAndWait(args, util); } playSoundAndWait (args, util) { const index = this._getSoundIndex(args.SOUND_MENU, util); if (index >= 0) { - const soundId = util.target.sprite.sounds[index].soundId; - if (util.target.audioPlayer === null) return; - return util.target.audioPlayer.playSound(soundId); + const {target} = util; + const {sprite} = target; + const {soundId} = sprite.sounds[index]; + if (sprite.soundBank) { + return sprite.soundBank.playSound(target, soundId); + } } } @@ -199,8 +199,9 @@ class Scratch3SoundBlocks { } _stopAllSoundsForTarget (target) { - if (target.audioPlayer === null) return; - target.audioPlayer.stopAllSounds(); + if (target.sprite.soundBank) { + target.sprite.soundBank.stopAllSounds(target); + } } setEffect (args, util) { @@ -224,23 +225,19 @@ class Scratch3SoundBlocks { soundState.effects[effect] = value; } - const effectRange = Scratch3SoundBlocks.EFFECT_RANGE[effect]; - soundState.effects[effect] = MathUtil.clamp(soundState.effects[effect], effectRange.min, effectRange.max); - - if (util.target.audioPlayer === null) return; - util.target.audioPlayer.setEffect(effect, soundState.effects[effect]); + const {min, max} = Scratch3SoundBlocks.EFFECT_RANGE[effect]; + soundState.effects[effect] = MathUtil.clamp(soundState.effects[effect], min, max); + this._syncEffectsForTarget(util.target); // Yield until the next tick. return Promise.resolve(); } _syncEffectsForTarget (target) { - if (!target || !target.audioPlayer) return; - const soundState = this._getSoundState(target); - for (const effect in soundState.effects) { - if (!soundState.effects.hasOwnProperty(effect)) continue; - target.audioPlayer.setEffect(effect, soundState.effects[effect]); - } + if (!target || !target.sprite.soundBank) return; + target.soundEffects = this._getSoundState(target).effects; + + target.sprite.soundBank.setEffects(target); } clearEffects (args, util) { @@ -253,8 +250,7 @@ class Scratch3SoundBlocks { if (!soundState.effects.hasOwnProperty(effect)) continue; soundState.effects[effect] = 0; } - if (target.audioPlayer === null) return; - target.audioPlayer.clearEffects(); + this._syncEffectsForTarget(target); } _clearEffectsForAllTargets () { @@ -278,8 +274,7 @@ class Scratch3SoundBlocks { _updateVolume (volume, util) { volume = MathUtil.clamp(volume, 0, 100); util.target.volume = volume; - if (util.target.audioPlayer === null) return; - util.target.audioPlayer.setVolume(util.target.volume); + this._syncEffectsForTarget(util.target); // Yield until the next tick. return Promise.resolve(); diff --git a/src/extensions/scratch3_music/index.js b/src/extensions/scratch3_music/index.js index 852c9261a..9099dc98a 100644 --- a/src/extensions/scratch3_music/index.js +++ b/src/extensions/scratch3_music/index.js @@ -52,18 +52,25 @@ class Scratch3MusicBlocks { this._concurrencyCounter = 0; /** - * An array of audio buffers, one for each drum sound. + * An array of sound players, one for each drum sound. * @type {Array} * @private */ - this._drumBuffers = []; + this._drumPlayers = []; /** - * An array of arrays of audio buffers. Each instrument has one or more audio buffers. + * An array of arrays of sound players. Each instrument has one or more audio players. * @type {Array[]} * @private */ - this._instrumentBufferArrays = []; + this._instrumentPlayerArrays = []; + + /** + * An array of arrays of sound players. Each instrument mya have an audio player for each playable note. + * @type {Array[]} + * @private + */ + this._instrumentPlayerNoteArrays = []; /** * An array of audio bufferSourceNodes. Each time you play an instrument or drum sound, @@ -87,14 +94,15 @@ class Scratch3MusicBlocks { const loadingPromises = []; this.DRUM_INFO.forEach((drumInfo, index) => { const filePath = `drums/${drumInfo.fileName}`; - const promise = this._storeSound(filePath, index, this._drumBuffers); + const promise = this._storeSound(filePath, index, this._drumPlayers); loadingPromises.push(promise); }); this.INSTRUMENT_INFO.forEach((instrumentInfo, instrumentIndex) => { - this._instrumentBufferArrays[instrumentIndex] = []; + this._instrumentPlayerArrays[instrumentIndex] = []; + this._instrumentPlayerNoteArrays[instrumentIndex] = []; instrumentInfo.samples.forEach((sample, noteIndex) => { const filePath = `instruments/${instrumentInfo.dirName}/${sample}`; - const promise = this._storeSound(filePath, noteIndex, this._instrumentBufferArrays[instrumentIndex]); + const promise = this._storeSound(filePath, noteIndex, this._instrumentPlayerArrays[instrumentIndex]); loadingPromises.push(promise); }); }); @@ -104,22 +112,22 @@ class Scratch3MusicBlocks { } /** - * Decode a sound and store the buffer in an array. + * Decode a sound and store the player in an array. * @param {string} filePath - the audio file name. - * @param {number} index - the index at which to store the audio buffer. - * @param {array} bufferArray - the array of buffers in which to store it. + * @param {number} index - the index at which to store the audio player. + * @param {array} playerArray - the array of players in which to store it. * @return {Promise} - a promise which will resolve once the sound has been stored. */ - _storeSound (filePath, index, bufferArray) { + _storeSound (filePath, index, playerArray) { const fullPath = `${filePath}.mp3`; if (!assetData[fullPath]) return; - // The sound buffer has already been downloaded via the manifest file required above. + // The sound player has already been downloaded via the manifest file required above. const soundBuffer = assetData[fullPath]; - return this._decodeSound(soundBuffer).then(buffer => { - bufferArray[index] = buffer; + return this._decodeSound(soundBuffer).then(player => { + playerArray[index] = player; }); } @@ -129,24 +137,14 @@ class Scratch3MusicBlocks { * @return {Promise} - a promise which will resolve once the sound has decoded. */ _decodeSound (soundBuffer) { - const context = this.runtime.audioEngine && this.runtime.audioEngine.audioContext; + const engine = this.runtime.audioEngine; - if (!context) { + if (!engine) { return Promise.reject(new Error('No Audio Context Detected')); } // Check for newer promise-based API - if (context.decodeAudioData.length === 1) { - return context.decodeAudioData(soundBuffer); - } else { // eslint-disable-line no-else-return - // Fall back to callback API - return new Promise((resolve, reject) => - context.decodeAudioData(soundBuffer, - buffer => resolve(buffer), - error => reject(error) - ) - ); - } + return engine.decodeSoundPlayer({data: {buffer: soundBuffer}}); } /** @@ -778,26 +776,34 @@ class Scratch3MusicBlocks { */ _playDrumNum (util, drumNum) { if (util.runtime.audioEngine === null) return; - if (util.target.audioPlayer === null) return; + if (util.target.sprite.soundBank === null) return; // If we're playing too many sounds, do not play the drum sound. if (this._concurrencyCounter > Scratch3MusicBlocks.CONCURRENCY_LIMIT) { return; } - const outputNode = util.target.audioPlayer.getInputNode(); - const context = util.runtime.audioEngine.audioContext; - const bufferSource = context.createBufferSource(); - bufferSource.buffer = this._drumBuffers[drumNum]; - bufferSource.connect(outputNode); - bufferSource.start(); - const bufferSourceIndex = this._bufferSources.length; - this._bufferSources.push(bufferSource); + const player = this._drumPlayers[drumNum]; + + if (typeof player === 'undefined') return; + + if (player.isPlaying) { + // Take the internal player state and create a new player with it. + // `.play` does this internally but then instructs the sound to + // stop. + player.take(); + } + + const engine = util.runtime.audioEngine; + const chain = engine.createEffectChain(); + chain.setEffectsFromTarget(util.target); + player.connect(chain); this._concurrencyCounter++; - bufferSource.onended = () => { + player.once('stop', () => { this._concurrencyCounter--; - delete this._bufferSources[bufferSourceIndex]; - }; + }); + + player.play(); } /** @@ -856,7 +862,7 @@ class Scratch3MusicBlocks { */ _playNote (util, note, durationSec) { if (util.runtime.audioEngine === null) return; - if (util.target.audioPlayer === null) return; + if (util.target.sprite.soundBank === null) return; // If we're playing too many sounds, do not play the note. if (this._concurrencyCounter > Scratch3MusicBlocks.CONCURRENCY_LIMIT) { @@ -871,28 +877,37 @@ class Scratch3MusicBlocks { const sampleIndex = this._selectSampleIndexForNote(note, sampleArray); // If the audio sample has not loaded yet, bail out - if (typeof this._instrumentBufferArrays[inst] === 'undefined') return; - if (typeof this._instrumentBufferArrays[inst][sampleIndex] === 'undefined') return; + if (typeof this._instrumentPlayerArrays[inst] === 'undefined') return; + if (typeof this._instrumentPlayerArrays[inst][sampleIndex] === 'undefined') return; - // Create the audio buffer to play the note, and set its pitch - const context = util.runtime.audioEngine.audioContext; - const bufferSource = context.createBufferSource(); + // Fetch the sound player to play the note. + const engine = util.runtime.audioEngine; - const bufferSourceIndex = this._bufferSources.length; - this._bufferSources.push(bufferSource); + if (!this._instrumentPlayerNoteArrays[inst][note]) { + this._instrumentPlayerNoteArrays[inst][note] = this._instrumentPlayerArrays[inst][sampleIndex].take(); + } - bufferSource.buffer = this._instrumentBufferArrays[inst][sampleIndex]; + const player = this._instrumentPlayerNoteArrays[inst][note]; + + if (player.isPlaying) { + // Take the internal player state and create a new player with it. + // `.play` does this internally but then instructs the sound to + // stop. + player.take(); + } + + const chain = engine.createEffectChain(); + chain.setEffectsFromTarget(util.target); + + // Set its pitch. const sampleNote = sampleArray[sampleIndex]; - bufferSource.playbackRate.value = this._ratioForPitchInterval(note - sampleNote); + const notePitchInterval = this._ratioForPitchInterval(note - sampleNote); - // Create a gain node for this note, and connect it to the sprite's audioPlayer. - const gainNode = context.createGain(); - bufferSource.connect(gainNode); - const outputNode = util.target.audioPlayer.getInputNode(); - gainNode.connect(outputNode); - - // Start playing the note - bufferSource.start(); + // Create a gain node for this note, and connect it to the sprite's + // simulated effectChain. + const context = engine.audioContext; + const releaseGain = context.createGain(); + releaseGain.connect(chain.getInputNode()); // Schedule the release of the note, ramping its gain down to zero, // and then stopping the sound. @@ -902,16 +917,24 @@ class Scratch3MusicBlocks { } const releaseStart = context.currentTime + durationSec; const releaseEnd = releaseStart + releaseDuration; - gainNode.gain.setValueAtTime(1, releaseStart); - gainNode.gain.linearRampToValueAtTime(0.0001, releaseEnd); - bufferSource.stop(releaseEnd); + releaseGain.gain.setValueAtTime(1, releaseStart); + releaseGain.gain.linearRampToValueAtTime(0.0001, releaseEnd); - // Update the concurrency counter this._concurrencyCounter++; - bufferSource.onended = () => { + player.once('stop', () => { this._concurrencyCounter--; - delete this._bufferSources[bufferSourceIndex]; - }; + }); + + // Start playing the note + player.play(); + // Connect the player to the gain node. + player.connect({getInputNode () { + return releaseGain; + }}); + // Set playback now after play creates the outputNode. + player.outputNode.playbackRate.value = notePitchInterval; + // Schedule playback to stop. + player.outputNode.stop(releaseEnd); } /** diff --git a/src/import/load-sound.js b/src/import/load-sound.js index e13db14d2..8f4f7a1a6 100644 --- a/src/import/load-sound.js +++ b/src/import/load-sound.js @@ -8,27 +8,32 @@ const log = require('../util/log'); * @property {Buffer} data - sound data will be written here once loaded. * @param {!Asset} soundAsset - the asset loaded from storage. * @param {!Runtime} runtime - Scratch runtime, used to access the storage module. + * @param {Sprite} sprite - Scratch sprite to add sounds to. * @returns {!Promise} - a promise which will resolve to the sound when ready. */ -const loadSoundFromAsset = function (sound, soundAsset, runtime) { +const loadSoundFromAsset = function (sound, soundAsset, runtime, sprite) { sound.assetId = soundAsset.assetId; if (!runtime.audioEngine) { log.error('No audio engine present; cannot load sound asset: ', sound.md5); return Promise.resolve(sound); } - return runtime.audioEngine.decodeSound(Object.assign( + return runtime.audioEngine.decodeSoundPlayer(Object.assign( {}, sound, {data: soundAsset.data} - )).then(soundId => { - sound.soundId = soundId; + )).then(soundPlayer => { + sound.soundId = soundPlayer.id; // Set the sound sample rate and sample count based on the // the audio buffer from the audio engine since the sound // gets resampled by the audio engine - const soundBuffer = runtime.audioEngine.getSoundBuffer(soundId); + const soundBuffer = soundPlayer.buffer; sound.rate = soundBuffer.sampleRate; sound.sampleCount = soundBuffer.length; + if (sprite.soundBank !== null) { + sprite.soundBank.addSoundPlayer(soundPlayer); + } + return sound; }); }; @@ -39,9 +44,10 @@ const loadSoundFromAsset = function (sound, soundAsset, runtime) { * @property {string} md5 - the MD5 and extension of the sound to be loaded. * @property {Buffer} data - sound data will be written here once loaded. * @param {!Runtime} runtime - Scratch runtime, used to access the storage module. + * @param {Sprite} sprite - Scratch sprite to add sounds to. * @returns {!Promise} - a promise which will resolve to the sound when ready. */ -const loadSound = function (sound, runtime) { +const loadSound = function (sound, runtime, sprite) { if (!runtime.storage) { log.error('No storage module present; cannot load sound asset: ', sound.md5); return Promise.resolve(sound); @@ -52,7 +58,7 @@ const loadSound = function (sound, runtime) { return runtime.storage.load(runtime.storage.AssetType.Sound, md5, ext) .then(soundAsset => { sound.dataFormat = ext; - return loadSoundFromAsset(sound, soundAsset, runtime); + return loadSoundFromAsset(sound, soundAsset, runtime, sprite); }); }; diff --git a/src/serialization/sb3.js b/src/serialization/sb3.js index 4f129c171..0ab76a15d 100644 --- a/src/serialization/sb3.js +++ b/src/serialization/sb3.js @@ -800,7 +800,7 @@ const parseScratchObject = function (object, runtime, extensions, zip) { // any translation that needs to happen will happen in the process // of building up the costume object into an sb3 format return deserializeSound(sound, runtime, zip) - .then(() => loadSound(sound, runtime)); + .then(() => loadSound(sound, runtime, sprite)); // Only attempt to load the sound after the deserialization // process has been completed. }); diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index aac0b059a..93e7d8c7f 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -170,21 +170,30 @@ class RenderedTarget extends Target { } } + get audioPlayer () { + /* eslint-disable no-console */ + console.warn('get audioPlayer deprecated, please update to use .sprite.soundBank methods'); + console.warn(new Error('stack for debug').stack); + /* eslint-enable no-console */ + const bank = this.sprite.soundBank; + const audioPlayerProxy = { + playSound: soundId => bank.play(this, soundId) + }; + + Object.defineProperty(this, 'audioPlayer', { + configurable: false, + enumerable: true, + writable: false, + value: audioPlayerProxy + }); + + return audioPlayerProxy; + } + /** * Initialize the audio player for this sprite or clone. */ initAudio () { - this.audioPlayer = null; - if (this.runtime && this.runtime.audioEngine) { - this.audioPlayer = this.runtime.audioEngine.createPlayer(); - // If this is a clone, it gets a reference to its parent's activeSoundPlayers object. - if (!this.isOriginal) { - const parent = this.sprite.clones[0]; - if (parent && parent.audioPlayer) { - this.audioPlayer.activeSoundPlayers = parent.audioPlayer.activeSoundPlayers; - } - } - } } /** @@ -1034,9 +1043,8 @@ class RenderedTarget extends Target { */ onStopAll () { this.clearEffects(); - if (this.audioPlayer) { - this.audioPlayer.stopAllSounds(); - this.audioPlayer.clearEffects(); + if (this.sprite.soundBank) { + this.sprite.soundBank.stopAllSounds(this); } } @@ -1122,6 +1130,9 @@ class RenderedTarget extends Target { dispose () { this.runtime.changeCloneCounter(-1); this.runtime.stopForTarget(this); + if (this.sprite.soundBank) { + this.sprite.soundBank.stopAllSounds(this); + } this.sprite.removeClone(this); if (this.renderer && this.drawableID !== null) { this.renderer.destroyDrawable(this.drawableID, this.isStage ? @@ -1132,10 +1143,6 @@ class RenderedTarget extends Target { this.runtime.requestRedraw(); } } - if (this.audioPlayer) { - this.audioPlayer.stopAllSounds(); - this.audioPlayer.dispose(); - } } } diff --git a/src/sprites/sprite.js b/src/sprites/sprite.js index de735ecc3..c7c35c9e0 100644 --- a/src/sprites/sprite.js +++ b/src/sprites/sprite.js @@ -8,7 +8,8 @@ const StageLayering = require('../engine/stage-layering'); class Sprite { /** * Sprite to be used on the Scratch stage. - * All clones of a sprite have shared blocks, shared costumes, shared variables. + * All clones of a sprite have shared blocks, shared costumes, shared variables, + * shared sounds, etc. * @param {?Blocks} blocks Shared blocks object for all clones of sprite. * @param {Runtime} runtime Reference to the runtime. * @constructor @@ -47,6 +48,11 @@ class Sprite { * @type {Array.} */ this.clones = []; + + this.soundBank = null; + if (this.runtime && this.runtime.audioEngine) { + this.soundBank = this.runtime.audioEngine.createBank(); + } } /** @@ -155,6 +161,12 @@ class Sprite { return Promise.all(assetPromises).then(() => newSprite); } + + dispose () { + if (this.soundBank) { + this.soundBank.dispose(); + } + } } module.exports = Sprite; diff --git a/src/virtual-machine.js b/src/virtual-machine.js index 767641eec..9e93b79d2 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -525,7 +525,7 @@ class VirtualMachine extends EventEmitter { * @returns {?Promise} - a promise that resolves when the sound has been decoded and added */ addSound (soundObject) { - return loadSound(soundObject, this.runtime).then(() => { + return loadSound(soundObject, this.runtime, this.editingTarget.sprite).then(() => { this.editingTarget.addSound(soundObject); this.emitTargetsUpdate(); }); @@ -549,7 +549,7 @@ class VirtualMachine extends EventEmitter { getSoundBuffer (soundIndex) { const id = this.editingTarget.sprite.sounds[soundIndex].soundId; if (id && this.runtime && this.runtime.audioEngine) { - return this.runtime.audioEngine.getSoundBuffer(id); + return this.editingTarget.sprite.soundBank.getSoundPlayer(id).buffer; } return null; } @@ -564,7 +564,7 @@ class VirtualMachine extends EventEmitter { const sound = this.editingTarget.sprite.sounds[soundIndex]; const id = sound ? sound.soundId : null; if (id && this.runtime && this.runtime.audioEngine) { - this.runtime.audioEngine.updateSoundBuffer(id, newBuffer); + this.editingTarget.sprite.soundBank.getSoundPlayer(id).buffer = newBuffer; } // Update sound in runtime if (soundEncoding) { diff --git a/test/unit/blocks_sounds.js b/test/unit/blocks_sounds.js index 9a44e7f9f..53acc8aff 100644 --- a/test/unit/blocks_sounds.js +++ b/test/unit/blocks_sounds.js @@ -11,10 +11,10 @@ const util = { {name: 'second name', soundId: 'second soundId'}, {name: 'third name', soundId: 'third soundId'}, {name: '6', soundId: 'fourth soundId'} - ] - }, - audioPlayer: { - playSound: soundId => (playedSound = soundId) + ], + soundBank: { + playSound: (target, soundId) => (playedSound = soundId) + } } } }; From 4431b43e45744e07c7d16bfd0749a4b0816debf8 Mon Sep 17 00:00:00 2001 From: Corey Frang Date: Fri, 22 Jun 2018 09:45:23 -0400 Subject: [PATCH 18/22] A few leftover loadSound that werent updated --- src/serialization/sb2.js | 2 +- src/sprites/sprite.js | 2 +- src/virtual-machine.js | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index 9ca378d90..ffde8220a 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -420,7 +420,7 @@ const parseScratchObject = function (object, runtime, extensions, topLevel, zip) // followed by the file ext const assetFileName = `${soundSource.soundID}.${ext}`; soundPromises.push(deserializeSound(sound, runtime, zip, assetFileName) - .then(() => loadSound(sound, runtime))); + .then(() => loadSound(sound, runtime, sprite))); } } diff --git a/src/sprites/sprite.js b/src/sprites/sprite.js index c7c35c9e0..bc61960a0 100644 --- a/src/sprites/sprite.js +++ b/src/sprites/sprite.js @@ -155,7 +155,7 @@ class Sprite { newSprite.sounds = this.sounds.map(sound => { const newSound = Object.assign({}, sound); const soundAsset = this.runtime.storage.get(sound.assetId); - assetPromises.push(loadSoundFromAsset(newSound, soundAsset, this.runtime)); + assetPromises.push(loadSoundFromAsset(newSound, soundAsset, this.runtime, this)); return newSound; }); diff --git a/src/virtual-machine.js b/src/virtual-machine.js index 9e93b79d2..57ae97e76 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -495,7 +495,7 @@ class VirtualMachine extends EventEmitter { duplicateSound (soundIndex) { const originalSound = this.editingTarget.getSounds()[soundIndex]; const clone = Object.assign({}, originalSound); - return loadSound(clone, this.runtime).then(() => { + return loadSound(clone, this.runtime, this.editingTarget.sprite).then(() => { this.editingTarget.addSound(clone, soundIndex + 1); this.emitTargetsUpdate(); }); @@ -966,8 +966,8 @@ class VirtualMachine extends EventEmitter { shareSoundToTarget (soundIndex, targetId) { const originalSound = this.editingTarget.getSounds()[soundIndex]; const clone = Object.assign({}, originalSound); - return loadSound(clone, this.runtime).then(() => { - const target = this.runtime.getTargetById(targetId); + const target = this.runtime.getTargetById(targetId); + return loadSound(clone, this.runtime, target.sprite).then(() => { if (target) { target.addSound(clone); this.emitTargetsUpdate(); From f06b8a39432f2181c0a23950d757dbc68c458f84 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Mon, 25 Jun 2018 09:11:48 -0400 Subject: [PATCH 19/22] Clean up calls to fixUpVariableReferences --- src/engine/target.js | 2 -- test/unit/engine_target.js | 8 ++++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/engine/target.js b/src/engine/target.js index b5d94eb5a..69620c91b 100644 --- a/src/engine/target.js +++ b/src/engine/target.js @@ -348,8 +348,6 @@ class Target extends EventEmitter { * variable does exist in the target itself (e.g. it's a local variable in the sprite being uploaded), * then the 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. - // * @param {Target} target The new target being uploaded, with potential variable conflicts - // * @param {Runtime} runtime The runtime context with any pre-existing variables */ fixUpVariableReferences () { if (!this.runtime) return; // There's no runtime context to conflict with diff --git a/test/unit/engine_target.js b/test/unit/engine_target.js index c39154cd2..098908e1f 100644 --- a/test/unit/engine_target.js +++ b/test/unit/engine_target.js @@ -306,7 +306,7 @@ test('fixUpVariableReferences fixes sprite global var conflicting with project g 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(target, runtime); + target.fixUpVariableReferences(); t.equal(Object.keys(target.variables).length, 0); t.equal(Object.keys(stage.variables).length, 1); @@ -344,7 +344,7 @@ test('fixUpVariableReferences fixes sprite local var conflicting with project gl t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id'); t.equal(target.variables['mock var id'].name, 'a mock variable'); - target.fixUpVariableReferences(target, runtime); + target.fixUpVariableReferences(); t.equal(Object.keys(target.variables).length, 1); t.equal(Object.keys(stage.variables).length, 1); @@ -378,7 +378,7 @@ test('fixUpVariableReferences fixes conflicting sprite local var without blocks t.equal(Object.keys(stage.variables).length, 1); t.equal(target.variables['mock var id'].name, 'a mock variable'); - target.fixUpVariableReferences(target, runtime); + target.fixUpVariableReferences(); t.equal(Object.keys(target.variables).length, 1); t.equal(Object.keys(stage.variables).length, 1); @@ -414,7 +414,7 @@ test('fixUpVariableReferences does not change variable name if there is no varia t.equal(target.blocks.getBlock('a block').fields.VARIABLE.id, 'mock var id'); t.equal(target.variables['mock var id'].name, 'a mock variable'); - target.fixUpVariableReferences(target, runtime); + target.fixUpVariableReferences(); t.equal(Object.keys(target.variables).length, 1); t.equal(Object.keys(stage.variables).length, 2); From 55a5592ab6fda4f3c769d6173f69ad9101e6550e Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Mon, 25 Jun 2018 09:23:04 -0400 Subject: [PATCH 20/22] Add optional type param to export sprite API. --- src/virtual-machine.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/virtual-machine.js b/src/virtual-machine.js index b0571e52c..7218d4432 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -281,7 +281,19 @@ class VirtualMachine extends EventEmitter { } } - exportSprite (targetId) { + /** + * Exports a sprite in the sprite3 format. + * @param {string} targetId ID of the target to export + * @param {string=} optZipType Optional type that the resulting + * zip should be outputted in. Options are: base64, binarystring, + * array, uint8array, arraybuffer, blob, or nodebuffer. Defaults to + * blob if argument not provided. + * See https://stuk.github.io/jszip/documentation/api_jszip/generate_async.html#type-option + * for more information about these options. + * @return {object} A generated zip of the sprite and its assets in the format + * specified by optZipType or blob by default. + */ + exportSprite (targetId, optZipType) { const soundDescs = serializeSounds(this.runtime, targetId); const costumeDescs = serializeCostumes(this.runtime, targetId); const spriteJson = JSON.stringify(sb3.serialize(this.runtime, targetId)); @@ -291,7 +303,7 @@ class VirtualMachine extends EventEmitter { this._addFileDescsToZip(soundDescs.concat(costumeDescs), zip); return zip.generateAsync({ - type: 'blob', + type: typeof optZipType === 'string' ? optZipType : 'blob', compression: 'DEFLATE', compressionOptions: { level: 6 From 0dffc65ce99307d048f6b9a10b1c31b01ab0133d Mon Sep 17 00:00:00 2001 From: "Michael \"Z\" Goddard" Date: Mon, 25 Jun 2018 15:31:35 -0400 Subject: [PATCH 21/22] stop drum immediately if it is still starting (#1266) --- src/extensions/scratch3_music/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/extensions/scratch3_music/index.js b/src/extensions/scratch3_music/index.js index 9099dc98a..1ef4d6942 100644 --- a/src/extensions/scratch3_music/index.js +++ b/src/extensions/scratch3_music/index.js @@ -786,7 +786,7 @@ class Scratch3MusicBlocks { if (typeof player === 'undefined') return; - if (player.isPlaying) { + if (player.isPlaying && !player.isStarting) { // Take the internal player state and create a new player with it. // `.play` does this internally but then instructs the sound to // stop. @@ -889,7 +889,7 @@ class Scratch3MusicBlocks { const player = this._instrumentPlayerNoteArrays[inst][note]; - if (player.isPlaying) { + if (player.isPlaying && !player.isStarting) { // Take the internal player state and create a new player with it. // `.play` does this internally but then instructs the sound to // stop. From 236f914f556d2066587c154da59dc8c224b25a9c Mon Sep 17 00:00:00 2001 From: Mx Corey Frang Date: Mon, 25 Jun 2018 15:44:39 -0400 Subject: [PATCH 22/22] delete clone should not stop sound (#1269) --- src/sprites/rendered-target.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/sprites/rendered-target.js b/src/sprites/rendered-target.js index 93e7d8c7f..eb996786b 100644 --- a/src/sprites/rendered-target.js +++ b/src/sprites/rendered-target.js @@ -1044,7 +1044,7 @@ class RenderedTarget extends Target { onStopAll () { this.clearEffects(); if (this.sprite.soundBank) { - this.sprite.soundBank.stopAllSounds(this); + this.sprite.soundBank.stopAllSounds(); } } @@ -1130,9 +1130,6 @@ class RenderedTarget extends Target { dispose () { this.runtime.changeCloneCounter(-1); this.runtime.stopForTarget(this); - if (this.sprite.soundBank) { - this.sprite.soundBank.stopAllSounds(this); - } this.sprite.removeClone(this); if (this.renderer && this.drawableID !== null) { this.renderer.destroyDrawable(this.drawableID, this.isStage ?