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 PathFollow's offsets to progress & progress_ratio #64804

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 23, 2022

Partially closes godotengine/godot-proposals#5104

As brought up in #54161 (comment).

Properties containing "offset" are very vaguely utilised across the language. Often that's fine, but for both PathFollow2D and PathFollow3D it's particularly egregious. There's offset, unit_offset, h_offset and v_offset. The latter two refer to the visual offset, and unlike other classes (such as Sprite2D), these cannot be unified as a single Vector2 value, because the singular offset property already exists, referring to the offset along the curve. This PR attempts to fix one part of that.

For both PathFollow2D and PathFollow3D:
offset -> progress
unit_offset -> progress_ratio

I find the name "progress" to be quite suitable and a much more specific term. Think of "The progress along the track, measured in units of distance".
Meanwhile, "ratio" fits a lot more than "unit" to to imply the range between 0.0 and 1.0. The new name also allows it to be grouped better along with "progress" This name would also bring more consistency with this other PR: #64665

Applies for both PathFollow2D and PathFollow3D. Updates documentation and project converter, as well.

@Mickeon Mickeon requested review from a team as code owners August 23, 2022 22:21
@Calinou Calinou added this to the 4.0 milestone Aug 23, 2022
@Mickeon Mickeon force-pushed the rename-path-progress branch 2 times, most recently from 1f1f9e6 to 38f4015 Compare August 23, 2022 23:11
Applies for both PathFollow2D and PathFollow3D
@Mickeon

This comment was marked as outdated.

@Zireael07
Copy link
Contributor

First I've heard of progress being used in the update loop aka _process or _physics_process.

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.

Approved in PR review meeting. We bikeshedded for a while as we weren't fully convinced by the new names, but it's definitely better than the current confusion with offset, h_offset and v_offset.

@akien-mga akien-mga merged commit 88145e8 into godotengine:master Aug 25, 2022
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the rename-path-progress branch September 23, 2022 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

Rename some properties of PathFollower3D and PathFollower2D
5 participants