Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

rburing
Copy link
Member

@rburing rburing commented Oct 16, 2022

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).

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screen Shot 2022-10-17 at 12 16 37 PM

@aaronfranke
Copy link
Member

aaronfranke commented Oct 17, 2022

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.

@rburing rburing force-pushed the opposite_of_abstract_is_concrete branch from 10549e6 to cca5d90 Compare October 17, 2022 18:20
@rburing rburing marked this pull request as draft October 17, 2022 18:45
@Chaosus Chaosus added this to the 4.0 milestone Oct 19, 2022
@groud
Copy link
Member

groud commented Dec 1, 2022

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
Copy link
Member

aaronfranke commented Dec 1, 2022

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.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 10, 2023
@rburing rburing marked this pull request as ready for review August 18, 2023 21:49
@akien-mga akien-mga requested a review from a team August 21, 2023 06:10
@dsnopek
Copy link
Contributor

dsnopek commented Aug 21, 2023

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
Copy link
Member

@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 rburing force-pushed the opposite_of_abstract_is_concrete branch from cca5d90 to acf9d4e Compare August 22, 2023 07:54
@rburing rburing changed the title Fix GDExtension classes derived from abstract classes always being registered as abstract Fix GDExtension classes derived from abstract GDExtension classes always being registered as abstract Aug 22, 2023
@rburing
Copy link
Member Author

rburing commented Aug 22, 2023

@dsnopek Thanks for investigating!

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

Done.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and confirmed that both of these things are correct:

  1. Abstract GDExtension classes (like CollisionObject1D) are allowed to have non-abstract derived classes:
Screenshot 2023-08-22 at 12 56 45 PM
  1. 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 YuriSizov modified the milestones: 4.x, 4.2 Aug 25, 2023
@YuriSizov YuriSizov merged commit fff32bb into godotengine:master Aug 25, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants