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

#5364 Bugfix : Sound : processSound callback saved and used with loaded + mustPlay #5365

Merged
merged 4 commits into from
Oct 23, 2023
Merged

#5364 Bugfix : Sound : processSound callback saved and used with loaded + mustPlay #5365

merged 4 commits into from
Oct 23, 2023

Conversation

JonathannJacobs
Copy link
Contributor

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

@JonathannJacobs
aframevr#5364 Bugfix : The processSound callback given to playSound()…
bf3e196 
… before the sound was loaded will be called when loaded and not ignored anymore.
@JonathannJacobs
aframevr#5364 : Sound : Added API for reaching THREE.Audio.loopStart(…
f91140a 
…) and loopEnd()
@dmarcos
Copy link
Member

dmarcos commented Oct 18, 2023

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

@JonathannJacobs
aframevr#5364 : Sound : Automatically set the loop end to the end of …
61a5455 
…the sound if a loop start was specified without an end
@JonathannJacobs
Copy link
Contributor Author

JonathannJacobs commented Oct 19, 2023 •
edited
Loading

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
dmarcos reviewed Oct 19, 2023
View reviewed changes
src/components/sound.js Outdated
@@ -32,6 +34,7 @@ module.exports.Component = registerComponent('sound', {
this.pool = new THREE.Group();
this.loaded = false;
this.mustPlay = false;
this.processSound = undefined; // Saved callback for the mustPlay mechanic
Copy link
Member

@dmarcos dmarcos Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
dmarcos reviewed Oct 19, 2023
View reviewed changes
src/components/sound.js Outdated
if(data.loopStart != 0 && data.loopEnd == 0){
sound.setLoopEnd(sound.buffer.duration);
}
else sound.setLoopEnd(data.loopEnd);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

we always use brackets for if statements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, will change.

dmarcos
dmarcos reviewed Oct 19, 2023
View reviewed changes
src/components/sound.js Outdated
@@ -208,6 +219,9 @@ module.exports.Component = registerComponent('sound', {
if (!this.loaded) {
warn('Sound not loaded yet. It will be played once it finished loading');
this.mustPlay = true;
if(processSound){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single liner:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, will do.

@dmarcos
Copy link
Member

dmarcos commented Oct 23, 2023

Thanks so much!

@dmarcos dmarcos merged commit ff6da4b into aframevr:master Oct 23, 2023
1 check failed
@dmarcos dmarcos mentioned this pull request Oct 23, 2023
Closed
@arpu
Copy link
Contributor

arpu commented Dec 14, 2023 •
edited
Loading

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 mrxz mentioned this pull request Dec 15, 2023
Merged
@mrxz
Copy link
Contributor

mrxz commented Dec 15, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmarcos dmarcos dmarcos left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JonathannJacobs @dmarcos @arpu @mrxz