-
-
Notifications
You must be signed in to change notification settings - Fork 36.2k
Audio: add fadeIn & fadeOut
#32549
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.
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
Closed
+48
−0
Closed
Audio: add fadeIn & fadeOut
#32549
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've quickly checked Unreal and they also have fadein/fadeout in their Blueprint API:
https://dev.epicgames.com/documentation/en-us/unreal-engine/BlueprintAPI/Audio/Components/Audio/FadeIn
https://dev.epicgames.com/documentation/en-us/unreal-engine/BlueprintAPI/Audio/Components/Audio/FadeOut
When checking our own methods, I think
fadeIn()andfadeOut()should have an optionaldelayparameter similar toplay()andstop(). It's in seconds and you add itcurrentTimewhen scheduling the values. New singnature:Also, do we need
volumeNow? Can't we just use the current volume like infadeOut(). It seems the Unreal component also just allows to define the target volume.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we can set the volumeNow beforehand
although there might be some issues because setVolume does not set the volume immediately.
it is using the same
setTargetAtTimeAPI with a small0.01timeConstant.so it could be
setVolumewill be canceled byfadeInUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we save the value passed to
setVolume()is a private variable an use it in the fade methods? In this way, the value set viasetVolume()isn't lost when fading in an audio like in https://github.com/mrdoob/three.js/pull/32549/files#r2622754999. The implementation would use:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check if it works fine or not with the above example
it was just a hypothesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this old issue #12510
do you remember why
setTargetAtTimewas used and notsetValueAtTime?setValueAtTime(instant volume update)this._volumecurrentTime + 0.01 * 3will make suresetVolumesmoothing is completeUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we care about cases when people access
audio.gain.gain.valuedirectly?because in this scenario
this._volumewill not be setThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not ideal that
gainis public but in general we expect developers work withsetVolume().There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested the code today an realized the usage of
setTargetAtTime()insetVolume()does not work with the new fading methods. In the above code example, whenfadeIn()is executed, the current volume is not set to0.1yet which means the interpolation to the target value won't work. After some testing this can only be fixed by usingsetValueAtTime()insetVolume(). Or by using your original implementation. However, I'm not a fan of setting the current volume in the fade methods since we already havesetVolume()for that.So I think before we can make this change, we have to migrate from
setTargetAtTime()tosetValueAtTime(). But that requires good testing and some research in older issues sincesetTargetAtTime()was chosen on purpose, see https://github.com/mrdoob/three.js/pull/32549/files#r2631137839.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some more reading I'm not feeling comfortable to make the change from
setTargetAtTime()tosetValueAtTime(). As explained in WebAudio/web-audio-api#76 (comment),setTargetAtTime()provides a De-zippering behavior:setValueAtTime()does not:So if we use
setValueAtTime(), we risk to introduce (platform specific) audio regressions e.g. when changing the volume. Bugs like crackling sounds were reported in the past (I actually heard them myself) and we don't experience them since we usesetTargetAtTime().There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reconsideration it's probably better to close the PR and let fadein/fadeouts be implemented on application level.
_scheduleFading()depends onsetValueAtTime()and the consequences of using this method in audio animations are unclear.