-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Support typed arrays in the extension API dump #64249
Conversation
Your issue with the hint seems to be similar to what I have already encountered at some point too. I think it was never really fixed in the end but you can have a starting point with the posts there. |
@CedNaru Thanks! After reading that it's still not clear to me if the JSON should leave that as is. I think other places use the type name directly without the need to specify the hint so I think I'll trim the leading |
8b85412
to
df2698d
Compare
edit: removing that comment. I made a mistake |
Regarding "return_value": {
"type": "Array",
"array_type": "Node" # <-- This is added with this PR
}, Are we sure Also, are nested annotations allowed, e.g. Maybe the notation could be written more general: # Array<Node>
"return_value": {
"type": "Array",
"generic_types": ["Node"]
},
# Dictionary<String, *> (if that's ever a thing)
"return_value": {
"type": "Dictionary",
"generic_types": ["String", "Variant"]
},
# Array<Array<int>>
"return_value": {
"type": "Dictionary",
"generic_types": [{
"type": "Array",
"generic_types": ["int"]
]},
}, In essence, a type could then either be a string |
My idea was other classes would use a different JSON field, so for dictionaries it could be: "return_value": {
"type": "Dictionary",
"dictionary_key_type": "String",
"dictionary_value_type": "int"
}, The auto return_type = read_json_return_type();
if (return_type["type"] == "Array") {
String array_type = "Variant";
if (return_type.has("array_type")) {
array_type = return_type["array_type"];
}
// Do something with the Array type
} else if (return_type["type"] == "Dictionary") {
String key_type = "Variant";
String value_type = "Variant";
if (return_type.has("dictionary_key_type")) {
key_type = return_type["dictionary_key_type"];
}
if (return_type.has("dictionary_value_type")) {
value_type = return_type["dictionary_value_type"];
}
// Do something with the Dictionary type
}
I don't think they are supported in ClassDB at all because I'm using the I can see how this won't scale if we have more generic classes than I do like your idea better and kinda wanted to implement it like that at the beginning since it's more similar to how generic parameters work in C# reflection APIs but I thought it'd be better to keep it simple. |
The question is: do we anticipate that at some point, more classes may have generic parameters than just currently A Your deserialization example is procedural because C++ doesn't have JSON mapping frameworks. Languages that support some sort of reflection however, would likely use a declarative version, e.g. Rust: // Note: Option is like std::optional, meaning value can be absent
// Vec is like std::vector
#[derive(Deserialize)]
struct ReturnType {
type: String,
generic_args: Option<Vec<String>>,
} and with your proposal: #[derive(Deserialize)]
struct ReturnType {
type: String,
array_type: Option<String>,
dictionary_key_type: Option<String>,
dictionary_value_type: Option<String>,
... // more for others?
} I'm pretty sure C# offers something like that as well. I think both approaches are possible, let's maybe see what engine developers think about it. |
@Bromeon I opened #64469 with your proposed design. The C# bindings generator is implemented as a module so it can access |
regarding #64469 vs #54249, I'd rather have dedicated Generics support with variant is only informative (i.e. there is no guarantee the declared type will be respected at runtime). On top of that generic typing support for custom classes is a complex topic (see for instance how much energy Python community put into this). So I don't think support for deep nested generic types ( So I'd much rather take the YAGNI pragmatic approach there 😃 |
See also #65817. |
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 agree with following YAGNI here - there are no concrete plans for further generics support so we should keep it simple/explicit for what TypedArray provides (which is really still just an array of Variants with a type hint).
Needs rebase. |
I'm pretty sure this is superseded by #65817. This PR's goal was to add the type of the array's elements as a separate property so consumers of "return_value": {
"type": "Array",
"array_type": "Node"
} With #65817 the "return_value": {
"type": "typedarray::Node"
} With this PR, parsing the type could be implemented with this pseudo-code: auto return_type = read_json_return_type();
if (return_type["type"] == "Array") {
String array_type = "Variant";
if (return_type.has("array_type")) {
array_type = return_type["array_type"];
}
// Do something with the Array type
} With #65817, consumers would need something like this instead: auto return_type = read_json_return_type();
if (return_type["type"] == "Array") {
String array_type = "Variant";
// Do something with the Array type
} else if (return_type["type"].starts_with("typedarray::")) {
String array_type = return_type["type"].substring("typedarray::".size());
// Do something with the Array type
} And existing code that only handles Since #65817 has already been merged, we already broke compatibility, so that ship has already sailed but it's fine since we're still in beta. The only difference between this PR and #65817 is that I slice the hint string: As opposed to:
I do this because in some cases it uses the syntax godot/editor/editor_properties_array_dict.cpp Lines 593 to 594 in df2698d
An example of a property that uses this syntax: godot/scene/resources/font.cpp Line 1019 in a8a8819
Which currently results in this JSON: {
"type": "typedarray::24/17:Font"
} |
We could rediscuss this in next week's GDExtension meeting and see if we're all happy with the merged option or if we want to change it further. I think it's still OK to break compatibility if we need to to get a solid base for 4.x. |
Glad to hear it's implemented 🙂 I'm not too convinced the If there were a small spec of this internal protocol, things would already be quite a bit easier. |
Closing for now:
|
Summary
Currently we're using
{"type: "typedarray::Node"}
to represent a typed array.This PR changes this to:
{"type": "Array", "array_type": "Node"}
.Changing the value of
type
breaks compatiblity.Details
Adds optional
array_type
field to specify the type of the array element whentype
isArray
(for example inNode::get_children
).The JSON for
Node::get_children
with this PR is now:Alternative design