Standardize Object ptrcall encoding on Object **

[dsnopek]

This is an attempt to implement "possible solution nr 2" from godotengine/godot-cpp#1119

That issue is about problems in godot-cpp due to Godot using a different encoding of Object in two different situations:

• When calling normal methods, they are encoded as Object *
• When calling virtual GDExtension methods, they are encoded as Object **

This PR attempts to standardize on Object ** per @vnen's suggestion:

For an object, VariantInternal::get_opaque_pointer() returns a plain Object *. Hence, we're encoding an Object * as Object *.

This seems like a problem in this method itself, it should return an Object **. The thing is that this is meant to return a pointer to the value inside the Variant. If it's a basic type, like int it returns an int *. For objects, the base type is Object * which is what is stored in Variant, so the method should return a pointer to this value, which is an Object **.

I do take the blame for this since I implemented the VariantInternal::get_opaque_pointer() initially. It should just call VariantInternal::get_object(), which does return an Object ** (get_object() itself was added later, that's why it's not called there). But I probably did this way because it is what worked.

[snip]

If we change this, I would go with always using Object **, for the same reasoning as Variant::get_opaque_pointer() I mention above. Changing PtrToArg would indeed require changes in GDScript and C#, as this affect all calls, not only virtual ones.

It appears to work in very limited testing? And, when paired with PR godotengine/godot-cpp#1123, it seems to fix the godot-cpp issues (both the original issues and the regressions).

However, I suspect this breaks some things I'm not aware of and/or didn't try - in particular, this probably has an impact on C#, which I didn't test at all. My limited testing of GDScript seems to work, but it's possible I'm just not testing in a way that would trigger issues.

UPDATE: I have now tested C# a little, and it seems to work in limited testing with the small change to C# bindings generator.

In any case, please let me know what you think :-)

Fixes #61967

[dsnopek]

I've done some more testing with this, and GDScript still appears to be working fine. (I did realize that I forgot to update PtrToArg<Ref<T>> but updating that fixed the one crash I encountered.)

I built the C# version of Godot and can confirm that these changes totally break C# - it crashes immediately. :-) I spent a little time looking at the C# code, and I think it's using GodotObject.GetPtr() to encode objects for pointer calls. Maybe a new method could be added to return an Object ** and that could be used in ptrcalls instead? But I'm really out of my depth here, and I have no idea if that makes sense or how to do it even if it does. :-)

@godotengine/dotnet: Is it possible to change C# so that it makes ptrcalls with objects encoded as Object **? How messy would that be? What effect would that have on compatibility?

[dsnopek]

Actually, I seem to have gotten C# working, at least in my super simplistic testing :-)

I'm going to take this out of draft, since it does at least appear to be working, and to try and get some more feedback!

[Bromeon]

Cool to see some consistency improvements in this regard! 🙂

Maybe worth noting that a similar problem is present for variant-to-object conversions, as mentioned by @saki7 in the godot-cpp thread: #61967

Also, general question regarding these fixes: would it be possible to incorporate them during the 4.0 -> 4.1 GDExtension API change? That would allow an extension to use the old ("inconsistent") convention when compiled against a Godot 4.0.x version.

[dsnopek]

Discussed at the GDExtension meeting: this needs lots of testing, but if it actually works then (among the people at the meeting) we think this is probably the way to go. It would be great to have input from @vnen

@Bromeon is going to test with the Rust bindings soon

[vnen]

This is the best approach IMO, as I've mentioned in other discussions.

I tested this with GDScript ptrcall and it works fine. I can't say about the C# side.

[saki7]

This approach is the best choice in light of the discussion in godotengine/godot-cpp#1119. I think this would remove the "inconsistency" on encodings, thus we can hopefully avoid regressions like those we had been repeatedly experiencing in past few months.

I haven't tested extensively, but this approach works on my game's codebase which is utilizing a limited subset of GDExtension. (My application is based on 100% godot-cpp, no GDScript is involved.)

That said, even if this commit would fail on unexpected corner cases, I believe we should stick on this Object** protocol.

[saki7]

@akien-mga Could you endorse this PR for merging into 4.1? If there are any objections, we need to process the discussion as soon as possible as this PR will define a stable ABI which affects all GDExtension bindings.

[akien-mga]

Yes, we discussed this in #gdextension and agreed to merge this for 4.1, but after the imminent 4.1 beta 1 release, so we have enough time to do downstream changes and potential bugfixes before beta 2.

[akien-mga]

Thanks everyone! Great work :)