godot-docs icon indicating copy to clipboard operation
godot-docs copied to clipboard

Fix documentation on exportable C# native arrays

Open esainane opened this issue 1 year ago • 6 comments

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.

  • Bugsquad edit, closes: https://github.com/godotengine/godot/issues/95358

esainane avatar Nov 28 '24 16:11 esainane

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[] :x: :x: :x: :x:
Rect2[] :x: :x: :x: :x:
Vector2I[] :x: :x: :x: :x:
NodePath[] :ballot_box_with_check: AsSystemArrayOfNodePath :ballot_box_with_check: :ballot_box_with_check: :ballot_box_with_check:
StringName[] :ballot_box_with_check: AsSystemArrayOfStringName :ballot_box_with_check: :ballot_box_with_check: :ballot_box_with_check:
Rid[] :ballot_box_with_check: AsSystemArrayOfRid :ballot_box_with_check: :ballot_box_with_check: Kinda (too low level to meaningfully interact with)
GodotObject[] :ballot_box_with_check: AsGodotObjectArray :ballot_box_with_check: :x: :ballot_box_with_check:
Node2D[] :heavy_minus_sign: AsGodotObjectArray<Node2D> :ballot_box_with_check: :x: :ballot_box_with_check:
Resource[] :heavy_minus_sign: AsGodotObjectArray<Resource> :ballot_box_with_check: :x: :ballot_box_with_check: (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?

esainane avatar Nov 29 '24 05:11 esainane

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.

raulsntos avatar Dec 03 '24 02:12 raulsntos

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?

esainane avatar Dec 03 '24 03:12 esainane

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.

raulsntos avatar Dec 10 '24 07:12 raulsntos

This probably closes https://github.com/godotengine/godot/issues/95358.

tetrapod00 avatar Dec 16 '24 00:12 tetrapod00

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.

esainane avatar Dec 16 '24 04:12 esainane