`Span<float>` and `Memory<T>` overloads for RenderingServer.MultimeshSetBuffer

thygrrr commented 9 months ago

Describe the project you are working on

fennecs, a pure C# ECS that works with Godot.

Describe the problem or limitation you are having in your project

A common use case is to pass component data into a Multimesh for rendering by Godot. With RenderingServer, this is done by passing a float[]. (internally, this gets marshalled and copied into native memory)

My ECS queries preferably pass Span<T> and Memory<T> to the user. These aren't System.Array.

Calling .ToArray() on a Spanadds a full, redundant memory copy and heap allocation operation. (the data is usually too large for any sort of stackalloc, and originates from the heap anyway)

In a more commonplace use case (which also applies to ECS), it is somewhat cumbersome to cast a Transform3D[] or Span<Transform3D> into a float[] (because of the steps involved, and exacerbated by struct layout incompatibility); and the same is true for some hypothetical contiguous array of structs extended with the optional data for the Multimesh.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Suggested overloads:

public static void MultimeshSetBuffer(Rid multimesh, Span<float> buffer)   //basic requirement
public static void MultimeshSetBuffer<T>(Rid multimesh, Memory<T> buffer)  //marry me please

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

This...

... will become this (note the removed array allocation) ...

... and most elegantly, with the Memory overload (would internally use MemoryHandle Memory.Pin to get the pointer for the native call, and could use Memory.Length and sizeof(T) to determine length of the PackedArray):

If this enhancement will not be used often, can it be worked around with a few lines of script?

The extra memory allocation has a significant impact that, to my knowledge, cannot be worked around at the moment. Because the internal API of Multimesh is so strict, arrays of larger length (with unused elements at the end) cannot be submitted, so a workaround using ArrayPool or something similar just isn't feasible, because that only returns arrays of a minimum size, not an exact size. Going through Span<float>s is the right way here, and the backing Native interop API already uses them. They are just not exposed to the user of GodotSharp.

Is there a reason why this should be core and not an add-on in the asset library?

This should be part of the C# module / extension. I think it primarly affects the glue code and a few lines of code within earshot of Godot.Marshaling.

thygrrr commented 9 months ago

As a side remark on the subject matter - I think it's unfortunate that Transform3D's memory layout doesn't match what MeshStorage actually uses. This means client code needs to maintain their own Transform3D type struct (in my case, Matrix 4x3). This is true for C# as well as GDScript if they wish to be using RenderingServer.MultimeshSetBuffer.

And more impactfully, this incongruency is the reason Godot engine code itself needs to perform a lot of these operations each time MeshStorage is accessed (I think it's some sort of matrix transposition/concatenation):

... for instance, in:

void MeshStorage::skeleton_bone_set_transform and skeleton_bone_get_transform
void MeshStorage::_multimesh_re_create_aabb
Transform2D MeshStorage::multimesh_instance_set_transform_2d and multimesh_instance_get_transform_2d
Transform3D MeshStorage::multimesh_instance_set_transform_3d and multimesh_instance_get_transform_3d
(and various other MeshStorage API)

thygrrr commented 9 months ago

Okay, the scope of this is:

BindingsGenerator does not have a concept of overloads, and makes the unfortunate decision of mapping Godot.NativeInterop.godot_packed_float32_array / PackedFloat32Array to float[] instead of Span<float>.

It might be useful to view this as two changes rolled into one here:

a) make the generated code for the Packed Array types take Span instead (it currently creates its own Span, because the internal marshaling method already operates on a Span)😢

    internal static unsafe void godot_icall_2_902(
      IntPtr method,
      IntPtr ptr,
      Rid arg1,
      float[] arg2)
    {
      if (ptr == IntPtr.Zero)
        throw new ArgumentNullException(nameof (ptr));
      godot_packed_float32_array packedFloat32Array = Marshaling.ConvertSystemArrayToNativePackedFloat32Array((Span<float>) arg2);
      try
      {
        // ISSUE: untyped stack allocation
        IntPtr num = __untypedstackalloc(checked (new IntPtr(2) * sizeof (void*)));
        *(IntPtr*) num = (IntPtr) &arg1;
        *(IntPtr*) (num + sizeof (void*)) = (IntPtr) &packedFloat32Array;
        void** p_args = (void**) num;
        NativeFuncs.godotsharp_method_bind_ptrcall(method, ptr, p_args, (void*) null);
      }
      finally
      {
        packedFloat32Array.Dispose();
      }
    }

this would simply become (you can see there already is a cast to Span there, so the natural signature of the function ought to indeed take a Span parameter):

5c5
<       float[] arg2)
---
>       Span<float> arg2)  // <--- change 1
9c9
<       godot_packed_float32_array packedFloat32Array = Marshaling.ConvertSystemArrayToNativePackedFloat32Array((Span<float>) arg2);
---
>       godot_packed_float32_array packedFloat32Array = Marshaling.ConvertSystemArrayToNativePackedFloat32Array(arg2);  // <--- change 2

b) provide a convenience/compatibility overload to the porcelain functions, which should be easy, possibly trivial, for the code generator to output

 // new signature
 public static void MultimeshSetBuffer(Rid multimesh, Span<float> buffer)
    {
      NativeCalls.godot_icall_2_902(RenderingServer.MethodBind77, GodotObject.GetPtr((GodotObject) RenderingServer.Singleton), multimesh, buffer);
    }

 // convenience overload
 public static void MultimeshSetBuffer(Rid multimesh, float[] buffer) =>
      NativeCalls.godot_icall_2_902(RenderingServer.MethodBind77, GodotObject.GetPtr((GodotObject) RenderingServer.Singleton), multimesh, buffer.AsSpan());

thygrrr commented 9 months ago

I've tried to directly patch the GodotSharp project to change the method signatures and overloads, and the technique works as expected, but is not maintainable since it relies on edits to generated code.

Upgrading the code generator itself seems tricky, as the code is quite unreadable and poorly understood by me. (kind of a running theme with code generators... not your fault)

godotengine / godot-proposals