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

Deprecate damping, add reflectivity to match AudioEffectReverb's documentation #99222

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

Conversation

PhairZ
Copy link
Contributor

@PhairZ PhairZ commented Nov 14, 2024

Closes #98945.
Renamed damping to reflectivity in the documentation and class of AudioEffectReverb. And tested it.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Changing the name of a property and its methods breaks compatibility, so this isn't something we can do so lightly.

If it's correct that the name damping is wrong, then renaming it to reflectivity may be correct, but a deprecated compatibility binding still needs to be included, so that user projects don't break.

servers/audio/effects/audio_effect_reverb.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga added this to the 4.4 milestone Nov 14, 2024
@PhairZ PhairZ force-pushed the tanoshiidesu branch 4 times, most recently from b16ffc0 to 66f63ca Compare November 14, 2024 14:15
@PhairZ
Copy link
Contributor Author

PhairZ commented Nov 14, 2024

This is the best I could do. I can't really wrap my head around compatibility issues.

@akien-mga
Copy link
Member

You did pretty well! This would be sufficient if our compatibility system was advanced enough, but sadly that provides compatibility for GDExtension but not for GDScript. For GDScript we'll want to keep registering normal (not compatibility) set_damping/get_damping/damping methods/properties, just enclosed in #ifndef DISABLE_DEPRECATED.

The compatibility method bindings are used when doing changes like adding a new optional parameter, which breaks compat for GDExtension but not for GDScript.

I can update the PR myself with these changes if you want, it's not something we have well documented currently.

@PhairZ
Copy link
Contributor Author

PhairZ commented Nov 14, 2024

I'll try to give it my best later. I think i can handle it.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 14, 2024

For reference, take a look at how Control's deprecated auto_translate property handles it. See also #87530

@PhairZ PhairZ force-pushed the tanoshiidesu branch 6 times, most recently from fb1343a to 642f65f Compare November 14, 2024 21:03
@PhairZ PhairZ marked this pull request as ready for review November 14, 2024 21:43
@PhairZ PhairZ requested review from a team as code owners November 14, 2024 21:43
@PhairZ PhairZ requested a review from akien-mga November 14, 2024 21:53
@PhairZ
Copy link
Contributor Author

PhairZ commented Nov 15, 2024

Thanks, @Mickeon. The deprecated 'auto_translate' property were a good reference. I'm glad it was able to pass CI.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

I'm going to let someone else review the compatibility code

servers/audio/effects/audio_effect_reverb.cpp Outdated Show resolved Hide resolved
servers/audio/effects/audio_effect_reverb.h Outdated Show resolved Hide resolved
@Mickeon Mickeon changed the title Rename damping to reflectivity to match AudioEffectReverb's documentation Deprecate damping, add reflectivity to match AudioEffectReverb's documentation Nov 15, 2024
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 "Damp" parameter values don't match description.
3 participants