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

Gdextension declarative property #68479

Conversation

touilleMan
Copy link
Member

See godot-rust/gdext#19 (comment)

ping @Bromeon

On top of that, I've reverted the change on GDNativeExtensionScriptInstanceInfo I made in #67750, I thought it would be simpler to have a similar interface between GDExtension and GDNativeExtensionScriptInstanceInfo, but after deeper investigation it turns out it is much better to have GDNativeExtensionScriptInstanceInfo being just a proxy over Godot's internal ScriptInstance

@Bromeon
Copy link
Contributor

Bromeon commented Nov 10, 2022

For my understanding, was it possible to add/remove/rename properties for extensions at runtime before this PR (i.e. dynamic list)?
Or was the implicit assumption that the data returned via callback would be never-changing?

Also, in light of future extension reloading (#66231), reloading would likely be done on a per-class level, requiring re-registration of the entire class including methods and properties?

@touilleMan
Copy link
Member Author

touilleMan commented Nov 11, 2022

For my understanding, was it possible to add/remove/rename properties for extensions at runtime before this PR (i.e. dynamic list)?

In theory yes given extension's get_property_list is called each time Object::get_property_list is called.

However I'm pretty sure this is not a feature but an implementation detail: having dynamic property is going to make things much more complex (think of the property view in the editor :/), on top of that all Godot internal classes don't have dynamic property and hence nobody is going to think about correctly handling that (e.g. avoiding querying the property list once and keeping it in cache).

So I think this API given the wrong kind of assumption and should be corrected ;-)

Or was the implicit assumption that the data returned via callback would be never-changing?

GDExtension has been developed together with Godot-CPP, in the latter the callback always return the same thing. From what I understand the choice of using callback was just due to how you implement type specialization in C++.

Also, in light of future extension reloading (#66231), reloading would likely be done on a per-class level, requiring re-registration of the entire class including methods and properties?

Yes, that seems logical to me given GDExtension registration of methods/properties leads to registration into ClassDB, which will forget everything when the related class is unloaded.

@raulsntos
Copy link
Member

on top of that all Godot internal classes don't have dynamic property

If I understand correctly what you mean by dynamic properties, doesn't Curve do that? And I'm sure I've been seen other Godot classes where the properties returned by get_property_list change during runtime.

void Curve::_get_property_list(List<PropertyInfo> *p_list) const {
for (int i = 0; i < _points.size(); i++) {
PropertyInfo pi = PropertyInfo(Variant::VECTOR2, vformat("point_%d/position", i));
pi.usage &= ~PROPERTY_USAGE_STORAGE;
p_list->push_back(pi);
if (i != 0) {
pi = PropertyInfo(Variant::FLOAT, vformat("point_%d/left_tangent", i));
pi.usage &= ~PROPERTY_USAGE_STORAGE;
p_list->push_back(pi);
pi = PropertyInfo(Variant::INT, vformat("point_%d/left_mode", i), PROPERTY_HINT_ENUM, "Free,Linear");
pi.usage &= ~PROPERTY_USAGE_STORAGE;
p_list->push_back(pi);
}
if (i != _points.size() - 1) {
pi = PropertyInfo(Variant::FLOAT, vformat("point_%d/right_tangent", i));
pi.usage &= ~PROPERTY_USAGE_STORAGE;
p_list->push_back(pi);
pi = PropertyInfo(Variant::INT, vformat("point_%d/right_mode", i), PROPERTY_HINT_ENUM, "Free,Linear");
pi.usage &= ~PROPERTY_USAGE_STORAGE;
p_list->push_back(pi);
}
}
}

@Bromeon
Copy link
Contributor

Bromeon commented Nov 11, 2022

How does Godot even know that the property/method list has changed? For this dynamic-ness to be remotely useful, it needs to call those callbacks prohibitively often -- meaning the extension library code has to do smart caching (aka duplication) or will waste performance just copying the same data over and over.

Wouldn't a better design be GDExtension library notifying Godot about a change in properties, for example simply by providing the new properties through a set_property_list(class, properties, ...) call?

@raulsntos
Copy link
Member

raulsntos commented Nov 11, 2022

How does Godot even know that the property/method list has changed?

You call notify_property_list_changed or just emit the property_list_changed signal on the Object.

…ype_func in GDExtension

This function pointer is needed to stay close to internal Godot's ScriptInstance class.
Besides, by removing this function pointer, we had to do property list create/free each time
we want to access type which is quadratic complexity :/
@touilleMan
Copy link
Member Author

Wow thanks for pointing this out @raulsntos !

I didn't know about this notify_property_list_changed signal and the need for it from Curve.
(to be honest this seems like a horribly complicated and error prone mechanism, but I'm pretty sure this is there for a reason 😄 )

So of course this PR is pointless if property can be updated dynamically !

(I'll create a new PR with only the revert of the change on GDNativeExtensionScriptInstanceInfo then)

@groud
Copy link
Member

groud commented Dec 1, 2022

According to the PR review meeting, this seems not needed anymore. Closing.

@groud groud closed this Dec 1, 2022
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.

None yet

6 participants