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

Fix initial skin update timing in Skeleton3D #98009

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Oct 9, 2024

I have changed the update timing on the MeshInstance side in #97489 to READY, but now move it to the original timing as ENTER_TREE with calling deferred. Instead, we will update the skin once before the deferred process in the skeleton.

This means that notifications for updates are called twice only in the first frame; once in the normal process and once in the deferred process, but this is probably not a serious problem since it is managed by the update flag, which is separate from the dirty flag.

Also, confirmed that #97459 does not recur.

@TokageItLab TokageItLab added this to the 4.4 milestone Oct 9, 2024
@TokageItLab TokageItLab requested a review from a team as a code owner October 9, 2024 07:45
@TokageItLab TokageItLab added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Oct 9, 2024
@TCROC
Copy link
Contributor

TCROC commented Oct 15, 2024

We ran into an issue with this change but also found a workaround in our project. I'll break down what happens so we can discuss whether any changes should be made in this PR to account for this problem we encountered.

So in our project, we generate icons for user inventory items at runtime. We do this by equipping an item to the character, taking a screen capture (offscreen) of that item, and then displaying it as an icon like so:

image

Now the problem is, resolve_skeleton_path has been moved to call_deferred

callable_mp(this, &MeshInstance3D::_resolve_skeleton_path).call_deferred();

This means the skeleton doesn't move right away to its location and I can't take a screenshot of it right after equipping it.

Which in turn looks like:

image

I found a workaround to be this:

foreach (var child in currentMesh.FindChildren("*", recursive: true))
{
    if (child is MeshInstance3D childSkel)
    {
        // Reassign the skeleton to itself so the skin has an immediate refresh
        childSkel.Skeleton = childSkel.Skeleton;
    }
}

This works because set_skeleton_path invokes resolve_skeleton_path right away unlike the new deferred call in notification:

https://github.com/godotengine/godot/blob/af77100e394dcaca609b15bef815ed17475e51ed/scene/3d/mesh_instance_3d.cpp#L211C1-L218C1

I propose a few possible solutions:

  1. Is the reason for call_deferred for thread safety? If so, maybe we can use the call_threadsafe function?

  2. Can we make resolve_skeleton_path public? This way I can just call currentMesh.PropagateCall("resolve_skeleton_path"); to make sure it gets invoked right away.

  3. I just bite the bullet and stick to my workaround. This is my least favorite option... but it is an option 😅

NOTE:

I ran into this same issue with the BoneAttachment3D class. We use this class when we aren't using skeletons. I worked around it by manually invoking OnSkeletonUpdate after adding the item as a child.

@fire fire requested a review from a team October 15, 2024 18:34
@TokageItLab
Copy link
Member Author

TokageItLab commented Oct 15, 2024

@TCROC

Is the reason for call_deferred for thread safety? If so, maybe we can use the call_threadsafe function?

This is probably fine. I thought I had a recurring problem with the #98001 project when I did that before, but maybe I had the wrong build binary.

I've made the change, so please test to see if it's a problem.

@TCROC
Copy link
Contributor

TCROC commented Oct 16, 2024

@TCROC

Is the reason for call_deferred for thread safety? If so, maybe we can use the call_threadsafe function?

This is probably fine. I thought I had a recurring problem with the #98001 project when I did that before, but maybe I had the wrong build binary.

I've made the change, so please test to see if it's a problem.

Just tested. Everything goes back to working again! Thanks! :)

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.

Tests from developers work correctly.

@clayjohn clayjohn removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Oct 17, 2024
@clayjohn clayjohn merged commit acc3786 into godotengine:master Oct 17, 2024
20 checks passed
@clayjohn
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants