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

Fix AudioStreamRandomizer.random_volume_offset_db not working #82478

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Sep 28, 2023

Fixes #82469

Note:I did not quite obey the rule of OOP here but I think it's fine. Add a dedicated virtual method for getting the volume seems a little overkill.

@jsjtxietian jsjtxietian requested a review from a team as a code owner September 28, 2023 09:55
@Chaosus Chaosus added this to the 4.2 milestone Sep 28, 2023
@rakkarage
Copy link
Contributor

get_volume_variantion_range = get_volume_variation_range? :)

@jsjtxietian jsjtxietian force-pushed the fix-AudioStreamRandomizer-random_volume_offset_db-not-working branch 2 times, most recently from e971842 to 7299ac0 Compare September 28, 2023 16:57
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 good overall, just a naming suggestion. I feel kind of silly that I didn't hook that up originally but I'm glad someone noticed and fixed it 😅

servers/audio/audio_stream.cpp Outdated Show resolved Hide resolved
@ellenhp
Copy link
Contributor

ellenhp commented Sep 28, 2023

Couldn't we just apply the volume shift here[1]? Leave the code that sets it where it is and just multiply the values returned by playing->mix by volume_scale

[1] https://github.com/godotengine/godot/blob/master/servers/audio/audio_stream.cpp#L811C3-L811C3

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Sep 29, 2023

multiply the values returned by playing->mix by volume_scale

I tried, it didn't work out. The output sound is glitch.

@jsjtxietian jsjtxietian force-pushed the fix-AudioStreamRandomizer-random_volume_offset_db-not-working branch from 7299ac0 to 57a300c Compare September 29, 2023 03:42
@jsjtxietian jsjtxietian requested a review from ellenhp October 13, 2023 10:05
scene/audio/audio_stream_player.cpp Outdated Show resolved Hide resolved
scene/audio/audio_stream_player.cpp Outdated Show resolved Hide resolved
@jsjtxietian jsjtxietian force-pushed the fix-AudioStreamRandomizer-random_volume_offset_db-not-working branch from 57a300c to dc35c9d Compare October 13, 2023 10:42
@ellenhp
Copy link
Contributor

ellenhp commented Oct 13, 2023

I tried, it didn't work out. The output sound is glitch.

Just to confirm, what I was suggesting it to save the value of volume_scale that you're calculating here into a field of the playback object. Once it's set, it won't be updated ever again. If you're re-calculating the value each time _mix is called, that's going to cause some really glitchy audio. But calculating it once when the playback is started, and then re-using it each time _mix is called will work. I don't want to approve this with the cast_to because I don't want to special-case the randomizer stream.

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Oct 13, 2023

@ellenhp Sorry for my misunderstanding and thank you for your suggestion.
What I mean is if I only add volume_scale * to this mix function like below (only change here without all my other changes), the sound is weird. But I might be wrong.

int AudioStreamPlaybackRandomizer::mix(AudioFrame *p_buffer, float p_rate_scale, int p_frames) {
	if (playing.is_valid()) {
		return volume_scale * playing->mix(p_buffer, p_rate_scale * pitch_scale, p_frames);
	} else {
		for (int i = 0; i < p_frames; i++) {
			p_buffer[i] = AudioFrame(0, 0);
		}
		return p_frames;
	}
}

And regarding special-case the randomizer stream, I do agree we should try our best to avoid cast-to because it's not clean and oop, also make the code hard to debug sometimes. I'm not familiar to the audio code but it seems to me _get_volume_vector is where we set the volume. I hope we can find the right place to insert some code and fix this problem :)

@ellenhp
Copy link
Contributor

ellenhp commented Oct 13, 2023

Ah! Okay, I see now. Can you try doing something more like this:

......
	if (playing.is_valid()) {
		int mixed_samples = playing->mix(p_buffer, p_rate_scale * pitch_scale, p_frames);
		for (int samp = 0; samp < mixed_samples; samp++) {
			p_buffer[samp] *= volume_scale;
		}
		return mixed_samples;
	} else {
......

The mix function returns an integer representing the number of samples it actually mixed into the provided buffer, and the samples themselves are written directly to the array provided in p_buffer. So to multiply the samples by volume_scale you have to go through that array and perform the multiplication for each one.

@jsjtxietian
Copy link
Contributor Author

Tested locally, it worked!

@jsjtxietian jsjtxietian force-pushed the fix-AudioStreamRandomizer-random_volume_offset_db-not-working branch from dc35c9d to 34eba41 Compare October 14, 2023 03:03
@akien-mga akien-mga changed the title Fix AudioStreamRandomizer.random_volume_offset_db not working Fix AudioStreamRandomizer.random_volume_offset_db not working Nov 9, 2023
@akien-mga akien-mga merged commit 34e34f0 into godotengine:master Nov 10, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@jsjtxietian jsjtxietian deleted the fix-AudioStreamRandomizer-random_volume_offset_db-not-working branch November 11, 2023 05:29
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.

AudioStreamRandomizer.random_volume_offset_db not working
6 participants