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

[.NET] Generate ReadOnlySpan<T> Overloads for GodotSharp APIs #96329

Conversation

Delsin-Yu
Copy link
Contributor

@Delsin-Yu Delsin-Yu commented Aug 30, 2024

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:

@Delsin-Yu Delsin-Yu requested a review from a team as a code owner August 30, 2024 13:24
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from 49f1c2c to 0aa2c07 Compare August 30, 2024 13:27
@AThousandShips AThousandShips added this to the 4.x milestone Aug 30, 2024
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from 0aa2c07 to b4d916a Compare August 30, 2024 13:55
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.

This is a massive compatibility break, so it's not acceptable. We could do it as long as it's only adding span overloads while keeping the array overloads, but only for non-virtual methods and spans should never be returned either.

@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from b4d916a to 34f85da Compare August 30, 2024 15:06
@Delsin-Yu Delsin-Yu marked this pull request as draft August 30, 2024 15:07
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from 34f85da to 6bdbeb7 Compare August 30, 2024 16:45
@Delsin-Yu Delsin-Yu changed the title [.Net & BindingGenerator] Replace the Usage of C# Array with ReadOnlySpan<T> in APIs [.Net & BindingGenerator] Generate ReadOnlySpan<T> Overloads for GodotSharp APIs Aug 30, 2024
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch 2 times, most recently from 51d5c9d to ba36a45 Compare August 30, 2024 16:48
@dotlogix
Copy link
Contributor

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

@Delsin-Yu
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@dotlogix
Copy link
Contributor

dotlogix commented Aug 30, 2024

@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 instead of a T[] but instead of creating an array and return it, take a Span and write to that instead.

@Delsin-Yu
Copy link
Contributor Author

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.

@dotlogix
Copy link
Contributor

dotlogix commented Aug 30, 2024

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)

@Delsin-Yu
Copy link
Contributor Author

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.

@dotlogix
Copy link
Contributor

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.

@Delsin-Yu Delsin-Yu marked this pull request as ready for review August 30, 2024 17:51
@Delsin-Yu Delsin-Yu requested a review from raulsntos August 30, 2024 17:51
@Delsin-Yu
Copy link
Contributor Author

Request removing breaks_compact & feature_proposal.

@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch 4 times, most recently from a7b039f to c472476 Compare August 31, 2024 12:56
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch 2 times, most recently from ceb7079 to c2b2963 Compare October 5, 2024 19:23
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from c2b2963 to 5db07f9 Compare October 7, 2024 09:30
@Delsin-Yu Delsin-Yu requested a review from raulsntos October 7, 2024 11:10
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch 2 times, most recently from 540a55b to dedddee Compare October 7, 2024 18:42
@Delsin-Yu Delsin-Yu requested a review from raulsntos October 7, 2024 20:21
@Delsin-Yu
Copy link
Contributor Author

Delsin-Yu commented Oct 7, 2024

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 Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from 06ade5f to eb00fd7 Compare October 7, 2024 22:10
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.

Looks good to me, thanks.

I have compared the generated bindings with the ones we were generating before this PR, and the changes look good. It also doesn't look like it breaks compatibility, I double-checked with APICompat.

@raulsntos raulsntos changed the title [.Net & BindingGenerator] Generate ReadOnlySpan<T> Overloads for GodotSharp APIs [.NET] Generate ReadOnlySpan<T> Overloads for GodotSharp APIs Oct 8, 2024
@raulsntos raulsntos modified the milestones: 4.x, 4.4 Oct 8, 2024
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from eb00fd7 to 5bd5646 Compare October 8, 2024 07:53
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch 2 times, most recently from f39861c to 98e1b82 Compare October 9, 2024 00:41
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from 048f6c0 to 799931b Compare October 9, 2024 15:09
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from 799931b to 9a3d085 Compare October 9, 2024 15:13
Co-authored-by: Raul Santos <raulsntos@gmail.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@Delsin-Yu Delsin-Yu force-pushed the dotnet-replace-array-with-ros-in-binding-apis branch from 9a3d085 to d4695e8 Compare October 9, 2024 15:14
@Repiteo Repiteo merged commit 147ccfe into godotengine:master Oct 10, 2024
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 10, 2024

Thanks!

@Delsin-Yu Delsin-Yu deleted the dotnet-replace-array-with-ros-in-binding-apis branch October 11, 2024 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Span<> overloads to Godot methods which accept arrays
6 participants