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.

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

Add AutoVolume option to AudioPlaybackTrack to prevent to override AudioStreamPlayer volume #55218

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Nov 22, 2021

Context: #49099

https://www.youtube.com/watch?v=0ztTcX7Z-Do

In a BlendTree, AudioTrack will change the volume of the AudioStreamPlayer implicitly when blending. This is useful for damping sounds like footsteps, but it can be a problem if you want to play sound effects with NodeOneshot, or if you are controlling the volume externally. So I added an option to the AudioStreamPlayer to allow or disallow the BlendTree to change the volume.

Implemented an option in AudioPlaybackTrack to handle that whether to allow AnimationTree to override AudioStreamPlayer volume.

スクリーンショット 2021-11-22 21 31 05

スクリーンショット 2021-11-22 21 31 14

Includes little code style fixes and loop_wrap’s undo redo bug fixes in animation track editor.

@ellenhp
Copy link
Contributor

ellenhp commented Nov 22, 2021

I don't think this needs a full review from the audio team, but I do want to mention that AutoVolume seems like a confusing name for what this option does. I can't think of anything better though.

@TokageItLab
Copy link
Member Author

TokageItLab commented Nov 22, 2021

If there is a better name for this option, the change is fully acceptable :) I respect the decision of the maintainer.

@TokageItLab TokageItLab force-pushed the implement-auto-volume-option branch from 83ed198 to 538774a Compare December 7, 2021 06:25
@TokageItLab
Copy link
Member Author

TokageItLab commented Dec 7, 2021

Rebased for resolving conflicts.

Discussion in Contributors Chat with ellenhp:

ellenhp
2021/12/4 AM1:06(JST)
I didn't intend to block the PR. Quite the opposite. I was just trying to say that it doesn't touch any part of the audio system that I have knowledge on.
The "AutoVolume" comment I made was just a suggestion.
If you can't think of anything better and I can't think of anything better and nobody chimes in, I think it's probably fine 🙂

@akien-mga Please review when you have time.

@TokageItLab
Copy link
Member Author

TokageItLab commented Jan 19, 2022

Need to add to some methods in AnimationTrackEditor; superseded by #56902.

@YeldhamDev YeldhamDev removed request for a team January 19, 2022 11:54
@TokageItLab
Copy link
Member Author

TokageItLab commented Mar 8, 2022

I managed this in #56902, but reduz is not that positive about retargeting at least for 4.0, so I don't know if it will be merged by 4.0 or not.
In the meantime, I will salvage these for bug fixes separately from that.

@akien-mga akien-mga requested a review from a team July 19, 2022 13:08
@TokageItLab TokageItLab force-pushed the implement-auto-volume-option branch 2 times, most recently from 0a409de to a6683f1 Compare July 19, 2022 13:32
@TokageItLab
Copy link
Member Author

Rebased & fixed. @reduz I think that it is ready to go.

Preview (please turn on volume):

Desktop.2022.07.19.-.22.55.20.05.mp4

test project:
volume_blend_test.zip

@TokageItLab TokageItLab force-pushed the implement-auto-volume-option branch 2 times, most recently from 5ec4258 to 1af9432 Compare July 19, 2022 15:16
@reduz
Copy link
Member

reduz commented Jul 26, 2022

On one side I understand why this is wanted, the current code is not very good because it has to be fixed from AudioStream/AudioServer so the streams are faded correctly. But I am not certain that this is the best solution. The volume should just blend correctly instead, as this feels more like a hack to me.

@reduz
Copy link
Member

reduz commented Jul 26, 2022

I think the right solution for this would be for the AnimationTree (or Player) to instantiate an AudioStreamPlayback for each clip (and cache it), and have a lower level API in StreamPlayers to hand them out with a volume and position, but this is quite a good amount of work to do.

@TokageItLab
Copy link
Member Author

TokageItLab commented Jul 26, 2022

@reduz Do you think that it is the right way to simply remove this implicit volume overwrite and implement a log-scale adjustable volume interface in StreamPlayer and let the user arbitrarily adjust the volume with ValueTrack?
In the meantime, I think implicit volume overwrite should be eliminated since it is blocking sound effect placement.

@reduz
Copy link
Member

reduz commented Jul 26, 2022

@TokageItLab I am not sure, but defintely I think StreamPlayer needs more work to make this work as intended.

@TokageItLab
Copy link
Member Author

@reduz I mean, why not just have the user explicitly create a track called [ValueTrack]AudioStream:Volume? There is no need for BlendTree to change the volume of the AudioStream internally, and the implementation should be obsoleted.

The only thing that makes sense in the current implementation is that the blending is done in the linear2db() space, and the video I posted in #55218 (comment) has the problem that without the linear2db(), the volume changes are too abrupt.

Wouldn't sufficient to implement a VolumeEditMode on the AudioStream side, like Node3D's RotationEditMode, and let it set the volume value through linear2db()?

@reduz
Copy link
Member

reduz commented Jul 27, 2022

@TokageItLab because the main problem here is that you do want to play more than one stream at the same time and blend them properly, this is not possible now even if the user does something manually.

@TokageItLab
Copy link
Member Author

TokageItLab commented Jul 27, 2022

@reduz Do you mean caching and playing back multiple audio source in one AudioStream and adjust volume? That is currently not possible, but at least I think that this implicit volume overwriting should be removed, even when multiple AudioStreams are placed, as it prevents the audio from playing back.

@TokageItLab
Copy link
Member Author

@reduz It may indeed be necessary to change to a lower layer to get the blending correctly, but this PR is necessary for cases where we do not want to do the blending.

As I have said many times, the current implicit blending prevents us from using AudioTrack if we want to play sound effects in our animation. It forces us to use MethodTrack.

For now, we need to implement this AutoVolume option to prevent that implicit blending. The PR for correct blending can be send in the future.

@TokageItLab TokageItLab force-pushed the implement-auto-volume-option branch 3 times, most recently from aba52d4 to 13d3278 Compare August 16, 2022 21:45
@TokageItLab
Copy link
Member Author

As reduz said above it needs to be implemented at a lower layer. Close this PR and track the issue at #65750.

@TokageItLab TokageItLab deleted the implement-auto-volume-option branch September 16, 2022 21:04
@TokageItLab TokageItLab restored the implement-auto-volume-option branch January 26, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants