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

[Core] [Mono] [GDNative] Rename "linear_interpolate" methods to "lerp" #20371

Merged
merged 2 commits into from
Apr 29, 2020

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jul 22, 2018

Lerp is a much better name, it's shorter, more familiar, and it's consistent with lerp, inverse_lerp, and slerp in math_funcs.h, and with Mono's Mathf.Lerp and Slerp methods.

I made the changes for C#, C++, GDScript, and GDNative, but I kept the old "linear_interpolate" methods as well to maintain backwards compatibility.

@akien-mga
Copy link
Member

I don't think it a good idea for C# to diverge from the C++, GDNative and GDScript APIs, unless there's a strong reason why it should be different in C# from the other languages (like snake_case to PascalCase, etc.).

I don't think this is the case, it's just a cosmetic change which breaks the correspondence between C++/GDNative/GDScript and C#. If we want to make this change, it should be done for all languages at once next time we decide to break API compatibility for cosmetic changes (#16863).

@aaronfranke
Copy link
Member Author

If desired, I could add lerp without removing linear_interpolate just by copying the code, and we could remove linear_interpolate in 4.0?

@aaronfranke aaronfranke changed the title [Mono] Rename Vector, Transform2D, and Color LinearInterpolate to Lerp [Core] [Mono] [GDNative] Rename "linear_interpolate" methods to "lerp" Aug 1, 2018
@karroffel
Copy link
Contributor

Thanks for the GDNative implementation! Unfortunately, breaking compatibility of the core struct is a NO-GO, so I will have to create a new GDNative core extension for any new changes.

@aaronfranke
Copy link
Member Author

aaronfranke commented Aug 1, 2018

@karroffel I kept the existing methods, what else can be done to preserve compatibility?

As a quick refernece, there's one commit at the bottom which has all the GDNative changes.

If desired I could undo that commit so we can merge the rest. GDNative calling "linear_interpolate" from Core will still work since I left wrapper methods.

@aaronfranke aaronfranke force-pushed the vector-lerp branch 3 times, most recently from e06cfc4 to 74b18b5 Compare August 14, 2018 23:53
Copy link
Contributor

@neikeq neikeq left a comment

Choose a reason for hiding this comment

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

The C# changes look good to me.

@aaronfranke
Copy link
Member Author

aaronfranke commented Feb 10, 2020

I'm adding the breaks compat label since this does break C# compatibility, which wasn't important back when this PR was created, but I think we care about C# compat now.

For GDScript/etc there are still wrapper methods for linear_interpolate so compat isn't broken there but these are easy to remove if desired. Otherwise this should be good to merge.

There aren't deprecated aliases or anything as @akien-mga mentions above, but the wrapper methods can easily be converted when/if that system is added.

@neikeq
Copy link
Contributor

neikeq commented Feb 11, 2020

C# compat matters if this is going to be cherry-picked. For 4.0 I plan to do a lot of breaking changes though, if I have the time.

@akien-mga
Copy link
Member

You can fully break compatibility, the aim is to make the API more consistent in 4.0 so we don't need to drag around compat methods forever.

@aaronfranke
Copy link
Member Author

aaronfranke commented Mar 5, 2020

@akien-mga Done.

@akien-mga
Copy link
Member

Will need yet another rebase. We can now break compat for GDNative too in 4.0, we'll want the API to match Core anyway so there's no way to presence compatibility without tons of outdated bindings.

@akien-mga akien-mga merged commit 58435b0 into godotengine:master Apr 29, 2020
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the vector-lerp branch April 29, 2020 17:12
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.

5 participants