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

#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

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf3e196
#5364 Bugfix : The processSound callback given to playSound() before …
JonathannJacobs Oct 18, 2023
f91140a
#5364 : Sound : Added API for reaching THREE.Audio.loopStart() and lo…
JonathannJacobs Oct 18, 2023
61a5455
#5364 : Sound : Automatically set the loop end to the end of the soun…
JonathannJacobs Oct 19, 2023
6d48043
#5364 : Sound : Code review changes applied.
JonathannJacobs Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/components/sound.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ module.exports.Component = registerComponent('sound', {
autoplay: {default: false},
distanceModel: {default: 'inverse', oneOf: ['linear', 'inverse', 'exponential']},
loop: {default: false},
loopStart: {default: 0},
loopEnd: {default: 0},
maxDistance: {default: 10000},
on: {default: ''},
poolSize: {default: 1},
Expand All @@ -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.


// Don't pass evt because playSound takes a function as parameter.
this.playSoundBound = function () { self.playSound(); };
Expand All @@ -58,6 +61,14 @@ module.exports.Component = registerComponent('sound', {
sound.setRolloffFactor(data.rolloffFactor);
}
sound.setLoop(data.loop);
sound.setLoopStart(data.loopStart);

// With a loop start specified without a specified loop end, the end of the loop should be the end of the file
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.


sound.setVolume(data.volume);
sound.isPaused = false;
}
Expand All @@ -80,7 +91,7 @@ module.exports.Component = registerComponent('sound', {

// Remove this key from cache, otherwise we can't play it again
THREE.Cache.remove(data.src);
if (self.data.autoplay || self.mustPlay) { self.playSound(); }
if (self.data.autoplay || self.mustPlay) { self.playSound(this.processSound); }
self.el.emit('sound-loaded', self.evtDetail, false);
});
}
Expand Down Expand Up @@ -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.

this.processSound = processSound;
}
return;
}

Expand All @@ -231,6 +245,7 @@ module.exports.Component = registerComponent('sound', {
}

this.mustPlay = false;
this.processSound = undefined;
},

/**
Expand Down