fix: bug where incorrect track was removed when stop/start in rapid s…

…uccession (#2805) This PR fixes a bug that caused looping Sound to be unstoppable when stopped then started in rapid succession. There were two issues, an errant await and not checking the result of an `indexOf` for -1
excaliburjs · Nov 26, 2023 · 5be00b5 · 5be00b5
1 parent 44ddc31
commit 5be00b5
Show file tree

Hide file tree

Showing 9 changed files with 100 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- Fixed async issue where sound could not be stopped if stop()/start() were called in rapid succession
 - Fixed issue with input mapper where `keyboard.wasPressed(...)` did not fire
 - Fixed issue issue where TileMaps would not properly draw Tiles when setup in screen space coordinates
 - Fixed issue where the ex.Line graphics bounds were incorrect causing erroneous offscreen culling

diff --git a/package.json b/package.json
@@ -43,6 +43,7 @@
     "core:bundle:esm": "webpack --progress --config webpack.config.js --mode production --env output=esm",
     "core:watch": "npm run core:bundle -- --watch",
     "lint": "npm run eslint && npm run eslint:spec",
+    "lint:fix": "npm run eslint --fix && npm run eslint:spec --fix",
     "eslint": "eslint \"src/engine/**/*.ts\"",
     "eslint:sandbox": "eslint \"sandbox/**/*.ts\"",
     "eslint:spec": "eslint \"src/spec/**/*.ts\"",

diff --git a/sandbox/tests/sound-loop/gba1complete.mp3 b/sandbox/tests/sound-loop/gba1complete.mp3
diff --git a/sandbox/tests/sound-loop/index.html b/sandbox/tests/sound-loop/index.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+  <meta charset="UTF-8">
+  <meta name="viewport" content="width=device-width, initial-scale=1.0">
+  <title>Sound Loop Test</title>
+</head>
+<body>
+  Stopping a looping sounds should stop all running tracks
+  <script src="../../lib/excalibur.js"></script>
+  <script src="index.js"></script>
+</body>
+</html>
diff --git a/sandbox/tests/sound-loop/index.ts b/sandbox/tests/sound-loop/index.ts
@@ -0,0 +1,48 @@
+
+
+var game = new ex.Engine({
+  width: 400,
+  height: 400
+});
+
+var sound = new ex.Sound('./gba1complete.mp3');
+sound.volume = .05;
+sound.loop = true;
+
+
+var loader = new ex.Loader();
+loader.addResource(sound)
+
+class BaseScene extends ex.Scene {
+  constructor(public name) {
+    super();
+  }
+  override onActivate(): void {
+    console.log('activate', this.name);
+    sound.play();
+  }
+  override onDeactivate(): void {
+    console.log('deactivate', this.name);
+    sound.stop();
+  }
+}
+
+var scene1 = new BaseScene("scene1");
+var scene22 = new BaseScene("scene2");
+var scene3 = new BaseScene("scene3");
+
+game.add('scene1', scene1);
+game.add('scene2', scene22);
+game.add('scene3', scene3);
+
+game.start(loader).then(() => {
+  game.goToScene('scene1');
+
+  setTimeout(() => {
+    game.goToScene('scene1');
+    sound.stop();
+  }, 1000)
+});
+
+// going to the same scene again causes the sound to become unstoppable?!?!
+
diff --git a/src/engine/Interfaces/Audio.ts b/src/engine/Interfaces/Audio.ts
@@ -28,6 +28,11 @@ export interface Audio {
    */
   isPaused(): boolean;
 
+  /**
+   * Returns if the audio is stopped
+   */
+  isStopped(): boolean;
+
   /**
    * Will play the sound or resume if paused
    */

diff --git a/src/engine/Resources/Sound/Sound.ts b/src/engine/Resources/Sound/Sound.ts
@@ -228,6 +228,10 @@ export class Sound implements Audio, Loadable<AudioBuffer> {
     return this._tracks.some(t => t.isPaused());
   }
 
+  public isStopped(): boolean {
+    return this._tracks.some(t => t.isStopped());
+  }
+
   /**
    * Play the sound, returns a promise that resolves when the sound is done playing
    * An optional volume argument can be passed in to play the sound. Max volume is 1.0
@@ -355,16 +359,20 @@ export class Sound implements Audio, Loadable<AudioBuffer> {
    * Starts playback, returns a promise that resolves when playback is complete
    */
   private async _startPlayback(): Promise<boolean> {
-    const track = await this._getTrackInstance(this.data);
+    const track = this._getTrackInstance(this.data);
 
     const complete = await track.play(() => {
       this.events.emit('playbackstart', new NativeSoundEvent(this, track));
       this.logger.debug('Playing new instance for sound', this.path);
     });
 
-    // when done, remove track
     this.events.emit('playbackend', new NativeSoundEvent(this, track));
-    this._tracks.splice(this.getTrackId(track), 1);
+
+    // cleanup any done tracks
+    const trackId = this.getTrackId(track);
+    if (trackId !== -1) {
+      this._tracks.splice(trackId, 1);
+    }
 
     return complete;
   }

diff --git a/src/engine/Resources/Sound/WebAudioInstance.ts b/src/engine/Resources/Sound/WebAudioInstance.ts
@@ -2,6 +2,7 @@ import { StateMachine } from '../../Util/StateMachine';
 import { Audio } from '../../Interfaces/Audio';
 import { clamp } from '../../Math/util';
 import { AudioContextFactory } from './AudioContext';
+import { Future } from '../../Util/Future';
 
 interface SoundState {
   startedAt: number;
@@ -16,10 +17,8 @@ export class WebAudioInstance implements Audio {
   private _instance: AudioBufferSourceNode;
   private _audioContext: AudioContext = AudioContextFactory.create();
   private _volumeNode = this._audioContext.createGain();
-  private _playingResolve: (value: boolean) => void;
-  private _playingPromise = new Promise<boolean>((resolve) => {
-    this._playingResolve = resolve;
-  });
+
+  private _playingFuture = new Future<boolean>();
   private _stateMachine = StateMachine.create({
     start: 'STOPPED',
     states: {
@@ -42,7 +41,7 @@ export class WebAudioInstance implements Audio {
         onExit: ({to}) => {
           // If you've exited early only resolve if explicitly STOPPED
           if (to === 'STOPPED') {
-            this._playingResolve(true);
+            this._playingFuture.resolve(true);
           }
           // Whenever you're not playing... you stop!
           this._instance.onended = null; // disconnect the wired on-end handler
@@ -63,7 +62,7 @@ export class WebAudioInstance implements Audio {
         onEnter: ({data}: {from: string, data: SoundState}) => {
           data.pausedAt = 0;
           data.startedAt = 0;
-          this._playingResolve(true);
+          this._playingFuture.resolve(true);
         },
         transitions: ['PLAYING', 'PAUSED', 'SEEK']
       },
@@ -94,7 +93,7 @@ export class WebAudioInstance implements Audio {
   private _handleEnd() {
     if (!this.loop) {
       this._instance.onended = () => {
-        this._playingResolve(true);
+        this._playingFuture.resolve(true);
       };
     }
   }
@@ -110,7 +109,7 @@ export class WebAudioInstance implements Audio {
       this._instance.loop = value;
       if (!this.loop) {
         this._instance.onended = () => {
-          this._playingResolve(true);
+          this._playingFuture.resolve(true);
         };
       }
     }
@@ -168,11 +167,15 @@ export class WebAudioInstance implements Audio {
     return this._stateMachine.in('PAUSED') || this._stateMachine.in('SEEK');
   }
 
+  public isStopped() {
+    return this._stateMachine.in('STOPPED');
+  }
+
   // eslint-disable-next-line @typescript-eslint/no-empty-function
   public play(playStarted: () => any = () => {}) {
     this._playStarted = playStarted;
     this._stateMachine.go('PLAYING');
-    return this._playingPromise;
+    return this._playingFuture.promise;
   }
 
   public pause() {

diff --git a/src/spec/SoundSpec.ts b/src/spec/SoundSpec.ts
@@ -295,8 +295,6 @@ describe('Sound resource', () => {
 
   it('should not have any tracks when stopped', (done) => {
     sut.load().then(() => {
-      sut.play();
-
       sut.once('playbackstart', () => {
         expect(sut.instanceCount()).toBe(1, 'should be one track');
 
@@ -306,13 +304,12 @@ describe('Sound resource', () => {
 
         done();
       });
+      sut.play();
     });
   });
 
   it('should not remove instance if paused', (done) => {
     sut.load().then(() => {
-      sut.play();
-
       sut.once('playbackstart', () => {
         expect(sut.instanceCount()).toBe(1, 'should be one track');
 
@@ -322,6 +319,7 @@ describe('Sound resource', () => {
 
         done();
       });
+      sut.play();
     });
   });
 
@@ -330,17 +328,17 @@ describe('Sound resource', () => {
       sut.loop = false;
       // start playing first track
       sut.play().then(() => {
-        expect(sut.instanceCount()).toBe(1, 'should be one track');
+        expect(sut.instanceCount()).withContext('should be on track').toBe(1);
       });
 
       // wait 250ms then play 2nd track
       setTimeout(() => {
         sut.on('playbackstart', () => {
-          expect(sut.instanceCount()).toBe(2, 'should be two simultaneous tracks');
+          expect(sut.instanceCount()).withContext('should be two simultaneous tracks').toBe(2);
         });
 
         sut.play().then(() => {
-          expect(sut.instanceCount()).toBe(0, 'should be no tracks');
+          expect(sut.instanceCount()).withContext('should be no tracks').toBe(0);
           done();
         });
       }, 250);
@@ -415,7 +413,6 @@ describe('Sound resource', () => {
     it('should stop all tracks when engine is stopped', (done) => {
       sut.load().then(() => {
         sut.wireEngine(engine);
-        sut.play();
 
         sut.once('playbackstart', () => {
           expect(sut.instanceCount()).toBe(1, 'should be one track');
@@ -427,14 +424,13 @@ describe('Sound resource', () => {
 
           done();
         });
+        sut.play();
       });
     });
 
     it('should not allow playing tracks when engine is stopped', (done) => {
       sut.load().then(() => {
         sut.wireEngine(engine);
-        sut.play();
-
         sut.once('playbackstart', () => {
           expect(sut.isPlaying()).toBe(true, 'should be playing');
 
@@ -446,6 +442,7 @@ describe('Sound resource', () => {
 
           done();
         });
+        sut.play();
       });
     });
 
@@ -476,7 +473,6 @@ describe('Sound resource', () => {
       sut.load().then(() => {
         engine.pauseAudioWhenHidden = true;
         sut.wireEngine(engine);
-        sut.play();
 
         sut.once('playbackstart', () => {
           expect(sut.isPlaying()).toBe(true, 'should be playing');
@@ -486,6 +482,8 @@ describe('Sound resource', () => {
           }, 100);
         });
 
+        sut.play();
+
         engine.once('hidden', () => {
           setTimeout(() => {
             engine.emit('visible', new ex.VisibleEvent(engine));