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

Fixed pointer indirection in the PtrToArg template for Object arguments. #677

Merged
merged 2 commits into from
Jul 28, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jan 2, 2022

Object pointer arguments for virtual function callbacks are not correctly converted by the PtrToArg template in godot-cpp. It is missing one level of pointer indirection, passing the void* wrapper directly to the gdnative_object_get_instance_binding implementation, which expects a Object*.

This patch adds the additional pointer dereference, which brings it in line with the core Godot PtrToArg template.

This fixes #651

@Calinou Calinou added bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation labels Jan 2, 2022
@saki7
Copy link
Contributor

saki7 commented Jan 6, 2022

This patch fixes the similar case #655 for PtrToArg<const Ref<T>&> and PtrToArg<Ref<T>>

--- a/include/godot_cpp/classes/ref.hpp
+++ b/include/godot_cpp/classes/ref.hpp
@@ -240,7 +240,7 @@ public:
 template <class T>
 struct PtrToArg<Ref<T>> {
        _FORCE_INLINE_ static Ref<T> convert(const void *p_ptr) {
-               return Ref<T>(godot::internal::gdn_interface->object_get_instance_binding((void *)p_ptr, godot::internal::token, &T::___binding_callbacks));
+               return Ref<T>(reinterpret_cast<T*>(godot::internal::gdn_interface->object_get_instance_binding(*(const GDNativeObjectPtr *)p_ptr, godot::internal::token, &T::___binding_callbacks)));
        }

        typedef Ref<T> EncodeT;
@@ -255,7 +255,7 @@ struct PtrToArg<const Ref<T> &> {
        typedef Ref<T> EncodeT;

        _FORCE_INLINE_ static Ref<T> convert(const void *p_ptr) {
-               return Ref<T>(godot::internal::gdn_interface->object_get_instance_binding((void *)p_ptr, godot::internal::token, &T::___binding_callbacks));
+               return Ref<T>(reinterpret_cast<T*>(godot::internal::gdn_interface->object_get_instance_binding(*(const GDNativeObjectPtr *)p_ptr, godot::internal::token, &T::___binding_callbacks)));
        }
 };

@lukas-toenne Could you include this in your PR?

Patch by Nana Sakisaka (saki7).
@saki7
Copy link
Contributor

saki7 commented Jan 7, 2022

Thanks!
Having these two commits together, I've confirmed overriding virtual function works without any other workarounds.

@jmdevy
Copy link

jmdevy commented Jul 13, 2022

I can also confirm that these two commits fix #655

@akien-mga akien-mga requested a review from a team July 13, 2022 09:13
@akien-mga akien-mga added this to the 4.0 milestone Jul 18, 2022
Copy link
Contributor

@Faless Faless left a comment

Choose a reason for hiding this comment

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

It took me a bit to fully understand what was going on, but this is correct.
Really excellent work both 🏅 🎆 !
And sorry for the very late review :/ it's been some crazy times, but we are finally moving into stabilization :).

@Faless Faless merged commit 8d4de1b into godotengine:master Jul 28, 2022
@Faless
Copy link
Contributor

Faless commented Jul 28, 2022

Note to self, we should probably add a test for this, I had one, but it's not great for CI, maybe an _input/_gui_input with synthetic source from GDScript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
5 participants