Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Audio effects state for clones #891

Merged
merged 4 commits into from
Jan 23, 2018
Merged

Audio effects state for clones #891

merged 4 commits into from
Jan 23, 2018

Conversation

ericrosenbaum
Copy link
Contributor

@ericrosenbaum ericrosenbaum commented Jan 9, 2018 •
edited
Loading

Resolves

Resolves scratchfoundation/scratch-audio#50

Depends on scratchfoundation/scratch-audio#71

Proposed Changes

  • Each clone has its own AudioPlayer (reverting the change here), so that they can apply audio effects (volume, pitch and pan) independently.
  • When a sprite or clone is deleted, its AudioPlayer is disposed, disconnecting its webaudio nodes. This prevents the bug where audio drops out after too many AudioPlayers have been created (Audio quality will degrade over time until it eventually fails scratch-audio#27).
  • Clones get a copy of the audio effects state from their parent sprite, and sync their own AudioPlayer to it.

Reason for Changes

If clones didn't have their own AudioPlayers, they couldn't have independent audio effects settings.

Test Coverage

Let me know if you have suggestions for tests.

@ericrosenbaum ericrosenbaum added this to the January 2018 milestone Jan 9, 2018
cwillisf
cwillisf requested changes Jan 18, 2018
View reviewed changes
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nitpick / discussion point, but otherwise this looks good.

src/blocks/scratch3_sound.js Outdated
*/
_onTargetCreated (newTarget, sourceTarget) {
if (sourceTarget) {
const soundState = this._getSoundState(sourceTarget);

This comment was marked as abuse.

# to view

This comment was marked as abuse.

# to view

@ericrosenbaum ericrosenbaum merged commit 115184a into scratchfoundation:develop Jan 23, 2018
@ericrosenbaum ericrosenbaum deleted the bugfix/audioplayer-clone-state branch January 23, 2018 16:55
@ericrosenbaum ericrosenbaum mentioned this pull request Jan 25, 2018
Closed
stefania11 pushed a commit to mitmedialab/cognimates-vm that referenced this pull request Mar 31, 2018
@paulkaplan
Merge pull request scratchfoundation#891 from LLK/greenkeeper/react-t…
133f04c 
…est-renderer-16.1.1

Update react-test-renderer to the latest version 🚀
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@cwillisf cwillisf cwillisf requested changes

Assignees

@ericrosenbaum ericrosenbaum

@cwillisf cwillisf

Labels
pr - needs review
Projects
None yet
Milestone
January 2018
Development

Successfully merging this pull request may close these issues.
2 participants
@ericrosenbaum @cwillisf