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 non const animation node _process virtual function #97020

Merged

Conversation

GuilhermeGSousa
Copy link
Contributor

@GuilhermeGSousa GuilhermeGSousa commented Sep 14, 2024

Quick fix for #74807

I'm working on a motion matching implementation, and am currently improving its API. I currently think the best way to do this by extending AnimationRootNode, but the docs mention the _process method is deprecated, and that many APIs are missing to be able to extend animation nodes. I guess my question is, is this the case? If so, is there anything planned to support this in coming releases?

@GuilhermeGSousa GuilhermeGSousa requested review from a team as code owners September 14, 2024 22:33
@GuilhermeGSousa
Copy link
Contributor Author

GuilhermeGSousa commented Sep 15, 2024

I'm not seeing how I can register compatibility methods for GDVIRTUAL functions, is that implementer?

Also, how do changes to the C++ bindings work? Is extension_api.json automatically updated, or should I open a PR for that?

@dsnopek
Copy link
Contributor

dsnopek commented Oct 5, 2024

I'm not seeing how I can register compatibility methods for GDVIRTUAL functions, is that implementer?

There is no way to safely change a virtual method in a binary compatible way. Instead, you need to add a new virtual method under a new name, and then selectively call whichever one is implemented (usually preferring the new one first). You can check if a virtual method is implemented via the GDVIRTUAL_IS_OVERRIDDEN() macro

@dsnopek
Copy link
Contributor

dsnopek commented Oct 5, 2024

Actually, looking at the PR in more detail, I'm not sure changing const-ness actually breaks compatibility. It may be fine. Someone should test if a GDExtension that overrides this virtual method compiled for Godot 4.3 works fine when loaded into Godot compiled with this PR.

@GuilhermeGSousa
Copy link
Contributor Author

GuilhermeGSousa commented Oct 14, 2024

@dsnopek Thanks for your reply, sorry I got to this so late. I can give that a try as I'm working on one such GDExtension, I'll get back to you on that.

@GuilhermeGSousa
Copy link
Contributor Author

GuilhermeGSousa commented Oct 14, 2024

@dsnopek just tested what you suggested, loading a GDExtension that overrides the const version of this method, on godot compiled with these changes. Works great, no errors!

@GuilhermeGSousa GuilhermeGSousa requested a review from a team as a code owner October 16, 2024 13:33
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Seems fine from the perspective of GDExtension!

@Repiteo
Copy link
Contributor

Repiteo commented Oct 25, 2024

The commits will need to be squashed before this can be merged.

@GuilhermeGSousa GuilhermeGSousa force-pushed the fix-non-const-anim-node-process branch from 683322f to ec62978 Compare October 26, 2024 15:56
@Repiteo Repiteo merged commit 598cfbd into godotengine:master Oct 30, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 30, 2024

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.

Should custom animation node make "_process" as a const function?
4 participants