Potential issue _soundSource in sound.ts

By now I have created a fully functional interactive soundtrack sequencer on top of the Sound class. For that I needed the private members _soundSource and _soundGain. I was thinking about doing a PR to make a get function for those, but there may be a broader issue with _soundSource:

As I schedule repeated instances of the same samples to create the soundtrack, each time a new _soundSource gets created in .play() and the old one is lost, so I keep track of all of them (for example for when I need to stop them all). In sound.ts, for example in dispose() and stop() only the last source is referenced.

This is no problem for me currently, but the question is if this should be addressed, for example by keeping track of them internally. I don’t think I would currently be comfortable making this change to be honest.

Hey! can you repro what the issue is in the PG? I’ll be happy to fix it but I need to understand it first :smiley:

https://www.babylonjs-playground.com/#5TUY4N#3

Will be on next nightly :slight_smile:

1 Like

if you want my input feel free to ask

It is live! Tell me if that works please :slight_smile:

How do I use it in my own project?
I only use the babylonjs/core package, is there a preview version of that?

It will be in our next npm push I would probably release next week :slight_smile:

Else you could build locally and rely on npm link if it can not wait until then ?

I checked out the change and that would actually break my code, which is fine, I can change it probably.
But this will also break code for everyone who relied on the fact that you could play the same sound multiple times and at the same time.
I’m not sure what the recommended way is of doing that, but if there isn’t one it may be good to go back to the drawing board before we fix this.

For me there is no hurry as I am keeping my own list of references to the _sourdSource which I can manipulate.

If you go through with this, you also still have to disconnect the old _soundSource.

I am sorry as @Deltakosh is only coming back in a couple of week and I was not part of the changes here, maybe you could explain to me a bit more about the issue, the broken change and how you see things ?

I am pretty sure we can fix this the both of us :slight_smile:

OK, let’s say you have an event that should play sound.mp3, and that event happens twice at the same time (so you want the sound to play twice), what would your preferred way of coding that in babylon as it is now?

more info:
The way I do it now is I load a new Sound(‘sound.mp3’) one time in my asset manager and call Play every time on the event.

the latest change makes sure that on a new Play, all old instances of the sound are stopped, which will break everyone’s code that works that way.

Ok, at least I better understand the issue, but I am pretty sure in this case you need several instances of Sound as they are back by only one AudioElement and thus can only play one at a time.

I think the change is fine then.

It does work actually. I think AudioElement is only used when streaming. If playing it multiple times should not be supported, then why are we still creating a new _soundSource when we call Play()? Disclaimer: I haven’t looked into the WebAudio API very much.

Now that we dispose it on each play I think we are fine right? I’m not the author of the audio API (it was done by David Rousset long time ago)

So this means that everything works as expected now ???

1 Like

it’s not fine imho.

Looking at the sample in the documentation:

var gunshot = new BABYLON.Sound("gunshot", "sounds/gunshot.wav", scene);

window.addEventListener("mousedown", function(evt) {
  // left click to fire
  if (evt.button === 0) {
    gunshot.play();
  }
});

if you click two times it will cut off the first one. If people follow this example, they made code that expects all the sounds to keep playing, so it would almost for sure break existing code. Also I think that sample makes sense, we don’t want to create a new Sound every time we click, so I think the api should keep supporting multiple plays at the same time.

A possible solution is: as I did in my workaround, keep a list of soundSources, but this time inside the class Sound and stop/dispose all of them when the Sound is stopped/disposed. The problem is there’s a lot of other calls to soundSource too and the class already is a bit of a mess.

The only other option is to don’t do anything (revese the fix) and let people do what I did and keep the list themselves. Then we come back to the beginning and only provide a getter:

    /**
     * Gets the WebAudio AudioBufferSourceNode, lets you keep track of and stop instances of this Sound.
     * @returns the source node
     */
    public getSoundSource(): Nullable<AudioBufferSourceNode> {
        return this._soundSource;
    }

    /**
     * Gets the WebAudio GainNode, gives you precise control over the gain of instances of this Sound.
     * @returns the gain node
     */
    public getSoundGain(): Nullable<GainNode> {
        return this._soundGain;
    }

note that soundGain has the same issues as soundSource.

I hope that is clear :slight_smile:

It looks like this gives more flexibility ???

I ll go ahead and do it today.