Add functions for non-ptr style virtual calls in GDExtension

[fuzzybinary]

This adds two functions to GDExtensionClassCreationInfo2 that allow for developers to supply a generic virtual call function along with user data to be sent to that call.

If get_virutal_call_data_func is not null, extensions call this function to get user data to pass to a supplied call_virtual_func. Both must be provided is one is provided.

If get_virtual_call_data_func is null, Godot falls back to the old get_virtual_func logic.

Fixes #63275

[dsnopek]

Thanks! Overall, I personally think this is a good approach.

This is something that will definitely need a bunch of discussion, though, perhaps at a meeting. It'd also be really great if we could get folks working on different language bindings (particularly those that were having trouble with the old way) to see if it would work for them.

[dsnopek]

Thanks, this is looking good!

There's some CI failures from build errors in unrelated code. Rebasing on the latest master may fix that?

[fuzzybinary]

This should now be ready. I've moved the new members into the GDExtensionClassCreationInfo2 struct.

@AThousandShips I think I fixed the uninitialized ptr return variable. Can you take another look and see if it makes sense to you?

[AThousandShips]

Will take a look!

[dsnopek]

This is looking great to me! I only have the one naming related note.

I think this should get some more feedback from other GDExtension team members, though, perhaps discussing it at the next meeting.

Also, the CI is failing on building godot-cpp, but I don't think that we can fix that. :-/ It's a circular dependency: godot-cpp will need to be updated for this change, but godot-cpp depends on the Godot change.

I think this will come up again as we make further changes to the new structs before Godot 4.2. I'm not sure what we can do about that? We could maybe have the Godot CI use the 4.1 branch of godot-cpp instead of master?

[AThousandShips]

I think the code paths look good now and the CI errors have gone away

[fuzzybinary]

Renamed the var, squashed and rebased on master. Should be good for review pending a decision on the godot-cpp build errors.

[dsnopek]

PR #81238 should fix the CI. If/when it's merged, this one will just need a rebase.

[fuzzybinary]

Rebased -- not sure if the CI error is something on my end?

[fuzzybinary]

Renamed variables and rebased onto master.

[dsnopek]

@fuzzybinary:

So even if this is more complicated it is for sure giving me higher performance than if we were just sending in the name.

Ah, thanks for sharing how you're using this in your Dart bindings! If you're able to take advantage of the API in order to cache pointers to the underlying Dart functions, I think that's a solid justification for this approach. It allows calls to virtual functions defined in Dart to avoid a lookup on every call, just like with the C++ binding. (Personally, I don't want any language bindings forced to be "second class citizens" just because they can't easily generate new functions with C linkage)

Renamed variables and rebased onto master.

Thanks! The suggestion from the meeting was to also rename the p_userdata argument to p_class_userdata on the other related function pointer typedefs as well, for consistency. So, also on GDExtensionClassCreateInstance and GDExtensionClassFreeInstance too.

[fuzzybinary]

👍 I can do that, I was thinking it would be better to keep these changes localized to virtuals but I can change it in this PR if we want.

[dsnopek]

Thanks! This is looking great to me :-)

[Bromeon]

Few things to change, mostly clarify the semantics a bit and guide the user to choose between old vs. new approach.

[dsnopek]

Looking good to me!

[Bromeon]

Two smaller things, see comments.

Side note, the names get_virtual_call_data and call_virtual_with_data are a bit difficult to memorize due to different order of the terms. Something like get_virtual_data and call_virtual_data might be a bit easier, although possibly less expressive. It's a tiny issue though and hardly relevant in practice, so if others are OK with the current naming, I won't bikeshed here 🙂

[fuzzybinary]

Two smaller things, see comments.

Side note, the names get_virtual_call_data and call_virtual_with_data are a bit difficult to memorize due to different order of the terms. Something like get_virtual_data and call_virtual_data might be a bit easier, although possibly less expressive.

Yeah, while easier, it's not actually descriptive of what's happening. Although harder to memorize with the reordering, I do think the parallelism of "get_call_data" -> "call_with_data" is helpful.

I'll rename the structure member to be more in line.

[fuzzybinary]

@Bromeon Took one more stab at the documentation. Let me know if that's clearer.

[Bromeon]

Mostly good, this is a tiny bit ambiguous due to unclear "precedence":

You should supply either get_virtual_func or get_virtual_call_data_func with call_virtual_with_data_func.

Probably a comma would help a lot:

You should supply either get_virtual_func, or get_virtual_call_data_func with call_virtual_with_data_func.

Other than that, good from my side, thanks for the adaptations!

[fuzzybinary]

Comma added. Rebased on master.

[Bromeon]

Thanks a lot for your work! 👍

[Bromeon]

There seems to be a memory corruption in the double build, here... Not sure if introduced by this PR?

Edit: re-running made it work. A bit flaky tests, but looks unrelated to this.

[akien-mga]

Thanks!