Allow GDExtension to register unexposed classes.

[Daylily-Zeleen]

Because of renaming from GDNative to GDExtension, I need to remake #69653. Otherwise I can't keep 1 commit.

For #970.

[akien-mga]

You should have been able to rebase this and the other PR, see https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html for details. But now that you opened new PRs it's fine, just something to keep in mind for later.

Both new PRs have unsuitable commit messages, "remake" is not informative. The commit message should say how the PR affects the engine, not what you did.

[Daylily-Zeleen]

You should have been able to rebase this and the other PR, see https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html for details. But now that you opened new PRs it's fine, just something to keep in mind for later.

Both new PRs have unsuitable commit messages, "remake" is not informative. The commit message should say how the PR affects the engine, not what you did.

Thanks for your warning. I fixed the commit message.

[Zylann]

Does this actually prevent the class from being added to the Add X dialogs and Editor Help? I found the latter to cause crashes on startup, because Editor Help queries properties default values, which then instantiate those classes temporarily, and their constructor can want to access singletons that are not available: https://github.com/godotengine/godot-cpp/issues/1179 Skipping such classes in these areas of the editor would fix that issue too.

[Daylily-Zeleen]

Does this actually prevent the class from being added to the Add X dialogs and Editor Help?

The variable exposed will be used to filter document in DocTool, and prevent adding unexposed classes to EditorHelp, I had tested it. ~~But it seems that it is not used in CreateDialog, so I'm not sure it will block unexposed classes to be added to CreateDialog or not, I have not tested for this. If you have time, please do some test.~~

I do a little change in CreatreDialog to make it ignore unexposed classes, so it can avoid adding them.

I found the latter to cause crashes on startup, because Editor Help queries properties default values, which then instantiate those classes temporarily, and their constructor can want to access singletons that are not available: godotengine/godot-cpp#1179

You can avoid temporarily instantiation by registering abstract class. This is not the issue of this pr.

[dsnopek]

Overall, I think this is a great change!

However, it will need to be done in a slightly different way to ensure compatibility. We haven't yet made a change to a struct in gdextension_interface.h yet, but we recently discussed how it should probably be done:

1. Rather than adding the new property to the existing struct, create a new struct with the new property. I'm not sure how these should be named (this will probably be hashed out on the first one we do), but it could be as simple as GDExtensionClassCreationInfo2 (ie with 2 added to the end)
2. Create a new function that takes the new struct - perhaps classdb_register_extension_class2 (again adding a 2)?
3. Have the old function construct the new struct, and then call the new function

Anyway, like I said, we haven't done one of these yet, so exact details (especially around naming) may take a different shape as this PR (or others which need to do a similar thing) are discussed.

[dsnopek]

Now that PR https://github.com/godotengine/godot/pull/78634 has been merged, the is_exposed property could be moved to the new GDExtensionClassCreationInfo2 struct, which would fix the compatibility issues from my previous comment.

@Daylily-Zeleen Is this a change you have time to do? If not, I'd be happy to take this over in a new PR :-)

[Daylily-Zeleen]

@dsnopek OK, now I moved it to GDExtensionClassCreationInfo2, both in godot-cpp/pull/970, too.

But I can't do a appropriate test about this, the editor help (F1) can't show classes made by GDExtension currently , I'm not sure if this change is working correctly (maybe it is related to #76796) , I just create a editor script like this:

@tool
extends EditorScript

func _run() -> void:
    print(ClassDB.class_existes("UnexposedClass"))
    print(ClassDB.class_existes("ExposedClass"))

And the Output panel show the reasult:

false
true

And if the class which is inherit from Node and use GDREGISTER_INTERNAL_CLASS() to register in gdextension. it will not appear in the CreateDialog.

[Daylily-Zeleen]

@dsnopek To be honest, I think the changes your mention about are not needed, too. However, is_virtual, is_abstract and is_exposed are in the same struct, I guess they need to be able to control at the same level.

And there already have register_class(bool p_virtual) and have register_abstract_class() to register an abstract class, therefore I add register_virtual_class() and register_internal_class(), and add argument p_exposed and p_virtual to each register_xxx_class() if it is need.

I think the best way is remove register_abstract_class(), register_virtual_class() and register_internal_class(), pass p_virtual, p_abstract and p_exposed to register_class(). Of course, GDREGISTER_CLASS(), GDREGISTER_VIRTUAL_CLASS(), GDREGISTER_ABSTRACT_CLASS() and GDREGISTER_INTERNAL_CLASS() are still needed and base on register_class().

But I guss that register_abstract_class() is already in use both godot and godot-cpp, I think this idea can't be realized.

---

Let's return to this pr, do you means that only provide ClassDB::register_internal_class() without argument and GDREGISTER_INTERNAL_CLASS(m_class) is enough?

[dsnopek]

Let's return to this pr, do you means that only provide ClassDB::register_internal_class() without argument and GDREGISTER_INTERNAL_CLASS(m_class) is enough?

Yes, in my opinion, that would be enough.

[Daylily-Zeleen]

@dsnopek OK, change again both this pr and godot-cpp/pull/970.

[Daylily-Zeleen]

@akien-mga Done.

[dsnopek]

For some reason this PR likes to sometimes fail CI in the "Linux / Editor with doubles and GCC sanitizers" job with this error:

AddressSanitizer: attempting free on address which was not malloc()-ed:

The stack trace appears to point to freeing a PropertyInfo from a MethodInfo's arguments when Godot is shutting down. But I don't really see how this PR could lead to this issue?

If I re-run the tests enough times, it does eventually pass, but it fails often enough that I'm questioning if there's something I'm missing?

[AThousandShips]

It's occuring generally even for completely trivial PRs

[reduz]

I am not against this change, but it looks like it breaks compatibility, @dsnopek can you clarify why do you believe it does not?

[dsnopek]

@reduz This adds the new is_exposed field to the GDExtensionClassCreationInfo2 struct (with a 2 at the end) which is accepted by the classdb_register_extension_class2 function (also with a 2) which are both new for Godot 4.2. GDExtensions created for Godot 4.1 or earlier use the GDExtensionClassCreationInfo struct and classdb_register_extension_class function (without a number at the end of either) which doesn't have this field, and will continue working as they used to.

The GDExtensionClassCreationInfo2 struct and classdb_register_extension_class2 function were added in PR https://github.com/godotengine/godot/pull/78634, and now other PRs (like this one) can continue to add things to them until Godot 4.2 is released, and after that they'll have to remain as they are for compatibility. There's a couple more PRs that need to change this new struct (including the "hot reload" PR!) and I'm hoping we can get as many as possible merged for 4.2 to avoid needing to do a 3 version of the struct right away for 4.3 :-)

[akien-mga]

Thanks!