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

Rename odd PathFollow2D/3D methods #72851

Closed

Conversation

TokageItLab
Copy link
Member

These are inconsistent with other parts.

@TokageItLab TokageItLab force-pushed the rename-pathfollow-method branch from b3b8edf to 9ba72ba Compare February 7, 2023 18:35
@TokageItLab
Copy link
Member Author

Renamed to

<member name="cubic_interpolation" type="bool" setter="set_use_cubic_interpolation" getter="is_using_cubic_interpolation" default="true">

Copy link
Member

@aaronfranke aaronfranke 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 not an active user of PathFollow* but this rename makes sense to me. We could merge for 4.0.

scene/2d/path_2d.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

If we want this merged for 4.0, then it needs to handle the old property name with _set and _get virtual methods so that it doesn't break users scenes and scripts (unless they use the methods, which we don't bother handling for now).

@TokageItLab TokageItLab force-pushed the rename-pathfollow-method branch from 9ba72ba to 9613c1e Compare February 7, 2023 19:12
@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 7, 2023

@akien-mga added (I guess it was only the setter that broke the scene)

@TokageItLab TokageItLab force-pushed the rename-pathfollow-method branch from 9613c1e to c6094c8 Compare February 7, 2023 19:23
@TokageItLab
Copy link
Member Author

Also added a getter for now.

@TokageItLab TokageItLab force-pushed the rename-pathfollow-method branch from c6094c8 to ea739fa Compare February 7, 2023 19:29
@TokageItLab TokageItLab requested a review from akien-mga February 7, 2023 20:11
@akien-mga
Copy link
Member

So actually I feel the better naming (and somewhat consistent with many other books) would be:

cubic_interpolation, set_cubic_interpolation / is_cubic_interpolation_enabled

It's not perfect as it doesn't make clear that false means linear interpolation instead of no interpolation, but it's less awkward that "use" IMO.

@TokageItLab TokageItLab force-pushed the rename-pathfollow-method branch from ea739fa to 22c7bff Compare February 7, 2023 22:00
@TokageItLab
Copy link
Member Author

@akien-mga rrrenamed!

@TokageItLab TokageItLab force-pushed the rename-pathfollow-method branch from 22c7bff to 30cb87c Compare February 7, 2023 22:03
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.

I'm still not convinced by swapping the order of the words only for the getter (is_interpolation_cubic). What I had suggested was set_cubic_interpolation/is_cubic_interpolation_enabled.

All in all, I find this change quite nitpicky and while I agree that cubic_interpolation is a better property name than cubic_interp (we try to avoid abbreviations), I think on the day of the RC1 release the ship has sailed for this kind of consistency renaming.

@TokageItLab TokageItLab requested a review from akien-mga February 8, 2023 10:22
@TokageItLab TokageItLab force-pushed the rename-pathfollow-method branch from 30cb87c to 5827f74 Compare February 8, 2023 10:22
@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 8, 2023

@akien-mga Sorry, I was already tired and my mind didn't seem to be working😵‍💫 Here is my last rename.

@TokageItLab
Copy link
Member Author

@akien-mga decided this was too late for 4.0RC, so it is merged into #72842 and this will be done in 4.1.

@TokageItLab TokageItLab closed this Feb 8, 2023
@TokageItLab TokageItLab removed this from the 4.0 milestone Feb 8, 2023
@TokageItLab TokageItLab deleted the rename-pathfollow-method branch February 14, 2024 05:39
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.

3 participants