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

Allow users to override SkeletonModifier3D._process_modification #91507

Merged
merged 1 commit into from
May 10, 2024

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented May 3, 2024

Currently, SkeletonModifer3D is abstract. This changes it so that it and _process_modification can be implemented.

This is needed so users can build their own custom IK, spring bones or body tracking.

@lyuma lyuma requested a review from a team as a code owner May 3, 2024 11:09
@akien-mga akien-mga changed the title Allow users to override SkeletonModifier3D._process_modification Allow users to override SkeletonModifier3D._process_modification May 3, 2024
@akien-mga akien-mga requested a review from TokageItLab May 3, 2024 11:14
@akien-mga akien-mga added this to the 4.3 milestone May 3, 2024
@lyuma lyuma force-pushed the expose_process_modification branch 2 times, most recently from d30b5b5 to fbc599a Compare May 8, 2024 06:13
@lyuma lyuma requested a review from a team as a code owner May 8, 2024 06:13
@lyuma lyuma force-pushed the expose_process_modification branch 3 times, most recently from 628271a to 275c947 Compare May 8, 2024 07:32
@lyuma
Copy link
Contributor Author

lyuma commented May 8, 2024

The main point of this PR is allowing custom modifiers with virtual _process_modification (with underscore) and I am happy with this.

I realized that when exposing process_modification (without underscore), invoking this method manually does not take the influence into account, since this logic is closely coupled to the Skeleton3D design. I tried to explain this in the documentation. But I also realize that manual processing isn't really part of the design and I can imagine we might change it in the future to handle influence differently, so I decided to mark it experimental. What do you think about this function?

@TokageItLab
Copy link
Member

TokageItLab commented May 8, 2024

In past designs, I would have considered exposing it as an advance(), but that may be a bit dangerous in the current design.

If you want to add it, I recommend to add the signal at the beginning of the process that iterates the modifier during the SKELETON_UPDATE process and return an error so that it is not fired until during that process. This is because to make blending and restoring current pose working needs that; In other words, if it is executed outside of the expected steps, the result of set_bone_pose() updated at an unintended time will remain which may cause corrupting modification calculation, moreover the Influence property will be ignored.

scene/3d/skeleton_modifier_3d.cpp Outdated Show resolved Hide resolved
@lyuma lyuma force-pushed the expose_process_modification branch from 275c947 to 357c431 Compare May 9, 2024 07:57
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.

LGTM from point of view of implementation

@lyuma lyuma force-pushed the expose_process_modification branch from 357c431 to 1ccf0c2 Compare May 10, 2024 02:49
@akien-mga akien-mga merged commit c469ab0 into godotengine:master May 10, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

4 participants