Fix bindings of PhysicsServer3DRenderingServerHandler

[mihe]

It's currently impossible to implement SoftBody3D from a GDExtension, due to the set_vertex, set_normal and set_aabb methods of PhysicsServer3DRenderingServerHandler not being bound, which need to be called in order to update the rendered positions of each particle/vertex, as seen in the Godot Physics implementation here:

godot/servers/physics_3d/godot_soft_body_3d.cpp

Lines 154 to 165 in 75de1ca

	const uint32_t vertex_count = map_visual_to_physics.size();
	for (uint32_t i = 0; i < vertex_count; ++i) {
	const uint32_t node_index = map_visual_to_physics[i];
	const Node &node = nodes[node_index];
	const Vector3 &vertex_position = node.x;
	const Vector3 &vertex_normal = node.n;
	p_rendering_server_handler->set_vertex(i, &vertex_position);
	p_rendering_server_handler->set_normal(i, &vertex_normal);
	}
	p_rendering_server_handler->set_aabb(bounds);

It's also impossible to bind set_vertex and set_normal with the interface they have currently, as they (for some reason) take a void* to a Vector3 rather than just a const Vector3&.

This PR addresses both of these problems by changing these methods to take const Vector3& and then binding all the relevant methods.

[akien-mga]

You need to run --doctool to generate the class reference for the changed/added bindings.

I expect there will also be a CI issue reported for breaking the compatibility in the extension API. Here it's arguably breaking something that apparently wouldn't work, but we're trying to preserve binary compatibility as far as possible. It should be possible to add compat methods for the old prototypes, so that it generates hashes that match the ones in 4.1. See #80168 for an example of adding compat methods, and listing the prototype change as an intentional change in misc/extension_api_validation/4.1.stable.expected.

[mihe]

You need to run --doctool to generate the class reference for the changed/added bindings.

Right, I forgot about that.

It should be possible to add compat methods for the old prototypes

The methods being broken are GDVIRTUAL methods, so bind_compatibility_method isn't applicable here from what I can tell. None of the other *.compat.inc files seem to have dealt with virtual methods previously either. Just adding these as separate overloads does not seem to be something that the GDVIRTUAL* macros allow either, so I'm not quite sure what to do here.

[mihe]

I suppose I could leave the virtual methods (_set_vertex etc.) untouched and just let the concrete methods (set_vertex etc.) use the const Vector3& interface. It'll look real strange, but wouldn't break anything at least.

[mihe]

There, no breaking changes, but the extension interface for PhysicsServer3DRenderingServerHandler now looks like:

class PhysicsServer3DRenderingServerHandler : public Object {
public:
    void set_vertex(int32_t vertex_id, const Vector3 &vertex);
    void set_normal(int32_t vertex_id, const Vector3 &normal);
    void set_aabb(const AABB &aabb);
    virtual void _set_vertex(int32_t vertex_id, const void *vertices);
    virtual void _set_normal(int32_t vertex_id, const void *normals);
    virtual void _set_aabb(const AABB &aabb);
};

So anyone wanting to implement their own SoftBody3D-like node from a GDExtension would have to deal with the const void*, same as before, but when invoking these methods (i.e. the non-virtual ones) from a GDExtension-based physics server implementation of SoftBody3D, you'll use the more reasonable const Vector3&.

[akien-mga]

Well before settling on keeping the const void * for the virtuals, we should discuss whether the compat breakage isn't preferable, and if we can provide compatibility bindings somehow (possibly by adding more functionality to handle compatibility, this is still WIP). CC @godotengine/gdextension

[dsnopek]

With virtual methods, I think the best we could do currently for compatibility is adding new virtual methods under a new name (rather than changing the signature of the original virtual method). Then Godot could check if the newer method is implemented (via GDVIRTUAL_IS_OVERRIDDEN_PTR()) and if so, call that one; if not, fallback on calling the old virtual method.

For the background on why: with virtual methods, Godot is asking the GDExtension for a function pointer for the virtual method by name, it's not passing any information about the expected method signature, like we do when GDExtension asks Godot for a function pointer. But even if it did, there isn't much the GDExtension can do with that information: it either has a method with the right signature or it doesn't.

I could imagine a system where Godot asks the GDExtension for a function pointer AND some information about the methods signature, so that Godot could say "if the GDExtension gives me a pointer with the old signature, call it the old way; if it has the new signature, call it the new way." Creating such a system is possible, but it would be a big change from what we have right now.

All that said, the current signature on these virtual methods is so weird and broken, an argument could be made that the compatibility breakage is perhaps OK?

[mihe]

All that said, the current signature on these virtual methods is so weird and broken, an argument could be made that the compatibility breakage is perhaps OK?

I would tend to agree. I have a hard time imagining that anyone is using this interface in its current form outside of the SoftBody3D implementation in Godot itself.

If we assume that nobody is actually using this currently, would there be any consequences other than technically breaking binary compatibility? @akien-mga mentioned something about hashes, but I'm not entirely sure what that means, or what the consequences would be from it changing.

[Bromeon]

All that said, the current signature on these virtual methods is so weird and broken, an argument could be made that the compatibility breakage is perhaps OK?

If we assume that nobody is actually using this currently, would there be any consequences other than technically breaking binary compatibility?

I agree, should be OK to break. But please add to the docs of each method that Godot < 4.2 accepted const void* instead, otherwise it's very hard for a user or binding developer to reconstruct what happened. So if someone does want 4.1 compatibility, they have the chance to do so.

[mihe]

Seeing as how the general consensus seems to be that a breakage is acceptable I went ahead and did just that.

I also added the breakages to misc/extension_api_validation/4.1-stable.expected as requested for the original revision, as well as documentation of all the methods along with notes about the change of parameter types as requested by @Bromeon.

Let me know if there's anything else I should do.

[dsnopek]

From the perspective of GDExtension, this looks good to me!

[akien-mga]

Thanks!