Bug/inconsistency in constructor and updateOptions method of Sound

The offset and length properties of ISoundOptions are handled differently in constructor and updateOptions method of Sound.

In constructor:

this._length = options.length;
this._offset = options.offset;

In updateOptions method:

this._length = options.length ? options.length / 1000 : undefined;
this._offset = options.offset ? options.offset / 1000 : undefined;

So we need to multiply offset and length values by 1000 when using updateOptions, to make sure we get the expected result.

Playground:

Interesting.

The documentation states Defines an optional length (in seconds) inside the sound file (in seconds being the important part), and the AudioBufferSourceNode.start() seems to also take seconds (AudioBufferSourceNode.start() - Web APIs | MDN). And thou I don’t agree with this approach at all (who converts seconds to floating points…), this is how we should work.

So, I believe the updateOptions is wrong and we will fix it soon.

Yes, one needs to change, and that one should probably be the updateOptions, because seconds are what gets passed to the underlying API.

Agree! We is doing the PR? (Do not forget to update the breaking changes of the what’s new)

PR is coming in 7.5 minutes (notice the floating point on a minute. It doesn’t feel right at all! :slight_smile: )

1 Like

The updateOptions method of Sound class still has another bug (even after latest fix). If offset and/or length properties are not set in the ISoundOptions parameter, it assigns the private members _offset and/or _length to undefined. So if they were set in the constructor, their values are lost.

This playground will demonstrate the problem: https://www.babylonjs-playground.com/#DIJA7S#2

Notice that after calling updateOptions, the whole track is played, instead of only playing the part defined by offset and length.

I already sent a PR which should fix the issue.

Sorry I submitted the PR to the preview branch, didn’t know I was supposed to submit it to the master branch. Please feel free to close the PR.

Sure! No worries. Want to submit a new one? Or should I fix it in my next update?

No, please fix it in your next update.

1 Like