Fix GDExtension classes derived from abstract GDExtension classes always being registered as abstract

[rburing]

Fixup to #66979. In particular, this makes concrete GDExtension nodes derived from abstract GDExtension nodes show up in the Create New Node dialog, and allows such nodes to be saved/loaded in scenes.

Thanks to @aaronfranke for pointing out the issue in godotengine/godot-cpp#883 (comment).

[aaronfranke]

I can confirm that this fixes the issue. Here's what it looks like:

[aaronfranke]

EDIT: For CanvasItem will not be a problem anymore after #67510 is merged, though it will still not work for other non-CanvasItem abstracts. A crash is still worse than a non-crash for invalid code but I think it's important to prioritize the good improvement this brings for valid code. So I think we should merge this.

Up until now I've been testing with my Node1D inheriting Node2D (which is sub-optimal). This PR is still an improvement because it fixes abstract classes when inheriting Node2D.

However, I'd like it to inherit CanvasItem. When I had it inside of the engine it "just worked" with nothing special going on. When I try to inherit CanvasItem I am getting an error. The behavior of this error message actually changed with this PR.

With this PR (and Godot crashes):

ERROR: Class 'CanvasItem' or its base class cannot be instantiated.
   at: instantiate (core/object/class_db.cpp:327)

Without this PR (and the classes fail to instance):

ERROR: Class 'Sprite1D' or its base class cannot be instantiated.
   at: instantiate (core/object/class_db.cpp:327)
ERROR: Class 'StaticBody1D' or its base class cannot be instantiated.
   at: instantiate (core/object/class_db.cpp:327)

It's not working either way, but maybe you have an idea of how to fix this fully.

[groud]

We reviewed this PR in the GDextension meeting. We think the idea makes sense, but we should probably clear our mind on how to handle abstract and virtual classes in extension vs core.
In in any case, we prefer not to merge the PR right now, as it is causing unwanted crashes for now.

Related PR: #65542

[aaronfranke]

My comment above may be obsolete, we want to move things in the engine to virtual instead of abstract so that they can be instanced on the engine level. For CanvasItem: #67510

Although, we still want to prevent Godot crashing when the user writes some invalid code.

[dsnopek]

What is the problem we're trying to solve here?

If it's just to allow instantiating a GDExtension class that descends from another GDExtension class that is abstract, I think this PR may go a little too far:

while (concrete_parent->creation_func == nullptr && concrete_parent->inherits_ptr != nullptr) {
	concrete_parent = concrete_parent->inherits_ptr;
}

This loop will keep going up the inheritance tree until it finds a parent that can be constructed, which could also go up passed a native Godot class that is abstract. I think we want to stop once we get to the native parent, otherwise the GDExtension instance could use a wrapper that doesn't match its class on the engine side, and lead to unexpected/broken behavior.

I think if that were fixed, I'd personally be happy to approve this PR!

On the other hand, if this PR is trying to solve extending abstract native classes in GDExtension (like CanvasItem, as mentioned above), I don't personally think this is the right path. In cases where we want to allow GDExtension to extend an abstract native class, I think we should either add an *Extension class (ex ScriptExtension) or consider making the class virtual rather than abstract (which is what PR #67510 is about for CanvasItem).

[aaronfranke]

@dsnopek This is about extensions providing abstract classes, for example, CollisionObject1D in my screenshot.

This loop will keep going up the inheritance tree until it finds a parent that can be constructed, which could also go up passed a native Godot class that is abstract. I think we want to stop once we get to the native parent, otherwise the GDExtension instance could use a wrapper that doesn't match its class on the engine side, and lead to unexpected/broken behavior.

That likely explains why this PR crashes the engine if you try to inherit CanvasItem (while before it just showed an error).

On the other hand, if this PR is trying to solve extending abstract native classes in GDExtension (like CanvasItem, as mentioned above)

This is a separate topic, for which I have PR #67510. I just mentioned it because that's another related thing I want to solve, and this PR changes the behavior a bit (crash instead of error). But if we have both this PR and #67510 merged then it will work for all of my use cases.

[rburing]

@dsnopek Thanks for investigating!

I think we want to stop once we get to the native parent

Done.

[dsnopek]

Thanks! This is looking great to me now :-)

[aaronfranke]

Tested and confirmed that both of these things are correct:

1. Abstract GDExtension classes (like CollisionObject1D) are allowed to have non-abstract derived classes:

2. If trying to inherit from an engine abstract, it correctly prints an error, and does not crash:

ERROR: Extension class Node1D cannot extend native abstract class CanvasItem
   at: register_extension_class (core/object/class_db.cpp:1671)

[YuriSizov]

Thanks!