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

PitchShift effect quality and performance tweaks for different pitch scale values #57985

Merged
merged 2 commits into from
Feb 14, 2022

Conversation

Listwon
Copy link
Contributor

@Listwon Listwon commented Feb 11, 2022

Fixes #20198 and #55090
For pitch_scale close to 1.0f it simply passes untouched audio samples avoiding expensive calculations. For other pitch scales there are some quality improvements like clearing buffers when pitch scale changes or higher precision of buffers for calculations. This should prevent some artifacts.

@Listwon Listwon requested a review from a team as a code owner February 11, 2022 19:08
@Calinou Calinou added this to the 4.0 milestone Feb 11, 2022
@Calinou Calinou added cherrypick:3.4 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 11, 2022
@EviTRea
Copy link

EviTRea commented Feb 12, 2022

If this gets merge to 3.X I can finally re-export my game to make it sound normal. Thanks for the PR.

@Listwon
Copy link
Contributor Author

Listwon commented Feb 12, 2022

@EviTRea can you confirm that it fixes the issues in your project? Edit: Oh sorry, I forgot we are on master branch here.

@EviTRea
Copy link

EviTRea commented Feb 12, 2022

I don't know how to compile the engine and stuff, besides my (very short mini) game has been up for 2 months now, it's nothing urgent for me.
But yes, if this gets a back port, I'll test it ASAP. Once again thanks for the effort.

@akien-mga akien-mga merged commit f45feda into godotengine:master Feb 14, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.5.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 15, 2022
@akien-mga
Copy link
Member

Cherry-picked for 3.4.3.

@ellenhp
Copy link
Contributor

ellenhp commented Feb 26, 2022

This broke someone's custom build because they're changing the pitch scale effect parameters several times a second. Why is it necessary to clear the buffers every time there's a change in the pitch scale parameters? It seems to be causing pops. I understand that once the parameters are changed all the buffers are filled with bad data but I'd rather have bad data than no data if it means avoiding a pop.

@GlitchedCode
Copy link

GlitchedCode commented Feb 26, 2022

Following up @ellenhp's comment, here is a minimal reproduction of pops occurring on 3.x. Turn the slider up to see this in action.
PitchShiftLatencyIssue3.x.zip

@EviTRea
Copy link

EviTRea commented Feb 26, 2022

Sadly it happens on me as well. My game was to lerp the pitch on character transform, and indeed the audio broke post-shift was fixed, but the transforming period is now completely silence.
Well, guess I should've lerp in the minimal reproduction as well when opening the issue.

@Listwon
Copy link
Contributor Author

Listwon commented Feb 26, 2022

Sorry to hear that. I wasn't aware of such use case and how it will be affected. Thanks for the MRP @GlitchedCode

@Listwon
Copy link
Contributor Author

Listwon commented Feb 26, 2022

Here's another MRP using Tween to lerp between pitch scales. I'm working on fixing this right now EffectTest.zip

@Listwon
Copy link
Contributor Author

Listwon commented Mar 1, 2022

@akien-mga I think it has to be reverted as for now I don't have a solution to the issue with dynamic changing/lerping and I don't want it to break projects until I find better solution. Sorry and thanks in advance

@ellenhp
Copy link
Contributor

ellenhp commented Mar 2, 2022

Regressions aside, thank you for the contribution @Listwon! Hopefully the issues can be worked out and if nothing more the type name changes and formatting fixes would be a welcome improvement for readability. I didn't chime in on this PR before it was merged but I don't personally think it's a good idea to conditionally bypass an effect like this does. I would rather allow users to switch the bus that an audio source is playing on when they need to change pitch, then change the bus back when they want the audio to sound nice again. In 3.x that's not possible, but bus changes during playback do work pretty well in 4.x.

@EviTRea would it be possible to lerp pitch_scale on the player node instead of using the bus effect? That will mess with playback speed instead of doing a proper pitch shift, but the audio quality is much much better.

@ellenhp
Copy link
Contributor

ellenhp commented Mar 2, 2022

I realized I didn't say why I don't want to conditionally disable effect like this. The main reason is that even if we do get lerping to work with these changes there's a near-guarantee that there will be a phase offset introduced by AudioEffectPitchShift, so when we go to disable the effect, we have to reconcile that phase offset otherwise there will be a loud pop as we transition from pitch shifted audio to passthrough audio. I'm not smart enough to do that, but it might be possible for someone who understands how the algorithm works. It will almost certainly require some amount of transition time though where the audio is gradually aligned to fit back into the original phase though.

@EviTRea
Copy link

EviTRea commented Mar 3, 2022

@EviTRea would it be possible to lerp pitch_scale on the player node instead of using the bus effect? That will mess with playback speed instead of doing a proper pitch shift, but the audio quality is much much better.

I don't know?
Frankly the game is pretty much MVP, and I currently have no plan to make a full game out of it,
I'll probably explore other game ideas for a few months,
and seriously make the game with 4.0's new Tilemap...which probably won't be there in a few months.
I'll keep following the update on the topic, but I can comfortably wait for an year or more for it to sort out.
If this doesn't ever get fixed, I'll just sample all the transformation and give up the procedural approach.
My point is, don't worry about me. I want it to be fixed but I don't really need it.

@Mickeon
Copy link
Contributor

Mickeon commented Mar 7, 2022

Unfortunate that this has to be reverted. Hopefully someday it'll be patched up.

In the 3.4.3 version, I mitigated the popping by lowering the fft_size value, but I do understand it may not be the solution for everyone.

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.

AudioEffectPitchShift deteriorates sample quality "without being used"
8 participants