#5364 Bugfix : Sound : processSound callback saved and used with loaded + mustPlay by JonathannJacobs · Pull Request #5365 · aframevr/aframe

JonathannJacobs · 2023-10-18T15:14:38Z

The processSound callback given to playSound() before the sound was loaded will now be called when loaded and not ignored anymore.

… before the sound was loaded will be called when loaded and not ignored anymore.

…) and loopEnd()

dmarcos · 2023-10-18T22:49:43Z

Thanks. Can you elaborate a scenario that illustrates the bug? Not sure I understand the fix.

…the sound if a loop start was specified without an end

JonathannJacobs · 2023-10-19T07:53:12Z

Hi dmarcos ! First, thank you for aframe, i love it.
We already talked of the issue here

Summary :
playSound have one argument, a process callback. If playSound is not yet loaded, there is a mechanic with mustPlay and some loaded events that will call playSound later, when it can run. However the given callback argument was lost here.

With the fix it is kept and used as expected.

I also added like @mrxz suggested directly the loopStart and loopEnd API bindings.

Here is some setup i tried that exposed the bug :

this.el.removeAttribute('sound'); this.el.setAttribute('sound', 'src', 'assets/some.mp3'); this.el.components.sound.playSound((snd) => { console.log('Here is the processSound callback !'); // Whatever sound changes we need snd.setLoop(true); snd.setLoopStart(playbackTime); snd.setLoopEnd(snd.buffer.duration); })

dmarcos · 2023-10-19T20:17:39Z

src/components/sound.js

    this.pool = new THREE.Group();
    this.loaded = false;
    this.mustPlay = false;
+    this.processSound = undefined; // Saved callback for the mustPlay mechanic


not need to explicitly assign variable to undefined. already undefined if it doesn't exist

Where / when is this callback called? If a callback variable should be named accordingly: playSoundCallback?

Used at line 96 : if (self.data.autoplay || self.mustPlay) { self.playSound(this.processSound); }

Yes i understand it looks useless to explicitely declare it undefined but it was giving me a chance to add a comment here. I'll remove it.

dmarcos · 2023-10-19T20:18:35Z

src/components/sound.js

+      if(data.loopStart != 0 && data.loopEnd == 0){ 
+        sound.setLoopEnd(sound.buffer.duration);
+      }
+      else sound.setLoopEnd(data.loopEnd);


else { sound.setLoopEnd(data.loopEnd); }

we always use brackets for if statements

I see, will change.

dmarcos · 2023-10-19T20:20:15Z

src/components/sound.js

    if (!this.loaded) {
      warn('Sound not loaded yet. It will be played once it finished loading');
      this.mustPlay = true;
+      if(processSound){


single liner:

if (processSound) { this.processSound = processSound; }

not sure if the if statement is necessary. probably can just do this.processSound = processSound

True, will do.

dmarcos · 2023-10-23T18:13:29Z

Thanks so much!

arpu · 2023-12-14T20:08:11Z

hi , after this change i get in the situation

sound.js:86 Uncaught TypeError: Cannot read properties of undefined (reading 'processSound')
    at eval (sound.js:86:1)
    at AudioContext.eval (three.module.js:47396:6)
eval @ sound.js:86

on line https://github.com/aframevr/aframe/blob/master/src/components/sound.js#L94

@JonathannJacobs any suggestions?

mrxz · 2023-12-15T09:24:04Z

@arpu Could you test if the changes in this PR #5414 fixes the issue?

JonathannJacobs added 2 commits October 18, 2023 17:10

aframevr#5364 Bugfix : The processSound callback given to playSound()…

bf3e196

… before the sound was loaded will be called when loaded and not ignored anymore.

aframevr#5364 : Sound : Added API for reaching THREE.Audio.loopStart(…

f91140a

…) and loopEnd()

aframevr#5364 : Sound : Automatically set the loop end to the end of …

61a5455

…the sound if a loop start was specified without an end

dmarcos reviewed Oct 19, 2023

View reviewed changes

aframevr#5364 : Sound : Code review changes applied.

6d48043

dmarcos merged commit ff6da4b into aframevr:master Oct 23, 2023

dmarcos mentioned this pull request Oct 23, 2023

Missing a way to set the start time of sound components #5364

Closed

mrxz mentioned this pull request Dec 15, 2023

Fix bug in handling processSound callback when sound hadn't loaded yet #5414

Merged

Uh oh!

Conversation

JonathannJacobs commented Oct 18, 2023

Uh oh!

dmarcos commented Oct 18, 2023

Uh oh!

JonathannJacobs commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmarcos Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmarcos Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

JonathannJacobs Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

dmarcos Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

JonathannJacobs Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

dmarcos Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

dmarcos Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

JonathannJacobs Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

dmarcos commented Oct 23, 2023

Uh oh!

arpu commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrxz commented Dec 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JonathannJacobs commented Oct 19, 2023 •

edited

Loading

dmarcos Oct 19, 2023 •

edited

Loading

arpu commented Dec 14, 2023 •

edited

Loading