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

Add Span<> overloads to Godot methods which accept arrays #10012

Closed
Cassunshine opened this issue Jun 21, 2024 · 6 comments · Fixed by godotengine/godot#96329
Closed

Add Span<> overloads to Godot methods which accept arrays #10012

Cassunshine opened this issue Jun 21, 2024 · 6 comments · Fixed by godotengine/godot#96329
Milestone

Comments

@Cassunshine
Copy link

Describe the project you are working on

Procedurally generated 'Chill vibes' sandbox game with Marching Cubes terrain.

Describe the problem or limitation you are having in your project

A significant portion of Godot's functions require you to use specifically C# arrays, which for tasks that are able to re-use arrays (like building meshes or passing in triangles for colliders), requires many allocations over top of the arrays you're keeping around in C#.

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

Methods which take arrays should also have an overload that takes a Span<> of the same type.

Adding this would enable two neat (but admittedly niche) features for basically no complexity.

  1. You can pass arrays to Godot directly from the stack using stackalloc.
  2. You can re-use C# arrays, passing slices of them into Godot, rather than having to allocate a new array whenever you want to pass just some range of values.

Additionally, both Spans and Arrays in Godot are implicitly convertible to Variants. This feature would promote parity across the board. After all, if Variant, Godot's 'catch all', allow both, why don't Godot's functions themselves?

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

The changes required to make this possible are pretty simple:

  1. In the INSERT_ARRAY_FULL macro in bindings_generator.cpp, add an additional copy of the existing array type, but append "Span" to the end of the name, and change proxy_name to "Span<" #m_proxy_t ">" for the copy.
  2. In _populate_object_type_interfaces, after a method has been generated and added to the method list, make a copy with a new argument list.
  3. For each argument of the method, check if the type is an array type. If it is, change out the type for the span equivalent by appending "Span" to the type.cname of that argument.
    4: If any arguments were an array, add the method to the method list, thus generating the overload down the pipe.

This implementation has a few issues, listed below.

There are functions in Godot which follow this format:void Foo(int bar = 10, int[] baz = null)
There are two ways to handle these functions.

  1. You allow Span<> to be nullable in these case, initialize it to a default value of stackalloc type[0], and handle the unique case of accessing a nullable value type (i.e. baz.Value).
  2. Convert baz into a Span<int> and remove the nullability, move it before bar, thus slightly changing the function's signature in relation to both the documentation and the non-span version of the function.

Godot's C# documentation isn't built for overloads, which means that the documentation for overloaded functions will be worded as if it was just an array. This would just take some more work & research than what I've done.

Godot's Native -> Invoke system won't use the overloads. It's not really a major thing, but it is worth noting that Godot's InvokeGodotClassMethod won't know that the overloads exist. It finds functions by name, which doesn't have enough information to distinguish. The easiest thing to do is leaving the current implementation unchanged, so it always calls the array versions of the function.

Godot generates cached names of functions. This one is incredibly easy to fix, don't generate a cached name for functions that are overloaded.

I've cloned the Godot repo and added the steps above. Here's the main block of code, placed immediately after methods are pushed to the type's method list.

	// Generate span overload of method.
	// Everything is identical, but the function's array arguments are replaced with spans.
	MethodInterface imethod_copy;
	imethod_copy = imethod;
	imethod_copy.is_overload = true;
	imethod_copy.arguments = {};

	List<ArgumentInterface> arguments_with_default = {};

	bool has_arrays = false;
	for (ArgumentInterface argument : imethod.arguments) {
		String argument_type = argument.type.cname;

		if (argument_type.contains("Array") && argument_type.contains("Packed")) {
			ArgumentInterface arg_copy = argument;

			arg_copy.type.cname = argument_type + "Span";
			arg_copy.default_argument = "";

			imethod_copy.add_argument(arg_copy);
			has_arrays = true;
		} else {
			if (argument.default_argument != "") {
				arguments_with_default.push_back(argument);
			} else {
				imethod_copy.add_argument(argument);
			}
		}
	}

	if (has_arrays) {
		for (ArgumentInterface arg : arguments_with_default) {
			imethod_copy.add_argument(arg);
		}

		itype.methods.push_back(imethod_copy);
	}

As a final note, context for those unfamiliar with Godot's C# glue, when you pass an array into a Godot function, Godot will call into a generated icall method for that specific function. The icall does all the actual heavy lifting of converting the types and putting them into a list so that they can then be passed into native code. For arrays, it does this very simply. It implicitly casts the array to a span, and then gets a pinnable reference to that span.

This means that, effectively, Godot already internally uses Span any time you pass in an array. If you simply switch out the argument of the icall, the implicit cast is removed, but the functionality of the generated method is identical, since it was already expecting the array to be implicitly cast into a span anyway.

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

This enhancement is a more advanced feature, so it likely wouldn't see much use in most games. You can, after all, simply use ToArray() to turn a Span into an array. This does unfortunately defeat the purpose of a span.

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

This feature requires changes to the C# code generator, an add-on cannot change how the C# code interacts with the engine like this.

@Cassunshine
Copy link
Author

Footnote with a little context here, I'd also like to clarify that I have seen issue #7842.

While this proposal has a bit of overlap with #7842, the focus of that proposal is to change the way methods are called and restructure the C# side of the engine to allow for no-allocation. This proposal instead exist because I was frustrated that Godot's C# calls just cast an array to a span before being passed to native, but Godot doesn't have any way to just pass in a Span instead of converting a span into an array, only for it to be converted back into a span.

@Aman-Anas
Copy link

I have a similar usecase (marching cubes terrain) and I definitely find it odd that you can pass a Span<> into the dictionary used for generating an ArrayMesh and it works fine, but for some reason the ConcavePolygonShape3D's set_faces (which is weirdly enough, a property called Data in C#) will only take a normal array. Definitely seems inefficient.

@Delsin-Yu
Copy link

Delsin-Yu commented Jul 6, 2024

Is there any reason we add overload instead of just changing the array type argument to the corresponding span?

Compatibility?

@Aman-Anas
Copy link

I believe most normal arrays can be implicitly converted to span, so for most use cases changing to Span should be fine, instead of maintaining both versions. If there are methods that require the original array (resizing etc) maybe they could accept refs as well

@Cassunshine
Copy link
Author

Is there any reason we add overload instead of just changing the array type argument to the corresponding span?

Compatibility?

Compatibility. We could convert the existing functions to span instead of arrays, but the nullability issue that I mentioned would become more of a thing then, and all the existing documentation would need to be changed.

The overloads just seemed more practical.

I believe most normal arrays can be implicitly converted to span, so for most use cases changing to Span should be fine

Yeah, all C# arrays support implicit casts to spans, but there may be some edge cases where you can't rely on that which I'm forgetting.

@Aman-Anas
Copy link

Aman-Anas commented Oct 4, 2024

Small update: it does seem that you can use .call to call the GDScript version of the function and then just pass in your span. So for example instead of

chunkShape.Data = myArray; // Array of Vector3s

you can technically use

chunkShape.Call("set_faces", mySpan); // Span of Vector3s

and it seems to function identically (and does not throw an error). I'm not sure how major of a performance difference there is, though. But it does construct the collision shape just fine. More benchmarking is probably needed. (also using a StringName is probably a little better)

I guess this is possible since both spans and arrays are implicitly convertible to variant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants