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.

Sign up for GitHub

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

Jump to bottom

Fix PathFollow3D update issues #80233

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Fix PathFollow3D update issues #80233

merged 1 commit into from
Feb 13, 2024

Conversation

xiongyaohua
Copy link
Contributor

@xiongyaohua xiongyaohua commented Aug 4, 2023 •
edited
Loading

This PR fix the following minor issues on PathFollow3D.

  • transform is not updated after setting new flags such as use_model_front
  • transform is not updated when the parent Path3D changes
  • correct_posture() behaves differently when assuming model front
  • _update_transform() was in immediate mode, could leads to chained calls on scene instantiation.

Before:

Screen.Recording.2023-08-07.at.15.08.10.mov

After:

Screen.Recording.2023-08-07.at.15.23.11.mov

@xiongyaohua xiongyaohua requested a review from a team as a code owner August 4, 2023 01:46
@Calinou Calinou added this to the 4.2 milestone Aug 4, 2023
@Calinou Calinou added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 4, 2023
@YuriSizov YuriSizov changed the title Fix PathFollow3D update issues. Fix PathFollow3D update issues Aug 4, 2023
@YuriSizov
Copy link
Contributor

Could you provide a test project that you've used to verify the fixes? Also, could you please amend the first line of the commit message to be a bit more explicit (the title of the PR is fine for that).

@xiongyaohua xiongyaohua force-pushed the fix_pathfollower3d_rotation_mode branch from 86dd781 to 94e58f2 Compare August 5, 2023 03:58
@xiongyaohua
Copy link
Contributor Author

Could you provide a test project that you've used to verify the fixes?

I found the problems when try to revamp this PR #68873

The test file uploaded by @Calinou at the end is used.

Also, could you please amend the first line of the commit message to be a bit more explicit (the title of the PR is fine for that).

Sure, changed.

@xiongyaohua xiongyaohua mentioned this pull request Aug 7, 2023
Merged
@xiongyaohua
Copy link
Contributor Author

xiongyaohua commented Aug 13, 2023 •
edited
Loading

@YuriSizov Hi, any chance to have this PR reviewed recently? It is relatively simple, and will significantly improve the user experience of this PR #80329

akien-mga
akien-mga reviewed Aug 17, 2023
View reviewed changes
scene/3d/path_3d.cpp Outdated Show resolved Hide resolved
akien-mga
akien-mga reviewed Aug 17, 2023
View reviewed changes
scene/3d/path_3d.cpp Outdated Show resolved Hide resolved
akien-mga
akien-mga reviewed Aug 17, 2023
View reviewed changes
scene/3d/path_3d.cpp Outdated Show resolved Hide resolved
@xiongyaohua xiongyaohua force-pushed the fix_pathfollower3d_rotation_mode branch 4 times, most recently from 35a3d45 to 3573419 Compare August 18, 2023 23:20
@xiongyaohua xiongyaohua force-pushed the fix_pathfollower3d_rotation_mode branch from 3573419 to fd87e34 Compare August 19, 2023 10:21
@xiongyaohua xiongyaohua force-pushed the fix_pathfollower3d_rotation_mode branch from fd87e34 to 7d59652 Compare September 16, 2023 07:03
@xiongyaohua xiongyaohua force-pushed the fix_pathfollower3d_rotation_mode branch from 7d59652 to 704aad0 Compare October 4, 2023 07:17
@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
@AThousandShips AThousandShips added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Oct 26, 2023
@AThousandShips AThousandShips removed their request for review November 9, 2023 15:42
@xiongyaohua xiongyaohua force-pushed the fix_pathfollower3d_rotation_mode branch from 704aad0 to bfdbe86 Compare November 30, 2023 14:23
YuriSizov
YuriSizov reviewed Dec 15, 2023
View reviewed changes
scene/3d/path_3d.h Outdated Show resolved Hide resolved
YuriSizov
YuriSizov reviewed Dec 15, 2023
View reviewed changes
scene/3d/path_3d.cpp Outdated Show resolved Hide resolved
YuriSizov
YuriSizov reviewed Dec 15, 2023
View reviewed changes
scene/3d/path_3d.cpp Outdated Show resolved Hide resolved
YuriSizov
YuriSizov reviewed Dec 15, 2023
View reviewed changes
scene/3d/path_3d.cpp Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

Other than what I mentioned above code looks fine and changes seem sensible.

@aaronfranke @kleonc Could you check the math, do these changes to transform updates make sense?

@xiongyaohua xiongyaohua force-pushed the fix_pathfollower3d_rotation_mode branch 3 times, most recently from dacdfd5 to b40c6a4 Compare January 9, 2024 02:25
@xiongyaohua
Fix PathFollow3D update issues
43b184b 
- transform is not updated after setting new flags such as `use_model_front`
- transform is not updated when the parent Path3D changes
- correct_posture() behaves differently when assuming model front
- _update_transform() was in immediate mode, could leads to chained calls on scene instantiation.
@xiongyaohua xiongyaohua force-pushed the fix_pathfollower3d_rotation_mode branch from b40c6a4 to 43b184b Compare January 9, 2024 02:28
@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 9, 2024
@romlok romlok mentioned this pull request Jan 16, 2024
Closed
@akien-mga akien-mga requested review from aaronfranke, kleonc and AThousandShips and removed request for YuriSizov February 8, 2024 09:43
akien-mga
akien-mga approved these changes Feb 13, 2024
View reviewed changes
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.

Looks good to me.

@akien-mga akien-mga merged commit 473c279 into godotengine:master Feb 13, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos mentioned this pull request Jul 12, 2024
Merged
@kleonc kleonc mentioned this pull request Aug 16, 2024
Closed
@kleonc
Copy link
Member

kleonc commented Aug 19, 2024

Note this should not be cherry-picked to 4.2 on its own because of a regression: #95612.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Aug 20, 2024
@akien-mga akien-mga mentioned this pull request Sep 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuriSizov YuriSizov YuriSizov left review comments

@akien-mga akien-mga akien-mga approved these changes

@aaronfranke aaronfranke Awaiting requested review from aaronfranke

@kleonc kleonc Awaiting requested review from kleonc

@AThousandShips AThousandShips Awaiting requested review from AThousandShips

Assignees
No one assigned
Labels
bug topic:core topic:3d
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
6 participants
@xiongyaohua @YuriSizov @akien-mga @kleonc @AThousandShips @Calinou