-
-
Notifications
You must be signed in to change notification settings - Fork 3.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 documentation on exportable C# native arrays #10332
base: master
Are you sure you want to change the base?
Conversation
It's quite hard to tell what is and isn't permissible from the current description, and there are both false positives and negatives. This makes explicit what is and isn't permissible, and suggests a general workaround when the relevant diagnostic is raised for a native array type.
6324f68
to
8f2ea4f
Compare
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.
We wanted to keep this short and link to the Variant-compatible types documentation page that already explains exactly what types can be used, it even lists exactly all the types that are supported as exports.
I think trying to list all the compatible types here will make it harder to maintain the list, since now we'll have to remember to update the list in both places.
Specifically about C# arrays, the only ones supported are the ones that match a Godot Packed Array (see the full list in Variant-compatible types), so if you want to include the mention of Packed Arrays in the wording that may be enough and would avoid making an exhaustive list.
I would also consider that not being able to export multi-dimensional arrays is already clear enough from saying that only Packed Arrays are supported, but we can explicitly include that too in this section. Although, multi-dimensional arrays are probably not used that much.
The Variant compatible types table lists:
It doesn't list:
I don't think these have specific If the intention is to keep these details in one place, perhaps another table should be added under the Variant compatible types table for the types which are marshalable/ Either way, the C# exported properties page shouldn't be claiming that C# arrays are Perhaps it should say that permitted C# arrays are explicitly listed at the end of the table in the Variant-compatible types page? Testing a bit further. All of the following types:
I didn't expect Is |
You are correct. These are a weird exception that, in my opinion, makes it harder to reason about what C# types are To be honest, I don't think these array types should have ever been supported since they don't match a Packed Array and essentially that means you are just copying every element on marshalling which can be costly. The better approach would be to use
I agree. I think we could change the wording to be more like "only C# arrays that match a Packed Array". Since that was the intention of supporting C# arrays as a marshallable type (to be the C# equivalent to Godot's Packed Arrays).
It should check that the generic type argument matches one of the types explicitly listed in the Variant-compatible table. But it may also allow some of the C# array types you mentioned, since the purpose of this attribute is to ensure the generic type can be safely used in
Probably not, since there's no Packed Array equivalent. But if it works that's fine. I'd still recommend Also, keep in mind that in Godot 3 we used to support many C# collections including arrays of any element type. This is probably the reason why we still have some support for these C# array types. To be clear, we want to add support back but it has to be done in a way that doesn't break trimming/NativeAOT, we have some ideas in mind but will likely not happen until after the move to GDExtension. |
From earlier testing,
Though perhaps a |
If it works today we won't remove support, it would break compatibility. But as I said, I'd really like these arrays to migrate to a more generic
It should because that's how we're marshalling these today, so they are essentially the same thing except if you use C# arrays we have to copy every element of the array back and forth when marshalling and that's going to be more expensive than simply using |
This probably closes godotengine/godot#95358. |
Yes, and other points from this discussion should probably be spun off so they aren't lost? Checking my understanding from here:
|
It's quite hard to tell what is and isn't permissible from the current description, and there are both false positives and negatives.
This makes explicit what is and isn't permissible, and suggests a general workaround when the relevant diagnostic is raised for a native array type.