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

Curve3D bake refactor continue #69043

Merged
merged 1 commit into from
Nov 24, 2022
Merged

Curve3D bake refactor continue #69043

merged 1 commit into from
Nov 24, 2022

Conversation

xiongyaohua
Copy link
Contributor

@xiongyaohua xiongyaohua commented Nov 23, 2022 •
edited by YuriSizov
Loading

This PR follows #64212, further refactoring the bake code of Curve3D.

The main change is to calculate tangent directly from bezier curve, without going
through discretized polyline.

This is mathematically correct method has the following benefits:

  1. More accurate transform frames sampling along the curve;
  2. Path3DFollow movement is completely smooth, fixing Move rotation interpolation to Curve3d and refactor baking #64212 (comment);
  3. Initial up vector is not influenced by baking interval, fixing Curve3D::sample_baked_up_vector() returns value perpendicular to its previous value after merge #68893
  4. Adhoc checks and fixes become unnecessary, leading to simpler and more performant code.

The same refactor can be applied to Curve2D in a seperate PR.

Production edit: Fixes #68893

@xiongyaohua xiongyaohua requested review from a team as code owners November 23, 2022 07:32
This was referenced Nov 23, 2022
Merged
Closed
@xiongyaohua xiongyaohua changed the title Path3d bake refactor continue Curve3D bake refactor continue Nov 23, 2022
fire
fire reviewed Nov 23, 2022
View reviewed changes
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

@TokageItLab has combined many bezier curve equations into a central place. Maybe you can combine?

@TokageItLab
Copy link
Member

@fire Is it a derivative? Perhaps it would make sense to make it a separate function.

Not sure whether it is better named _bezier_interpolate_tangent() or not. It certainly gets the tangent, but it might be better to name it something like bezier_derivative() without normalizing.

@xiongyaohua
Copy link
Contributor Author

@TokageItLab combined many bezier curve equations into a central place. Maybe you can combine?

Sure. Where is this "central place" refers to?

name it something like bezier_derivative() without normalizing

Done.

TokageItLab
TokageItLab reviewed Nov 23, 2022
View reviewed changes
core/math/vector3.h Outdated Show resolved Hide resolved
fire
fire reviewed Nov 23, 2022
View reviewed changes
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

@TokageItLab Can you look and review for approval? I'm not able to check the math. The structure is good.

@YuriSizov YuriSizov added this to the 4.0 milestone Nov 23, 2022
@YuriSizov
Copy link
Contributor

Please don't forget to squash your commits and give it a short but clear message.

@xiongyaohua xiongyaohua force-pushed the path3d_bake_refactor_fix branch from 01e154f to 10c4516 Compare November 24, 2022 00:44
TokageItLab
TokageItLab requested changes Nov 24, 2022
View reviewed changes
core/math/math_funcs.h Outdated Show resolved Hide resolved
TokageItLab
TokageItLab requested changes Nov 24, 2022
View reviewed changes
scene/resources/curve.cpp Outdated Show resolved Hide resolved
scene/resources/curve.h Outdated Show resolved Hide resolved
TokageItLab
TokageItLab approved these changes Nov 24, 2022
View reviewed changes
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

The behavior seems to be better than before.

@TokageItLab
Copy link
Member

@xiongyaohua Please squash again.

@xiongyaohua
Refactor Curve3D::_bake() method
f9fa182 
The main change is to caculate tangent directly from bezier curve, without going
through discretized polyline, avoiding pitfalls of discretization.

Other changes are:
1. Add an bezier_derivative() method for Vector3, Vector2, and Math;
2. Add an tesselate_even_length() method to Curve3D, which tesselate bezier curve to even length segments adaptively;
3. Cache the tangent vectors in baked_tangent_vector_cache;
@xiongyaohua xiongyaohua force-pushed the path3d_bake_refactor_fix branch from 02158f1 to f9fa182 Compare November 24, 2022 02:52
@xiongyaohua
Copy link
Contributor Author

@xiongyaohua Please squash again.

squashed

@akien-mga akien-mga merged commit f16c5b5 into godotengine:master Nov 24, 2022
@akien-mga
Copy link
Member

Thanks!

@TokageItLab TokageItLab mentioned this pull request Nov 24, 2022
Merged
@xiongyaohua xiongyaohua deleted the path3d_bake_refactor_fix branch November 24, 2022 12:22
@xiongyaohua xiongyaohua mentioned this pull request Nov 24, 2022
Merged
@raulsntos raulsntos mentioned this pull request Nov 24, 2022
Merged
@kleonc kleonc mentioned this pull request Nov 26, 2022
Closed
@Calinou Calinou mentioned this pull request Dec 2, 2022
Closed
@xiongyaohua xiongyaohua mentioned this pull request May 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TokageItLab TokageItLab TokageItLab approved these changes

@akien-mga akien-mga akien-mga left review comments

@fire fire fire left review comments

Assignees
No one assigned
Labels
enhancement topic:core topic:3d
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

Curve3D::sample_baked_up_vector() returns value perpendicular to its previous value after merge
6 participants
@xiongyaohua @TokageItLab @YuriSizov @akien-mga @fire @Calinou