godot icon indicating copy to clipboard operation
godot copied to clipboard

[.Net & BindingGenerator] Generate `ReadOnlySpan<T>` Overloads for GodotSharp APIs

Open Delsin-Yu opened this issue 1 year ago • 13 comments

By adding ReadOnlySpan<T> API overloads, this PR aims to make the overall GodotSharp API more flexible by addressing the need for a dedicated data transaction T[] when making API calls.

Using the example from this proposal, the original RenderingServer.MultimeshSetBuffer

public static void MultimeshSetBuffer(Rid multimesh, float[] buffer)

will have an additional overload that takes ReadOnlySpan<float>:

public static void MultimeshSetBuffer(Rid multimesh, ReadOnlySpan<float> buffer)

Bugsquad edit:

  • Closes https://github.com/godotengine/godot-proposals/issues/10012.

Delsin-Yu avatar Aug 30 '24 13:08 Delsin-Yu

Does this also include to add overloads which take a buffer span instead of returning an Array?

Currently one of my biggest performance issues are tied to GetBufferData in the RenderingDevice.BufferGetData. Internally most of these methods act on a pinned array which is created every single time you call this method.

For RenderingDevice.BufferGetData the array is a byte[] which is incompatible with most of the other Apis so you have to allocate yet another array (float[]) and memory copy again. Doing this allows to use pooled arrays / instances and avoid allocations alltogether

dotlogix avatar Aug 30 '24 17:08 dotlogix

I have modified my PR to fit the compatibility requirements; now that span-related APIs are generated as overloads, no spans are returned by public APIs.

Delsin-Yu avatar Aug 30 '24 17:08 Delsin-Yu

@dotlogix In my original unmodified PR, yes, most arrays are directly replaced into Spans, including the return value. But for compatibility reasons, we can't do this, not on the main branch.

Delsin-Yu avatar Aug 30 '24 17:08 Delsin-Yu

@dotlogix In my original unmodified PR, yes, most arrays are directly replaced into Spans, including the return value. But for compatibility reasons, we can't do this, not on the main branch.

Guess you forgot to revert: GD.VarToBytes Compat.*

I don't mean return a ReadOnlySpan<T> instead of a T[] but instead of creating an array and return it, take a Span<T> and write to that instead.

dotlogix avatar Aug 30 '24 17:08 dotlogix

Guess you forgot to revert: GD.VarToBytes

Good catch, thanks!

I don't mean return a ReadOnlySpan instead of a T[] but instead of creating an array and return it take a Span and write to that instead.

Oh, do you mean to return a Span for developers to write into, like a temporary buffer? As you mentioned, that would work by utilizing an array pool, but it would be challenging to manage the lifetime of the rented arrays. It sounds like a job for a custom struct-based, disposable array wrapper, but any of it is beyond the compatibility requirements. You can create a customized fork based on 34f85daa9a798be82d7839acfef4a662cdb6dd1d if you wish.

Delsin-Yu avatar Aug 30 '24 17:08 Delsin-Yu

Guess you forgot to revert: GD.VarToBytes

Good catch, thanks!

I don't mean return a ReadOnlySpan instead of a T[] but instead of creating an array and return it take a Span and write to that instead.

Oh, do you mean to return a Span for developers to write into, like a temporary buffer? As you mentioned, that would work by utilizing an array pool, but it would be challenging to manage the lifetime of the rented arrays. It sounds like a job for a custom struct-based, disposable array wrapper, but any of it is beyond the compatibility requirements. You can create a customized fork based on 34f85da if you wish.

No I mean in addition to this:

public byte[] BufferGetData(Rid buffer, uint offsetBytes = 0u, uint sizeBytes = 0u)

Have an overload like this:

public void BufferGetData(Rid buffer, Span<byte> target, uint offsetBytes = 0u, uint sizeBytes = 0u)

dotlogix avatar Aug 30 '24 17:08 dotlogix

No I mean in addition to this:

public byte[] BufferGetData(Rid buffer, uint offsetBytes = 0u, uint sizeBytes = 0u)

Have an overload like this:

public void BufferGetData(Rid buffer, Span<byte> target)

I don't think this is possible. The C# binding layer does not contain Godot logic; the binding generator creates the APIs you see.

Delsin-Yu avatar Aug 30 '24 17:08 Delsin-Yu

No I mean in addition to this:

public byte[] BufferGetData(Rid buffer, uint offsetBytes = 0u, uint sizeBytes = 0u)

Have an overload like this:

public void BufferGetData(Rid buffer, Span<byte> target)

I don't think this is possible. The C# binding layer does not contain Godot logic; the binding generator creates the APIs you see.

Hm that's quite unfortunate. I would know how to implement that in C# pretty easily, but I just don't understand the binding generator (C++) so I can't just add it myself.

dotlogix avatar Aug 30 '24 17:08 dotlogix

Request removing breaks_compact & feature_proposal.

Delsin-Yu avatar Aug 30 '24 17:08 Delsin-Yu

Did the stance on spans change? I saw this a bit ago. image

TinkerWorX avatar Sep 30 '24 17:09 TinkerWorX

@TinkerWorX From my understanding, these span APIs merely remove the unnecessary allocations on the C# side; spans are marshaled to godot-packed-arrays just as they were in array APIs.

Delsin-Yu avatar Sep 30 '24 17:09 Delsin-Yu

@TinkerWorX From my understanding, these span APIs merely remove the unnecessary allocations on the C# side; spans are marshaled to godot-packed-arrays just as they were in array APIs.

Alright, I was just not sure, maybe Juan meant C++ spans on the engine side.

TinkerWorX avatar Sep 30 '24 18:09 TinkerWorX

I should probably generate ROS overloads for vararg methods as well.

Edit: Done, vararg methods now have overloads like:

public Error Rpc(StringName method, params Variant[] @args)
public Error Rpc(StringName method, ReadOnlySpan<Variant> @args)

public Variant New(params Variant[] @args)
public Variant New(ReadOnlySpan<Variant> @args)

public void CallGroupFlags(long flags, StringName group, StringName method, params Variant[] @args)
public void CallGroupFlags(long flags, StringName group, StringName method, ReadOnlySpan<Variant> @args)

Delsin-Yu avatar Oct 07 '24 20:10 Delsin-Yu

Thanks!

Repiteo avatar Oct 10 '24 23:10 Repiteo

The only drawback is that RenderingDevice.TextureCreate is not affected.

Redwarx008 avatar Sep 13 '25 09:09 Redwarx008