-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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 GDExtension Variant type conversion #75758
Fix GDExtension Variant type conversion #75758
Conversation
To check if I'm understanding this correctly: Presently, With this PR, rather than using Do I have that right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested the reproduction project on #75757 and I'm able to reproduce the issue. Using the changes in this PR fixes it!
Assuming I'm understanding the changes (as described in my previous comment), this seems good to me. :-)
However, it should still be reviewed by someone on the @godotengine/core or @godotengine/gdextension teams in case this would cause some collateral damage that I'm not aware of.
@dsnopek |
core/variant/variant_internal.h
Outdated
@@ -1537,7 +1537,7 @@ struct VariantTypeConstructor { | |||
} | |||
|
|||
_FORCE_INLINE_ static void type_from_variant(void *p_value, void *p_variant) { | |||
*((T *)p_value) = VariantInternalAccessor<T>::get(reinterpret_cast<Variant *>(p_variant)); | |||
*((T *)p_value) = *reinterpret_cast<Variant *>(p_variant); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for the C-style cast instead of reinterpret_cast everywhere ?
*reinterpret_cast<T *>p_value = *reinterpret_cast<Variant *>(p_variant);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the answer to the question :-) but because that cast isn't added by this PR, I don't think this should block this PR.
There's loads and loads of casts in GDExtension, and more than one bug has been caused by using the inappropriate cast for the situation, so perhaps we need an issue/PR that's dedicated to sorting out all the different casts?
b4d7d3c
to
f4645a9
Compare
Given my increased understanding of GDExtension since I wrote my last comment, and the additional testing of this change from #1132, I'm feeling pretty confident that this change is correct. Let's see if the CI agrees with me :-) Probably should still have someone else from the GDExtension team take a look at it, though - I've added it to the agenda for the next GDExtension meeting. |
Would need a rebase to pass CI, last time this PR was updated godot-cpp was mid update and it's now locked to that state. |
f4645a9
to
d7eb710
Compare
A quick explanation for why I think this change is right for future reviewers: The functions generated by Despite coming from variant_internal.h and the generic name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed at the GDExtension meeting, and this change seems fine from the perspective of godot-cpp. It would be good if @Bromeon and @touilleMan could confirm that this won't cause any problems in the Rust or Python bindings. Approved if they say it looks fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran our full CI on commit d7eb710.
All green! ✔️
Thanks! |
Fixes #75757
Fixes godotengine/godot-cpp#1132
Fixes godotengine/godot-cpp#907
Fixes godotengine/godot-cpp#1106
With this PR,
VariantTypeConstructor<T>::type_from_variant
now does the type conversion fromVariant
toT
. Previously, it onlyreinterpret_cast
the variant data byVariantInternalAccessor<T>::get
.