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

Add method check for _notify_skeleton_bones_renamed. #83986

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

SaracenOne
Copy link
Member

Adds check for existence of _notify_skeleton_bones_renamed method when walking nodes in PostImportPluginSkeletonRenamer. Silences errors and closes #83148

@akien-mga
Copy link
Member

akien-mga commented Oct 26, 2023

Did you see this alternative PR? #83149

Also an alternative to both, or to combine wih #83149: Why not cast to BoneAttachment3D instead of Node, and do a proper method call instead of string based?

@akien-mga akien-mga changed the title Add method check for _notify_skeleton_bones_renamed. Add method check for _notify_skeleton_bones_renamed. Oct 26, 2023
@SaracenOne
Copy link
Member Author

SaracenOne commented Oct 26, 2023

Hmm, I didn't actually. It's somewhat hard to say which is a more appropriate fix. Hypothetically with this approach, you could allow other nodes which could consume this event, but honestly, I think checking for BoneAttachment3D would be fine since this is just one relatively bespoke notification event. If we changed this to a cast, even though it's exclusively an editor only thing, we could also remove an extra method bind I guess.

@akien-mga
Copy link
Member

I think a cast and removing the unnecessary binding would be better. BTW the current cast to Node seems redundant since the elements of the TypedArray are Nodes already.

@SaracenOne SaracenOne requested a review from a team as a code owner October 26, 2023 09:13
@SaracenOne
Copy link
Member Author

Okay, I've modified it now to remove the bind, and use casting instead. I didn't put the change @dpalais made in case you want to merge that seperately, though I can put it in mine too if you'd prefer this all be one PR. It's not strictly nessecary, but it results in less looping and feels like good practice.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Minor nitpicks, but looks good to me otherwise.

@SaracenOne
Copy link
Member Author

Oh, I could have sworn I put the include in the right order. I think my IDE did something strange. Anyway, it's resolved now.

@SaracenOne
Copy link
Member Author

SaracenOne commented Oct 26, 2023

Oh, another minor thing I missed: got rid of the redunant vargs since we don't need them anymore.

@akien-mga akien-mga merged commit 346459e into godotengine:master Oct 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@fire fire deleted the skeleton_bones_renamed_check branch October 26, 2023 16:42
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.

Editor errors on import of a scene with a skeleton and a Bone Map resource
2 participants