Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Closes #515] Refactor sound resources to address inconsistent behaviors #644

Merged
merged 23 commits into from
Sep 3, 2016

Conversation

kamranayub
Copy link
Member

@kamranayub kamranayub commented Sep 1, 2016

Closes #515

Context

Before this change, there were multiple subtle issues with how sounds were being paused and played. Both the AudioTag and WebAudio resources treated simultaneous playing and pausing differently--they weren't consistent with each other. In addition, they both inconsistently fetched the resource via XHR and handled setting data differently as well as volume (web audio was global volume and audio tags could be individual).

I basically standardized a multi-track based approach for both resources, where each "play" track keeps track of its own state. I also refactored enough where we can now share all the base controller logic for an audio resource, delegating any implementation-specific logic to new AudioTagTrack and WebAudioTrack classes.

Tasks

  • Test sandbox sounds
  • Add more proper debug logging
  • I think Resources\Sound.ts and my new AbstractAudioResource are essentially the same, I might be able to consolidate that (or at least fix the weird loading logic in AbstractAudioResource)
  • Write tests against AbstractAudioResource class (create test mock)

Looking for feedback:

  • (Done) Does Track make sense or would Instance make more sense? Or a different word? They essentially represent individual plays of a sound. But I don't want it to get confused with real multi-tracking similar to Support grouping of audio resources #380

Proposed Changes:

  • Create consistent implementation that handles behavior logic for both audio tags and web audio
  • Behavior changes:
    • Calling play() when paused will resume all unfinished tracks and will not trigger a new track to be played.
    • Calling play() when not paused will ensure current tracks play and will trigger a new track to be played.
  • Both audio tag and web audio tracks can support individual volume per play (but that is not supported in the API interface yet)
  • Updated type annotations to take advantage of new AudioContext support in TS 1.5+
  • Fix audio tag pausing/resuming to properly track offsets for each "play"
  • Fix web audio pausing/resuming to properly track offsets for each "play"
  • Fix issue with web audio where playing a sound during hidden game event would not mute the new sound

@kamranayub kamranayub added bug This issue describes undesirable, incorrect, or unexpected behavior enhancement Label applied to enhancements or improvements to existing features labels Sep 1, 2016
@kamranayub kamranayub added this to the 0.7.1 Release milestone Sep 1, 2016
/// <reference path="../Promises.ts" />

module ex {
export interface IAudio {
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to add docs

@alanag13
Copy link
Member

alanag13 commented Sep 2, 2016

Per your feedback request and my usual mode of contribution, i will push that Track is not the right term from an audio perspective and is definitely misleading (suggests layering audio resources like in a studio recording). Instance is probably too generic. I would suggest Mix as this aligns to audio terms for the collection if settings on a play and would still fit as a term if we decide to implement multitrack.

@kamranayub kamranayub changed the title [Closes #515] Fix simultaneous plays on sound instances [Closes #515] Refactor sound resources to address inconsistent behaviors Sep 2, 2016
@eonarheim
Copy link
Member

+100 for the awesome increase in test coverage! :)

@kamranayub
Copy link
Member Author

This is ready to merge when you all are good with the changes 👍

@eonarheim
Copy link
Member

Looks solid, I like the abstraction of the audio instances

:shipit:

@eonarheim eonarheim merged commit bda6db2 into master Sep 3, 2016
@eonarheim eonarheim deleted the 515-long-sounds branch September 3, 2016 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior enhancement Label applied to enhancements or improvements to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replaying a long sound will not be muted if game is not visible
4 participants