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

Make Skeleton3D bone simulator an internal child #95239

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

AThousandShips
Copy link
Member

The documentation lists this as internal, and it makes sense to keep this internal

@AThousandShips AThousandShips added this to the 4.3 milestone Aug 7, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner August 7, 2024 09:40
@AThousandShips AThousandShips requested a review from a team August 7, 2024 09:41
@akien-mga akien-mga merged commit 1e8bfdc into godotengine:master Aug 7, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the skeleton_internal_fix branch August 7, 2024 10:21
@AThousandShips
Copy link
Member Author

Thank you!

@MajorMcDoom
Copy link
Contributor

@AThousandShips
Hi there! I noticed that the auto-added child is still marked as active in the inspector when you inspect it in the Remote scene tree. Not sure if that actually means anything, but just thought I'd raise it.
Screenshot 2024-08-13 133249

@AThousandShips
Copy link
Member Author

That didn't happen before this? I don't see how that would be affected by this change

@MajorMcDoom
Copy link
Contributor

That didn't happen before this? I don't see how that would be affected by this change

This screenshot is from 4.3rc3. I don't think it's caused by this change, but I wasn't sure if this was a bug or something else, and this was the only place I thought to ask since the code setting the sim's active to false was right above the line changed by this PR and I thought you would be familiar with the code. Sorry about that. Should I open up a new issue?

@AThousandShips
Copy link
Member Author

I'd say to open an issue yes, I don't know the general animation area well beyond what I've worked on, and what code I can understand, my knowledge in this case was with internal/non-internal nodes

@TokageItLab
Copy link
Member

The internal Simulator active should be linked to animate_physical_bones. If it is not linked, it may be a bug. But if not so, no problem. See also #93504

@MajorMcDoom
Copy link
Contributor

The internal Simulator active should be linked to animate_physical_bones. If it is not linked, it may be a bug. But if not so, no problem. See also #93504

I see, thanks for the clarification! I think my concern now is just that there's an additional internal modifier node that is processing just because of the default value of this "linked active state" being true, despite there being no physical bones present.

If that is not an issue though, I'll just ignore it.

@TokageItLab
Copy link
Member

The existence of an active modifier will cause the deferred process to be called. So strictly speaking, there a very very small performance loss, so if you are really concerned about it, you should use a build with disable_deprecated = true to remove the internal modifier.

However, as I fixed in #93502, this should not be a big performance problem, since the updating does not actually occur in deferred process unless the physical bone is not in the child.

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.

PhysicalBoneSimulator3D child being added to Skeleton3D by Engine always
4 participants