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.

Sign up for GitHub

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

Jump to bottom

replaying a long sound will not be muted if game is not visible #515

Closed
jedeen opened this issue Sep 1, 2015 · 3 comments · Fixed by #644
Closed

replaying a long sound will not be muted if game is not visible #515

jedeen opened this issue Sep 1, 2015 · 3 comments · Fixed by #644
Assignees
@kamranayub
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior
Milestone
0.7.1 Release

Comments

@jedeen
Copy link
Member

jedeen commented Sep 1, 2015

When playing a sound with a long duration, if sound.play() is called more than once, each successive play of the sound (after the first one) ignores game visibility and does not mute.
@jedeen jedeen added the bug This issue describes undesirable, incorrect, or unexpected behavior label Sep 1, 2015
@jedeen jedeen changed the title replaying a long sound will ignore game visibility replaying a long sound will not be muted if game is not visible Sep 5, 2015
@jedeen jedeen added the on-deck label Sep 5, 2015
@jedeen jedeen added this to the vNext milestone Sep 5, 2015
@jedeen jedeen modified the milestones: 0.7.1 Release, vNext Mar 12, 2016
@jedeen jedeen removed the on-deck label Mar 13, 2016
@kamranayub kamranayub self-assigned this Aug 31, 2016
@kamranayub
Copy link
Member

Just to clarify, we are not trying to prevent multiple simultaneous plays, just respect engine visibility and keep muted on simultaneous plays.

@eonarheim
Copy link
Member

Correct, simultaneously playing sounds is something we want. It is that subsequent plays do not respect the visibility. I think it's because the engine only keeps track the first play similar to how playing sound awaits only the first promise for completion.

@kamranayub
Copy link
Member

This doesn't appear to be an issue with AudioTag implementation--it prevents playing when paused on the same frame.

DOMException: The play() request was interrupted by a call to pause().

I believe the issue occurs in Web Audio because simultaneous plays are not tracked--therefore, only currently playing track is paused when visibility changes. When you call play() it creates a new web audio buffer context each time (stored on the same instance variable), thus you can have multiple buffers out at once and only the last one is paused.

There's another minor bug (I think) where inside the hidden event on Engine, if you call play() the first event handler that stops playing sounds will not catch the newly played sound so it will play. We maybe want to enhance Sound to either have a reference to engine or a flag that says whether to prevent new sounds from playing (set by wireEngine on visibility change events).

@kamranayub kamranayub mentioned this issue Sep 1, 2016
Merged
4 tasks
eonarheim pushed a commit that referenced this issue Sep 3, 2016
@kamranayub @eonarheim
[Closes #515] Refactor sound resources to address inconsistent behavi…
bda6db2 
…ors (#644)

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

- [x] Test sandbox sounds
- [x] Add more proper debug logging
- [x] 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`)
- [x] 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 #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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kamranayub kamranayub

Labels
bug This issue describes undesirable, incorrect, or unexpected behavior
Projects
None yet
Milestone
0.7.1 Release
Development

Successfully merging a pull request may close this issue.

[Closes #515] Refactor sound resources to address inconsistent behaviors excaliburjs/Excalibur
3 participants
@kamranayub @eonarheim @jedeen