Skip to content

Conversation

@TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Dec 27, 2024

The same formula is used in multiple locations, so commonize them. This formula for the rotation does not respect the shortest path, but does not cause jumps in values when interpolating three or more rotations.

When using the shortest path in interpolating three or more rotations, the following problems occur when there are multiple interpolations:

shortest_problem

This PR methods prevents above problem.

This way is already used in the current AnimationMixer. If InterpolationMode is LerpAngle, AnimationMixer will use it for blending while referencing the Reset animation. It is also used by default in Skeleton3D's bone pose, which is already described in the following documentation:

https://docs.godotengine.org/en/latest/tutorials/animation/animation_tree.html#for-better-blending

FYI, some animation software, such as Spine, also uses this for rotation blending.

https://en.esotericsoftware.com/forum/d/16895-mixing-of-2-layers-with-low-alpha-may-cause-rotation-error

@TokageItLab TokageItLab added this to the 4.4 milestone Dec 27, 2024
@TokageItLab TokageItLab requested review from a team as code owners December 27, 2024 06:51
@TokageItLab TokageItLab force-pushed the math-blend-from-angle branch 2 times, most recently from 8ba18ab to 4436fec Compare December 27, 2024 06:55
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.

It looks like you moved the code around and cleaned it up.

👍

Copy link
Member

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

I don't personally see the value in exposing this method, I really don't, especially in the global scope. Internally it's a nice clean up, though.

Comment on lines +179 to +190
<method name="blend_from_angle">
<return type="float" />
<param index="0" name="rest" type="float" />
<param index="1" name="from" type="float" />
<param index="2" name="to" type="float" />
<param index="3" name="weight" type="float" />
<description>
Returns the blended value of the [param from] and [param to] angles with respect to the [param rest], but does not cross over the inverted axis [param rest].
This method does not respect the shortest path like [method lerp_angle], but instead does not jump values even if more than one interpolation is stacked unless the same [param rest] is passed to them.
This is useful for preventing bone penetration into the character's body when you rotate a [Bone2D].
</description>
</method>
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to discuss with other maintainers whether this makes sense to implement as a global scope function like this. It seems highly specific, as the comment points out too, so maybe it should be a helper in a more specific class. But if there are valid use cases outside of bone rotations it can make sense as a global utility function.

Copy link
Member Author

@TokageItLab TokageItLab Jan 15, 2025

Choose a reason for hiding this comment

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

I think the most common use will be within a custom SkeletonModifier3D in GDScript.

I don't strongly disagree with making them static functions outside of global scope, e.g. Skeleton3D or Skeleton2D, but these formulas are applied to other classes than those Skeletons in AnimationTree's blending process.

So I can't say if that is the right direction to go, since it would require including Skeleton2D just for that purpose in the AnimationTree.

@akien-mga akien-mga modified the milestones: 4.4, 4.5 Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Approved, Waiting for Production

Development

Successfully merging this pull request may close these issues.

5 participants