-
-
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
Allow implementing Object::_validate_property()
from GDExtension
#81515
Allow implementing Object::_validate_property()
from GDExtension
#81515
Conversation
62a5a2c
to
abef8e3
Compare
I just snuck a tiny bug fix for PR #81261 into this PR. When working on that one, I didn't realize that there was a mismatch for the data type used for |
@@ -527,6 +527,27 @@ void Object::get_property_list(List<PropertyInfo> *p_list, bool p_reversed) cons | |||
void Object::validate_property(PropertyInfo &p_property) const { | |||
_validate_propertyv(p_property); | |||
|
|||
if (_extension && _extension->validate_property) { | |||
// GDExtension uses a StringName rather than a String for property name. | |||
StringName prop_name = p_property.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.
Regarding these StringName manipulations, should we share similar concerns to other extended Object methods around here? There is this whole preprocessor pragma guard every time we cast to GDExtensionStringNamePtr
. We don't do it here, not directly at least, but maybe we should? And maybe we should add the same guards?
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 think the #pragma
stuff is about the const
ness in those casts. In the code added in this PR, we don't need to mess with const
, so I think it's OK? And running locally with GCC, I don't see any warnings.
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.
Alright, sounds fine then. I don't really understand this myself, so I'm trusting you :)
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.
Core logic seems correct to me.
Thanks! |
This is a follow-up to PR #75778 (exposed
_validate_property()
to scripting) and PR #81261 (exposed_validate_property()
to scripting languages defined in GDExtesion)This PR exposes
_validate_property()
on normal object types defined by GDExtension and registered inClassDB
Here is companion PR godotengine/godot-cpp#1239 for godot-cpp - it includes a unit test that validates that this functionality works.