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

Unify PathFollower's h/v offsets to a single Vector2/3 #64808

Closed

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 24, 2022

Closes godotengine/godot-proposals#5104 (comment).

As brought up in #54161 (comment).

If the original name of PathFollower2D and PathFollower3D's offset is changed, it can be brought back to reunite two properties, as can be argued should've been the case quite some time ago.

And as a bonus, it's now a Vector3 for PathFollower3D!

For PathFollower2D and :
h_offset and v_offset -> Vector2 relative_offset
PathFollower3D
h_offset and v_offset -> Vector3 relative_offset (forward and back included)
image

Depends on #64804 to be merged first.

@Mickeon Mickeon requested review from a team as code owners August 24, 2022 01:13
@Mickeon Mickeon changed the title Unify PathFollower's h/v offsets to a single Vector2 Unify PathFollower's h/v offsets to a single Vector2/3 Aug 24, 2022
@YuriSizov YuriSizov added this to the 4.0 milestone Aug 24, 2022
@Mickeon Mickeon force-pushed the rename-path-unify-offsets branch 2 times, most recently from 0588ce6 to 1a4bb4b Compare August 24, 2022 13:02
@KoBeWi
Copy link
Member

KoBeWi commented Aug 24, 2022

I think this shouldn't be a Vector2. Vector2 gives an impression that it's an absolute offset, while it is not. It's relative to the curve, so e.g. offset of Vector2(0, -50) will be different depending on the orientation.

h_offset is the offset parallel to the path.
v_offset is the offset perpendicular to the path.
If the current properties don't communicate it properly, we can come up with new names.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 24, 2022

I do agree, to some extent. At the same time, however, it does make Vector math using these properties a little more laborious than it should probably be.

Maybe another name for an unified "offset" can be found, or the documentation of this new property could state "The relative offset along the curve"...?

Regardless feel free to share the thought in the proposal, too.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 26, 2022

What if we group the offset properties with offset_ prefix and call them offset_parallel and offset_perpendicular?

Maybe another name for an unified "offset" can be found

relative_offset maybe? tbh I don't use PathFollower that much to say if changing it to vector is really bad. If the changed property doesn't give wrong idea then it's probably fine.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 26, 2022

I would favour a single Vector2/3 so relative_offset is a pretty nice name for it. If it's going to be just a rename that is okay too, and still way better than the current "h" and "v". I am just not sure on what the correct mathematical terms for each axis are. offset_parallel, offset_perpendicular and offset...?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 26, 2022

offset_parallel, offset_perpendicular and offset...?

I mean they would be grouped:

  • offset (group)
    • parallel
    • perpendicular

This would make the names 100% clear on purpose, but I agree that someone might find the names hard to understand. Especially "perpendicular" is not used very often.

@Mickeon Mickeon force-pushed the rename-path-unify-offsets branch from 1a4bb4b to d47d5ae Compare September 26, 2022 16:11
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 26, 2022

In the meantime, this PR has been rebased and updated to call it relative_offset.

I mean they would be grouped:

What I did mean is, for PathFollow3D, what would be the name of the third property to go along with the other two?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 26, 2022

What I did mean is, for PathFollow3D, what would be the name of the third property to go along with the other two?

Maybe offset_forward and offset_side for Z and X.

For PathFollower2D and PathFollower3D:
`h_offset` and `v_offset` -> Vector2 `relative_offset`.

Support third dimension for PathFollower 3D, as well.
@Mickeon Mickeon force-pushed the rename-path-unify-offsets branch from d47d5ae to a9310c9 Compare September 26, 2022 16:28
@Zireael07
Copy link
Contributor

+1 for offset_forward and offset_side

@akien-mga
Copy link
Member

We discussed this in the production team. It's too late for such compat breaking renames for consistency/clarity unless they're motivated by very clear usability issues which many users support. This doesn't seem to the case here, so closing.

@akien-mga akien-mga closed this Jan 24, 2023
@Mickeon Mickeon deleted the rename-path-unify-offsets branch December 30, 2023 11:47
@AThousandShips AThousandShips removed this from the 4.0 milestone Dec 30, 2023
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.

Rename some properties of PathFollower3D and PathFollower2D
6 participants