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

refactor rotation interpolation logic for PathFollower3D #64043

Closed

Conversation

xiongyaohua
Copy link
Contributor

This PR is related with PR #63956, which do the same thing for PathFollower2D.

Move logic for ROTAION_NONE and ROTATION_ORIENTED from PathFollower3D
to Curve3D. There is no functionality change.

To move other modes require more substantial refactor, as
the current Parallel Transform Frame algorithm keep internal state, and
interpolation on Curve3D should be stateless.

@xiongyaohua
Copy link
Contributor Author

xiongyaohua commented Aug 7, 2022

In the process of inspecting code of PathFollower3D, I find other things that can be improved.

  1. The property name h_offset and v_offset are confusing. The doc says

h_offset: The node's offset along the curve.
v_offset: The node's offset perpendicular to the curve.

which doesn't make sense in 3D and incorrect. In the code h_offset moves sideway, and 'v_offset' moves up.

Probably it is better to following the naming convention in the code, and name them as:

  • s_offset for sideway
  • u_offset for up
  • f_offset for forward

However this will break api, where should I ask permission for this?

  1. The logic for rotation mode ROTATION_X, ROTATION_XY and ROTATION_XYZ are left in the PathFollower3D, because they use "Parallel Transportation Frame"(Chapter 2.5, Game Gem2), which maintain internal state, and is awkward for interpolation. If I understand correctly, the PTF should be moved to the baking stage of the Curve3D, in which it produce smooth up vector for each baked points. Then the interpolation method can be stateless.

This needs a major refactor of the baking code.

  1. Related to the last point. Currently ROTATION_X, ROTATION_XY and ROTATION_XYZ modes have strange behavior. And after inspecting code, I suspect the reason is that current baking code doesn't control degenerate cases systematically, such as
  • two neighbouring points are too near
  • three neghbouring points are nearly in a line
    handle these cases systematically may be the solution.

I'd like to inspecting further on points 2 and 3, but if I make bigger change than current PR, how should I organize it. Is it possible to base the next PR on this PR, when it is not merged yet?

It's a long post. Thank you for your patience and probably advices :)

@Calinou
Copy link
Member

Calinou commented Aug 7, 2022

However this will break api, where should I ask permission for this?

I suggest opening an issue on the godot-proposals repository.

Is it possible to base the next PR on this PR, when it is not merged yet?

Yes, you can create a pull request that includes commits from another pull request. To do so, create a branch from your current fork's branch that you use for this pull request, make one or more new commits (don't squash!), then open a pull request against Godot's upstream master branch.

@xiongyaohua xiongyaohua force-pushed the interpolate_on_curve3d branch from f23e516 to fdc78dd Compare August 7, 2022 13:49
Move logic for `ROTAION_NONE` and `ROTATION_ORIENTED` from PathFollower3D
to Curve3D. There is *no* functionality change.

To move other modes require more substantial refactor, as
the current Parallel Transform Frame algorithm keep internal state, and
interpolation on Curve3D should be stateless.
@xiongyaohua
Copy link
Contributor Author

Yes, you can create a pull request that includes commits from another pull request. To do so, create a branch from your current fork's branch that you use for this pull request, make one or more new commits (don't squash!), then open a pull request against Godot's upstream master branch.

thanks!

I suggest opening an issue on the godot-proposals repository.

I opened an proposal

@xiongyaohua
Copy link
Contributor Author

close this one, as it is incoporated in #64212

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.

2 participants