-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Implement audio stream playback parameters. #86473
Conversation
Supersedes #65797 I guess. |
f2d3ad8
to
8791576
Compare
@AThousandShips Thanks!! Merged and squashed. |
8791576
to
3b8ad0b
Compare
Did some changes, looping is now something you can check to override, to make it less confusing. I think this is ready for review and merge. |
I actually implemented per-player audio looping in The Mirror's fork of Godot a few months ago so that audio players can be looped by users, but I didn't know if this was desired to have upstream. So from that perspective, it's great to have this in the engine, less stuff for us to maintain :P From the GLTF perspective, the latest iteration of the audio standard has looping as a part of the audio source (what Godot would call the audio stream), so this PR isn't useful for GLTF audio, but it doesn't hurt it either. |
@aaronfranke The problem we have in the audio side is that parameters exposed at stream level are very tricky because:
So, this makes editing or interacting with local instances very hard. This PR is created to make this easy and allow exposing parameters that can be edited locally. Additionally, as they appear at node level (similar to how animationtree works), you can animate them and edit them more freely. |
Now the parameter is always null and can't be changed. |
doc/classes/AudioStream.xml
Outdated
@@ -28,6 +28,12 @@ | |||
<description> | |||
</description> | |||
</method> | |||
<method name="_get_parameter_list" qualifiers="virtual const"> | |||
<return type="Array" /> |
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.
<return type="Array" /> | |
<return type="Dictionary[]" /> |
Based on the failing CI prompt.
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.
Might make sense to also slightly tweak the description below after this change since "array contains dictionaries" will be redundant.
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.
LGTM for audio. I have not built or tested so it would be good if someone else could confirm that they've played with it, but the approach seems sound and I think this'll make a lot of people really happy.
doc/classes/AudioStream.xml
Outdated
@@ -28,6 +28,12 @@ | |||
<description> | |||
</description> | |||
</method> | |||
<method name="_get_parameter_list" qualifiers="virtual const"> | |||
<return type="Array" /> |
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.
Might make sense to also slightly tweak the description below after this change since "array contains dictionaries" will be redundant.
@ellenhp Oh, very happy to see you back Ellen and thanks for the review! I still have a few bugs to fix, but should be able to get this one done soon. |
scene/2d/audio_stream_player_2d.cpp
Outdated
if (I) { | ||
ParameterData &pd = I->value; | ||
pd.value = p_value; | ||
for (Ref<AudioStreamPlayback> &playback : stream_playbacks) { | ||
playback->set_parameter(pd.path, pd.value); | ||
} | ||
if (p_value.is_null()) { | ||
playback_parameters.erase(p_name); | ||
} | ||
return true; | ||
} | ||
|
||
return false; |
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 would use the return early pattern instead of a huge if.
if (I) { | |
ParameterData &pd = I->value; | |
pd.value = p_value; | |
for (Ref<AudioStreamPlayback> &playback : stream_playbacks) { | |
playback->set_parameter(pd.path, pd.value); | |
} | |
if (p_value.is_null()) { | |
playback_parameters.erase(p_name); | |
} | |
return true; | |
} | |
return false; | |
if (!I) { | |
return false; | |
} | |
ParameterData &pd = I->value; | |
pd.value = p_value; | |
for (Ref<AudioStreamPlayback> &playback : stream_playbacks) { | |
playback->set_parameter(pd.path, pd.value); | |
} | |
if (p_value.is_null()) { | |
playback_parameters.erase(p_name); | |
} | |
return true; |
scene/3d/audio_stream_player_3d.cpp
Outdated
if (I) { | ||
ParameterData &pd = I->value; | ||
pd.value = p_value; | ||
for (Ref<AudioStreamPlayback> &playback : stream_playbacks) { | ||
playback->set_parameter(pd.path, pd.value); | ||
} | ||
if (p_value.is_null()) { | ||
playback_parameters.erase(p_name); | ||
} | ||
return true; | ||
} | ||
|
||
return false; |
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.
Same return early comment here.
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 think we will need to have a discussion about these things because I don't think we need to unify so much in style for this type of code. To me early return is not necessarily more readable always. It also makes more sense in a context like this one where all the 3 functions (_get _set _get_property list) work the same. I will do it for this PR, but I think we need to lax the review requirements a bit.
scene/audio/audio_stream_player.cpp
Outdated
if (I) { | ||
ParameterData &pd = I->value; | ||
pd.value = p_value; | ||
for (Ref<AudioStreamPlayback> &playback : stream_playbacks) { | ||
playback->set_parameter(pd.path, pd.value); | ||
} | ||
if (p_value.is_null()) { | ||
playback_parameters.erase(p_name); | ||
} | ||
return true; | ||
} | ||
|
||
return false; |
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.
Same return early comment here.
Ah shit, I thought It worked, let me test again 😆 |
Yeah nevermind, I was misled thinking it worked, can't do it like this. Guess the null will have to stay if unassigned, else the logic gets more hairy due to using checkable logic. |
Maybe you can handle it using |
@KoBeWi The problem is that this is very parameter dependant, and this null comes from a checkable parameter in the OGG/MP3 streams, other parameters may not necessarily be like this. |
c35a0fc
to
12d87c1
Compare
We'll get a report as soon as someone finds out a parameter in their scene that they can't remove. |
The fact that |
Actually this should check if the value is equal to the default value defined in the parameter. |
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.
Well, aside from the null issue, looks fine.
I'll try to address it when I update #87061.
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.
Follow-up from previous review
Implements a way for audio stream playback to be configured via parameters directly in the edited AudioStreamPlayer[2D/3D]. Currently, configuring the playback stream is not possible (or is sometimes hacky as the user has to obtain the currently played stream, which is not always immediately available). This PR only implements this new feature to control looping in stream playback instances (a commonly requested feature, which was lost in the transition from Godot 2 to Godot 3). But the idea is that it can do a lot more: * If effects are bundled to the stream, control per playback instance parameters such as cutoff or resoance, or any other exposed effect parameter per playback instance. * For the upcoming interactive music PR (godotengine#64488), this exposes an easy way to change the active clip, which was not possible before. * For the upcoming parametrizable audio support (godotengine/godot-proposals#3394) this allows editing and animating audio graph parameters. In any case, this PR is required to complete godotengine#64488. Update modules/vorbis/audio_stream_ogg_vorbis.h Co-authored-by: A Thousand Ships <[email protected]> Update modules/minimp3/audio_stream_mp3.h Co-authored-by: A Thousand Ships <[email protected]> Update modules/minimp3/audio_stream_mp3.h Co-authored-by: A Thousand Ships <[email protected]> Update modules/vorbis/audio_stream_ogg_vorbis.h Co-authored-by: A Thousand Ships <[email protected]> Update doc/classes/AudioStream.xml Co-authored-by: A Thousand Ships <[email protected]>
12d87c1
to
a40fe16
Compare
Alright, should be ready to merge once it passes CI. |
Thanks! |
🎉 Thanks all involved for giving me feedback on this! |
Implements a way for audio stream playback to be configured via parameters directly in the edited AudioStreamPlayer[2D/3D].
Currently, configuring the playback stream is not possible (or is sometimes hacky as the user has to obtain the currently played stream, which is not always immediately available).
All you can do is configure the AudioStream itself, which affects all instances in the game. With this PR, configuring playback parameters per each stream player is possible, giving users far more flexibility and freedom to configure playback or animate parameters.
This PR only implements this new feature to control looping in stream playback instances (a commonly requested feature, which was lost in the transition from Godot 2 to Godot 3). But the idea is that it can do a lot more:
Screenshot of how it looks:
Supersedes #65797, implements godotengine/godot-proposals#3120.