Handle missing instance binding callbacks by finding the closest parent

[dsnopek]

This attempts to solve a problem that was brought up on issue #1160 (although, this PR doesn't solve that issue).

Basically, the problem is that we assume we'll have the instance binding callbacks for any valid wrapper class. If we don't have the wrapper class, we output an error message and use Object.

However, in either of these cases:

• the class is a new class that was added to Godot after the GDExtension was created (so it won't know about it), or
• the developer explicitly excluded the class from godot-cpp

... we want to do what we're already doing for unexposed classes, which is to find the closest parent class that we do know about.

This will only output an error message, if we're unable to find any ancestor classes that we know about.

This depends on PR #1164 which is currently a draft. So, marking this one as a draft too!

Fixes #1163

[DmitriySalnikov]

ERROR: Condition "singleton_obj == nullptr" is true. Returning: nullptr at: godot::ClassDBSingleton::get_singleton (.../godot_debug_draw_3d/godot-cpp/gen/src/classes/class_db_singleton.cpp:44) 

The problem is that the class is now called ClassDBSingleton, and not ClassDB, as singleton is named.

I added this patch:

diff --git a/binding_generator.py b/binding_generator.py index d04c698..7d6f6e7 100644 --- a/binding_generator.py +++ b/binding_generator.py @@ -1453,7 +1506,10 @@ def generate_engine_class_source(class_api, used_classes, fully_used_classes, us if is_singleton: result.append(f"{class_name} *{class_name}::get_singleton() {{") - result.append(f"\tconst StringName _gde_class_name = {class_name}::get_class_static();") + if class_name != "ClassDBSingleton": + result.append(f"\tconst StringName _gde_class_name = {class_name}::get_class_static();") + else: + result.append(f"\tstatic const StringName _gde_class_name = StringName(\"ClassDB\");") result.append( "\tstatic GDExtensionObjectPtr singleton_obj = internal::gdextension_interface_global_get_singleton(_gde_class_name._native_ptr());" ) @@ -1480,7 +1536,10 @@ def generate_engine_class_source(class_api, used_classes, fully_used_classes, us result.append(method_signature + " {") # Method body. - result.append(f"\tconst StringName _gde_class_name = {class_name}::get_class_static();") + if class_name != "ClassDBSingleton": + result.append(f"\tconst StringName _gde_class_name = {class_name}::get_class_static();") + else: + result.append(f"\tstatic const StringName _gde_class_name = StringName(\"ClassDB\");") result.append(f'\tconst StringName _gde_method_name = "{method["name"]}";') result.append( f'\tstatic GDExtensionMethodBindPtr _gde_method_bind = internal::gdextension_interface_classdb_get_method_bind(_gde_class_name._native_ptr(), _gde_method_name._native_ptr(), {method["hash"]});' 

And now everything seems to be working as before. No errors, no crashes, at first glance.
Now the difference in size is not so significant, and it is quite acceptable.
37 MB -> 48 MB

Here is the action in my repository with this PR. (+9 MB for editor libs)

I hope this shouldn't affect performance too much. I didn't notice a major performance decrease overall.

---

I decided to try to compare the performance of the code that previously displayed a lot of errors. I didn't see any noticeable difference.

void DebugDraw::test_tree_process() { uint64_t start_time = TIME()->get_ticks_usec();	String editor3d = "Node3DEditorViewportContainer";	String subviewport = SubViewport::get_class_static();	Node *res = Utils::find_node_by_class(SCENE_ROOT(), editor3d);	Node *n = res->get_child(0)->get_child(0); UtilityFunctions::print("Test processed: ", TIME()->get_ticks_usec() - start_time); } Node *Utils::find_node_by_class(Node *start_node, const String &class_name) { for (int i = 0; i < start_node->get_child_count(); i++) { auto c = start_node->get_child(i); if (c->get_class() == class_name) return c; auto nc = find_node_by_class(c, class_name); if (nc) return nc;	} return nullptr; } #define SCENE_ROOT() (Object::cast_to<SceneTree>(Engine::get_singleton()->get_main_loop())->get_root()) #define TIME() Time::get_singleton()

First runs:

PR 1165	master	avg PR	avg master 3474	3529	3547	3563 3244	3164 3202	4092 4128	3987 3144	4244 3162	3190 4400	3130 3280	3256 3669	3803 3223	3282 3407	3491 4183	3814 3798	3610 3330	3452 3288	3200 3558	3980 3219	3888 3784	3124 3313	3585 3436	4532 3313	4977 3195	3204 3427	3215 4950	3202 3334	3524 4125	3222 3339	3150 3250	3349 3699	3153 

Second runs:

PR 1165	master	avg PR	avg master 3986	3169	3588	3551 3498	3174 5570	3141 3214	3184 3362	3520 3300	3145 3332	3155 3636	3223 3271	3108 3192	3425 3480	3286 3426	3260 3236	4652 3462	3142 3286	3470 3306	5472 3245	3274 3657	4339 5406	3159 3277	3238 3939	3453 4163	3360 3251	3280 3323	3269 3324	5647 3267	3381 3170	3787 3211	3920 4281	3357 

Without this PR, with excluded unused classes and with fixed crashes, the output looks like this:

... ERROR: Cannot find instance binding callbacks for class 'HSplitContainer'. at: (godot-cpp\src\core\class_db.cpp:321) ERROR: Cannot find instance binding callbacks for class 'VSplitContainer'. at: (godot-cpp\src\core\class_db.cpp:321) ERROR: Cannot find instance binding callbacks for class 'SubViewportContainer'. at: (godot-cpp\src\core\class_db.cpp:321) Test processed: 3134184 Test processed: 5601 Test processed: 3309 Test processed: 3402 Test processed: 3638 Test processed: 4239 

[dsnopek]

@DmitriySalnikov:

The problem is that the class is now called ClassDBSingleton, and not ClassDB, as singleton is named.

I added this patch:

Thanks for the patch! I've integrated it into PR #1164 and rebased this PR on that.

And now everything seems to be working as before. No errors, no crashes, at first glance.

That's awesome! Thanks for testing it ❤️

[dsnopek]

Discussed at the GDExtension meeting, and we think this PR makes sense! However, it needs to wait on getting access to the ClassDB singleton's function before it can be merged.

[dsnopek]

Now that PR #1164 has been merged, I've rebased it and taken this one of draft!

We already discussed this PR at a GDExtension meeting back in July and approved it pending the merge of PR #1164, so I think it just needs passing tests, and someone to formally press the "Approve" button :-)

[BastiaanOlij]

Reviewed during GDExtension meeting, this looks like a good fix.

[dsnopek]

Cherry-picked for 4.1 in PR #1261