Fixes to allow object-less callables throughout Godot

[maiself]

This fixes #81887

I searched the entire Godot source code for Callable methods get_object(), get_object_id() and get_method(), which could potentially be used to detect if a callable is associated with an object. Several places in the code used these methods to check callable validity instead of using Callable::is_valid(), causing object-less callables to be rejected. I then carefully and selectively updated the code to use the correct methods.

A few places remain where object-less callables may not be handled correctly. These files use get_object() and get_method() for various purposes:

• scene/main/node.cpp: serialization
• editor/editor_node.cpp: serialization
• scene/resources/packed_scene.cpp: serialization
• core/core_bind.cpp: error messages
• core/variant/callable.cpp: custom callable get_method()
• core/variant/variant.cpp: error messages

While I was quite thorough, its possible I may have missed things. I also left gdscript and mono alone.

[dsnopek]

Thanks!

I went through each change, and tried to think through what it was doing before, and check if the new code is roughly equivalent. For the most part this is looking good to me! I haven't had a chance to do any testing.

[maiself]

Updated as discussed and rebased on master.

[dsnopek]

This is looking good to me personally!

It doesn't fully fix all cases in the code base, but it does fix many of the most common ones, and it does so in a fairly safe and conservative way. When development on Godot 4.3 opens, we can look at the remaining tricky cases in order to fix this comprehensively, hopefully, standardizing on Callable::is_valid() everywhere.

But this PR would be nice to get in Godot 4.2 for GDExtension, since we added the ability to create custom callables from GDExtension in PR #79005, and fixing this issue even somewhat opens up what GDExtensions are able to do.

[RandomShaper]

I need a sanity recap about this. Let me rubberduck the situation:

We have the possibility of a Callable referring to a static function in C++ or GDScript. In the former case, CallableCustomStaticMethodPointer is used (null target object), whereas in the latter, it's a regular Callable whose target object is a Script.

Assuming the above is correct, are we sure these changes are fine for both cases? I'd say so, since after all the original issue was about the C++ side (wasn't it?) and GDScript static callables would pass the target-must-be-non-null-and-not-dangling check either if done at each call site (before this PR) or implicitly inside the default is_valid() implementation.

Is all this disquisition true?

[dsnopek]

We have the possibility of a Callable referring to a static function in C++ or GDScript. In the former case, CallableCustomStaticMethodPointer is used (null target object), whereas in the latter, it's a regular Callable whose target object is a Script.

Assuming the above is correct, are we sure these changes are fine for both cases?

Yesterday, I re-tested the EditorScript that @KoBeWi posted with this PR, to check that GDScript static methods work. I haven't re-tested callable_mp_static() in a little while, but that didn't work before and is what this PR aims to fix, so I presume that @maiself has tested it recently. In a couple hours I'll have some more time and I could re-test that too?

[dsnopek]

I just tested connecting to a signal with a callable_mp_static() using this PR and it worked! Specifically, it was a Button and I connected to it's "pressed" signal, which was emitted correctly, printing out a test message. So, this PR appears to be working as intended.

[RandomShaper]

Thanks for testing it, @dsnopek. (And greetings!) My only pending thing is #82695 (comment). Otherwise (even if without applying it), this has my blessings.

[maiself]

Assuming the above is correct, are we sure these changes are fine for both cases?

Yep, that's the intent. I've been testing with @KoBeWi's EditorScript example, an extended version of the scripts #79521, and my Python bindings, so a lot of types and uses of callables have been tested.

[KoBeWi]

This looks like some optimization and should work without an object too (though the condition probably needs changing). Why was this removed?

[maiself]

For object-less callables this will always fall into the optimized path because get_object_id() will always return ObjectID(0), so for those callables its no good, and why I removed it. Im not sure what this condition could be changed to.

I tried to see what this optimization and the code that follows was about, but I'm not so familiar with the physics servers inner workings. This code goes all they way back to the original opensource commit, so no explanation there. When I tried to follow all the paths that could lead here it seemed that the optimized path doesn't ever get used in the current code, tho I didn't test it.

In short, I don't know why that code is there in the first place, how to properly change it to support object-less callables (tho maybe just adding p_callback.is_standard() && to the condition would be enough?), and it doesn't appear that removing it would harm anything.

[KoBeWi]

Maybe something like id.is_valid() && id == monitor_callback.get_object_id()?
This code allows to swap callbacks without clearing shapes if the object is the same. So the condition I suggested would make it skipped if the new callback has no object.

That said, I'm also not very familiar with physics code either. This "optimization" doesn't look very important; normally the callbacks are changed only when toggling area's monitoring property, which is rather uncommon thing to do.

[KoBeWi]

I only tested Tweens, but the code seems fine.

[akien-mga]

Thanks!