Skip to content

Improve reverb predelay to delay wet sound #106543

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ueshita
Copy link
Contributor

@ueshita ueshita commented May 18, 2025

This PR modifies the Reverb predelay to separate dry from wet,
which is common in other software reverbs.

Closes: #98863

image

Why should dry and wet be separated?

Unlike direct sound, reverberation reaches our ears later.
Separating dry and wet preserves the clarity of dry, and we perceive the scale of the room.

The following captured audio were processed with the default parameter AudioEffectReverb for the footstep SFX.

before.mp4
after.mp4

@ueshita ueshita requested a review from a team as a code owner May 18, 2025 02:27
@Chaosus Chaosus added this to the 4.x milestone May 18, 2025
@fkeyzuwu
Copy link
Contributor

fkeyzuwu commented May 18, 2025

After some digging, the way to solve the problem seems pretty messy.
I believe this PR is the wrong approach.

In short, there's a few problematic things, but they all stem from one thing:
I am pretty sure the parameter named predelay isn't supposed to be named predelay like in traditional reverb effects. What I believe it does is specify the time between the early reflections, and feedback is how much early reflections there are. And in that sense it works correctly, just named incorrectly. The docs also don't say this.

Fixing this without breaking behavior compatibility for any project using the reverb would be problematic, since the current parameters are responsible for some part of the reverb effect, so we would want to preserve them. Changing the naming of these to fit with what they are actually doing and then adding an additional predelay parameter would break API compatibility. Changing the behavior of the predelay and repurposing the old behavior of the early reflections to new parameters would change the behavior of the effect in existing projects.

The only real way I can think of that wouldn't break compatibility is to add a new parameter, which will be the "predelay" we know from normal reverbs, but naming it something else. However this would add even MORE confusion. This will also be a new feature and not a bugfix at that point.

What we can maybe do to not break anything or make anything more confusing is to change the documentation of these parameters. We can acknowledge these problems and tell users that these parameters mean something different than what the parameter says, and is kept that way in order to avoid breaking changes, while keeping the reverb DSP code the same, and not adding in predelay in the traditional sense.

Don't really know what is the best approach in this situation.

@ueshita
Copy link
Contributor Author

ueshita commented May 19, 2025

Thank you for your comment.

In short, there's a few problematic things, but they all stem from one thing:
I am pretty sure the parameter named predelay isn't supposed to be named predelay like in traditional reverb effects. What I believe it does is specify the time between the early reflections, and feedback is how much early reflections there are. And in that sense it works correctly, just named incorrectly. The docs also don't say this.

I agree with this.
In the dsp code, it's an echo_buffer and I think this is the behavior of the early reflection.
It would be difficult to maintain compatibility with this modification.

The only real way I can think of that wouldn't break compatibility is to add a new parameter, which will be the "predelay" we know from normal reverbs, but naming it something else. However this would add even MORE confusion. This will also be a new feature and not a bugfix at that point.

For example, the new parameter could be initial_delay, which might be less confusing.
(This is something we should talk about in godot-proposals.)

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.

Reverb predelay does not separate dry from wet.
3 participants