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

Fix Pathfollow direction and add Z forward option #72842

Merged
merged 1 commit into from
May 24, 2023

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Feb 7, 2023

Fixes #53837.

The direction is changed, but some countermeasures are planned in the near future. See #72843, #72795 and #72753.

As long as #72843 will be merged, the change about the direction is not breaks compat. However, the following method changes are breaks compat. merged into this PR.


PathFollow2D:

  • Rename: rotates set_rotates is_rotating -> rotation_enabled set_rotation_enabled is_rotation_enabled
  • Remove: lookahead the DEAD PROPERTY💀

PathFollow2D/3D:

  • Rename: cubic_interp set_cubic_interpolation get_cubic_interpolation -> cubic_interpolation_enabled set_cubic_interpolation_enabled is_cubic_interpolation_enabled

But, to avoid breaking the scene, old methods is made migratable as deprecated in _set() _get().

@fire
Copy link
Member

fire commented Feb 7, 2023

I am supporting this, and this is a local change.

@TokageItLab TokageItLab force-pushed the fix-pathfollow branch 2 times, most recently from b600e3b to aaf95b4 Compare February 8, 2023 12:40
@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 8, 2023
@TokageItLab TokageItLab modified the milestones: 4.x, 4.1 Feb 8, 2023
@TokageItLab TokageItLab marked this pull request as draft February 10, 2023 01:27
@TokageItLab TokageItLab force-pushed the fix-pathfollow branch 2 times, most recently from 06aa436 to f98f92d Compare February 10, 2023 17:54
@TokageItLab TokageItLab marked this pull request as ready for review March 1, 2023 20:45
@TokageItLab
Copy link
Member Author

Rebased, and integrated #72843.

@TokageItLab TokageItLab changed the title Fix Pathfollow direction and rename methods Fix Pathfollow direction and add Z forward option and rename methods Mar 2, 2023
@TokageItLab TokageItLab requested a review from akien-mga May 23, 2023 08:38
@TokageItLab TokageItLab changed the title Fix Pathfollow direction and add Z forward option and rename methods Fix Pathfollow direction and add Z forward option May 23, 2023
@TokageItLab TokageItLab force-pushed the fix-pathfollow branch 2 times, most recently from bd8243d to a3fc8ce Compare May 23, 2023 09:58
@TokageItLab TokageItLab requested a review from a team as a code owner May 23, 2023 09:58
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.

In general I support this PR. Adding this option is a good idea, and naming it model front is a very good sensible choice.

I was initially opposed to adding new constants when they had the GLTF_* names, but I now after discussion and with the new names I think they are very good and sensible. The constants in this PR are very descriptive and are self-consistent with each other.

doc/classes/PathFollow2D.xml Show resolved Hide resolved
doc/classes/Vector3.xml Outdated Show resolved Hide resolved
}

bool PathFollow3D::get_cubic_interpolation() const {
bool PathFollow3D::is_cubic_interpolation_enabled() const {
Copy link
Member

Choose a reason for hiding this comment

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

These changes seem unrelated to adding a new model front mode to look_at. I'm not sure why not split these changes into a separate PR.

Copy link
Member Author

@TokageItLab TokageItLab May 24, 2023

Choose a reason for hiding this comment

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

I talked to @akien-mga earlier and got his agreement that these method names are bad, but he seems to have a terrible aversion to changing method names in 4.x, so I decided to change only the cpp function names same as #74190 (comment). Binding method names have not been changed. If he does not like it, I will split it.

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 updated my PR #75875 to be identical to the Basis/Transform3D/Node3D changes in this PR, excluding the path/curve changes, with the below review comments applied, and with this line added to each method's documentation:

If [param use_model_front] is [code]true[/code], the +Z axis (asset front) is treated as forward (implies +X is left) and points toward the [param target] position. By default, the -Z axis (camera forward) is treated as forward (implies +X is right).

(I also marked you as author since I started by copying the contents of this PR exactly)

core/math/transform_3d.cpp Outdated Show resolved Hide resolved
scene/3d/node_3d.cpp Outdated Show resolved Hide resolved
@TokageItLab TokageItLab force-pushed the fix-pathfollow branch 2 times, most recently from 8b73889 to ac7c401 Compare May 24, 2023 01:10
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 approve of these changes. In the event that the path/curve stuff is desired to be split off from the look_at stuff (pending decision from @akien-mga), I have this PR available that is mostly a subset of this one: #75875

Co-authored-by: aaronfranke <arnfranke@yahoo.com>
@YuriSizov
Copy link
Contributor

For the record, renaming method arguments is probably breaking compat too (for C# at least?)

@akien-mga
Copy link
Member

For the record, renaming method arguments is probably breaking compat too (for C# at least?)

That's true, but I think we've already renamed a lot of method arguments in 4.1 already. If we want to preserve this level of compatibility for C# (I personally think it's too demanding), we'd need another dedicated compatibility system for it.

@akien-mga akien-mga merged commit c39c565 into godotengine:master May 24, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Yeah, I agree. Just mentioning it so it's clear to anyone looking through the changes.

@raulsntos
Copy link
Member

Renaming arguments in C# is a source breaking change, not a binary breaking change. This means, if you are using a library that was built against 4.0 that library will still continue to work in 4.1 without having to recompile it. A source breaking change is only an issue for the application/game code but it's usually easy to fix.

In this case, the code will break only if you are explicitly using the argument names:

void MyMethod(int one, int two, int three) { ... }

// This method call will not break if the arguments of MyMethod are renamed.
MyMethod(1, 2, 3);

// This method call will break if one of the arguments of MyMethod is renamed.
MyMethod(one: 1, two: 2, three: 3);

I'm more worried about binary breaking changes because it's a lot more disruptive.

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.

Path/PathFollow (+Z) wrong orientation
6 participants