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

Allow AudioStreamPlayer(2D) to provide pitch_scale on playback #53641

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

DeeJayLSP
Copy link
Contributor

@DeeJayLSP DeeJayLSP commented Oct 10, 2021

Fixes #53585

AudioServer forcibly gives the stream's pitch scale a value of 1 when calling start_playback_stream(). These changes ensure any pitch scale from AudioStreamPlayer and AudioStreamPlayer2D are the ones provided before calling play()

@DeeJayLSP DeeJayLSP requested review from a team as code owners October 10, 2021 16:38
@Calinou Calinou added this to the 4.0 milestone Oct 10, 2021
@ellenhp
Copy link
Contributor

ellenhp commented Oct 10, 2021

Could you use this function signature instead?

void start_playback_stream(Ref<AudioStreamPlayback> p_playback, Map<StringName, Vector<AudioFrame>> p_bus_volumes, float p_start_time = 0, float p_highshelf_gain = 0, float p_attenuation_cutoff_hz = 0, float p_pitch_scale = 1);

That way this will occur atomically. With this change the AudioServer could choose to start playing audio between one line and the next which could cause pitch to change abruptly several milliseconds after playback has started. Ears are really fast so that causes audible artifacts.

@DeeJayLSP DeeJayLSP changed the title AudioStreamPlayers: set pitch immediately after play Allow AudioStreamPlayer(2D) to provide pitch_scale on playback Oct 10, 2021
@DeeJayLSP
Copy link
Contributor Author

I have added a pitch scale parameter to the convenience method, which then provides the value to the second start_playback_stream(). Tested and worked fine.

@ellenhp
Copy link
Contributor

ellenhp commented Oct 10, 2021 via email

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Oct 10, 2021

Convenience change: instead of providing p_pitch_scale as the fourth parameter on the second start_playback_stream() it will provide as the second, so no empty parameters need to be provided by the first one. AudioStreamPlayer3D.cpp changed to fit. Tested with projects provided here and here, all functional.

Copy link
Contributor

@ellenhp ellenhp left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you so much for fixing this one! I've had a hard time finding time to work on Godot lately.

@akien-mga akien-mga merged commit b47580a into godotengine:master Oct 11, 2021
@akien-mga
Copy link
Member

Thanks!

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.

AudioStreamPlayer: play() resets pitch_scale back to 1
4 participants