Skip to content

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

Merged
Faless merged 2 commits intomasterfrom
unknown repository
Jul 28, 2022
Merged

Fixed pointer indirection in the PtrToArg template for Object arguments.#677
Faless merged 2 commits intomasterfrom
unknown repository

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.

@ghost
Copy link

ghost 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

Development

Successfully merging this pull request may close these issues.

Hanging and undefined behavior in register_virtuals when overriding RigidDynamicBody3D's _integrate_forces for GDExtension class

4 participants