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

Fix documentation on exportable C# native arrays #10332

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

esainane
Copy link
Contributor

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.

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.
Copy link
Member

@raulsntos raulsntos left a 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.

@raulsntos raulsntos added enhancement topic:dotnet area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Nov 28, 2024
@esainane
Copy link
Contributor Author

esainane commented Nov 29, 2024

The Variant compatible types table lists:

  • All the types which can be used as an export directly.
  • Permitted C# native arrays of C# native types.
  • Godot.Vector2[], Godot.Vector3[], Godot.Vector4[], and Godot.Color[].

It doesn't list:

  • Arrays of types derived from GodotObject.
  • Godot.StringName[], Godot.NodePath[], or Godot.Rid[].

I don't think these have specific Packed*Array forms, either? These get marshaled tagged as MarshalType.GodotObjectOrDerivedArray, MarshalType.SystemArrayOfStringName, MarshalType.SystemArrayOfNodePath, MarshalType.SystemArrayOfRid, respectively. Arrays of types derived from GodotObject seem especially important to list as exportable, I think, though they're not themselves Variant compatible?

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/[Export]able, but not Variant compatible?

Either way, the C# exported properties page shouldn't be claiming that C# arrays are [Export]able if its element type is Variant compatible, since eg. bool[] isn't exportable, and so are C# arrays of the majority of the variant compatible types listed.

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:

  • Have an element type listed as Variant compatible.
  • Are not themselves listed as Variant compatible with their full type.
  • Are accepted in [MustBeVariant] parameters.
Type Has explicit AsType method on Variant Accepted as [Export] field Variant conversion (.From, .As...) works at runtime Property export works in editor
bool[]
Rect2[]
Vector2I[]
NodePath[] ☑️ AsSystemArrayOfNodePath ☑️ ☑️ ☑️
StringName[] ☑️ AsSystemArrayOfStringName ☑️ ☑️ ☑️
Rid[] ☑️ AsSystemArrayOfRid ☑️ ☑️ Kinda (too low level to meaningfully interact with)
GodotObject[] ☑️ AsGodotObjectArray ☑️ ☑️
Node2D[] AsGodotObjectArray<Node2D> ☑️ ☑️
Resource[] AsGodotObjectArray<Resource> ☑️ ☑️ (and the Resource filtering for arrays of Resource derived types is very nice to use)

I didn't expect GodotObject[] <-> Variant conversion to fail, given there is an explicit Variant.AsGodotObjectArray and I've used [Export]s of Resource derived types before.

Is [MustBeVariant] expected to be stricter? Is GodotObject[] <-> Variant conversion expected to work?

@raulsntos
Copy link
Member

It doesn't list:

  • Arrays of types derived from GodotObject.
  • Godot.StringName[], Godot.NodePath[], or Godot.Rid[].

You are correct. These are a weird exception that, in my opinion, makes it harder to reason about what C# types are [Export]-able. It's far easier to just say "all C# arrays that match a Packed Array", and so we usually ignore them for simplicity.

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 Godot.Collections.Array<T> and, in my opinion, that's what we should recommend for these element types.

Either way, the C# exported properties page shouldn't be claiming that C# arrays are [Export]able if its element type is Variant compatible, since eg. bool[] isn't exportable, and so are C# arrays of the majority of the variant compatible types listed.

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).

Is [MustBeVariant] expected to be stricter?

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 Variant.As<T>/Variant.From<T>.

Is GodotObject[] <-> Variant conversion expected to work?

Probably not, since there's no Packed Array equivalent. But if it works that's fine. I'd still recommend Godot.Collections.Array<GodotObject> though.


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.

@esainane
Copy link
Contributor Author

esainane commented Dec 3, 2024

Is [MustBeVariant] expected to be stricter?

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 Variant.As<T>/Variant.From<T>.

From earlier testing, bool[], Rect2[], Vector2I[], GodotObject[], Node2D[], and Resource[] are all accepted by [MustBeVariant], but will cause a runtime exception as soon as they are passed to Variant.From:

E 0:00:00:0989   VariantUtils.generic.cs:16 @ Godot.NativeInterop.godot_variant Godot.NativeInterop.VariantUtils+GenericConversion`1.ToVariant(T&): System.InvalidOperationException: The type is not supported for conversion to/from Variant: 'Godot.Resource[]'
  <C# Error>     System.InvalidOperationException
[...]

GodotObject[], Node2D[], and Resource[] all safely work as an [Export]ed field, though, and in particular I would really like Resouce[] (and derived) to continue to work.

Though perhaps a Godot.Collections.Array could serve just as well, without being too much of a pain to migrate?

@raulsntos
Copy link
Member

GodotObject[], Node2D[], and Resource[] all safely work as an [Export]ed field, though, and in particular I would really like Resouce[] (and derived) to continue to work.

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 T[] support where every array element type is supported as long as the element type is Variant-compatible.

Though perhaps a Godot.Collections.Array could serve just as well, without being too much of a pain to migrate?

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 Godot.Collections.Array.

@tetrapod00
Copy link
Contributor

This probably closes godotengine/godot#95358.

@esainane
Copy link
Contributor Author

Yes, and other points from this discussion should probably be spun off so they aren't lost?

Checking my understanding from here:

  • The documentation change in this PR could be simplified, saying something like: "Native C# arrays with a Packed*Array equivalent are supported and recommended. These are listed as their explicit array type in the Variant-compatbile types table. Other types may work, but will repeatedly incur expensive marshalling compared to using Godot.Collections.Array." Could drop the last sentence with a PR that introduces a diagnostic as a helpful nudge, though maybe it should still be present in documentation for versions of Godot where that won't be available?
  • New PR: Narrow the types [MustBeVariant] accepts to those that won't raise an exception with Variant.From. Check that this doesn't interfere with anything in the[Export] process.
  • New PR: Raise a new warning diagnostic with [Export] on a type that will work, but will repeatedly incur expensive marshalling, recommending the use of Godot.Collections.Array<T> instead.
  • New PR: Extend Common.ExportedMemberTypeIsNotSupportedRule's message format to include a trailing placeholder. Use this to suggest an alternative type when available, like when a Godot.Collections.Array<T> would work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement topic:dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants