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

AudioEffectPitchShift: Fix distortion when pitch is 1.0 #97109

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

pattlebass
Copy link
Contributor

Fixes #96510

I believe the issue was some kind of double to float precision error.
It only occurred when changing the pitch_scale to a value different from 1.0 and then back to 1.0 (from GDScript or the inspector), but not by default or when hard-coding it to 1.0 in C++.

The project I used for testing:
pitch-shift-bug.zip
Toggling the effect on and off should no longer yield any audible difference.

@pattlebass pattlebass requested a review from a team as a code owner September 17, 2024 11:22
@AThousandShips AThousandShips added bug topic:audio cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 17, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Sep 17, 2024
@akien-mga akien-mga merged commit 27dacd5 into godotengine:master Sep 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@pattlebass pattlebass deleted the pitch-shift-fix branch September 17, 2024 19:21
@JUSTCAMH
Copy link

I believe this fix won't have the intended effect. If you were to setup a script to linearly transition from pitch shift 1.1f to 1.0f, then you'd hear audible popping as the pitch shift effect turns itself off at pitch = 1.0. When I submitted the attached issue, I tried manually turning off the pitch shift effect when the pitch is 1.0, and this had a very noticeable popping to it.

We should fix the core problem (of this supposed double to float precision error) so that you can smoothly transition pitches without any popping or distortion.

@3lis4
Copy link

3lis4 commented Sep 30, 2024

Thank you all for your time on this issue.
@JUSTCAMH Do you know if it's planned somewhere to fix the core issue ?

This issue exists since a long time but maybe wasn't properly fixed, see #20198. They also thought about disabling the effect for 1.0 value, but call it "a cheap workaround" :

As soon as you activate the effect audio gets processed by an additional algorithm which transforms the original sound; regardless of the effect’s configuration, the input is changed.
A cheap workaround would be to bypass the effect as long as the value is 1.

It's probably good to have this workaround for now, but it shouldn't prevent from fixing the core issue later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:audio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AudioEffectPitchShift has audible effect when pitch = 1
6 participants