GDExtension: Prevent compatibility breakage from change to ClassDB::instantiate() for unexposed classes

[dsnopek]

In PR #102440 we changed ClassDB::instantiate() to prevent creating classes that are unexposed

I don't think this was never intended to work. Unexposed classes defined within Godot that are unregistered (which is all of them) cannot be instantiated from ClassDB::instantiate(). Unexposed classes defined in a GDExtension are intended to work just like unexposed classed defined in Godot, however, in Godot 4.3 and 4.4, it is actually possible to instantiate them via ClassDB::instantiate().

From the perspective of GDExtension, this behavior change (despite being a fix) is actually a compatibility breakage if an extension depended on creating unexposed classes via ClassDB::instantiate().

This PR prevents that compatibility breakage by adding a new classdb_register_extension_class5() that honors the is_exposed flag, and changes previous versions of that function to force that flag to true. This way older extensions that depended on the incorrect behavior of ClassDB::instantiate() should continue to work without issues, although, their unexposed classes will become exposed when the extension is loaded in Godot 4.5+

I'm not sure this is actually a good idea :-)

While we're aware of one extension that depends on the old behavior, my gut feeling is that most GDExtensions that use internal classes do not depend on this, and this change will simply make all their unexposed classes become exposed (which won't break anything, but would be annoying) until they upgrade to the new function and hence opt in to the new behavior

So, the question is:

1. Do we go with this PR to preserve compatibility, even though not many extensions depend on the incorrect behavior, and it will have annoying side effects?
2. Or, do we say this breakage is acceptable, and leave things as they are?

See godotengine/godot-cpp#1818 for the godot-cpp changes, which include additions to the tests to verify that unexposed classes registered from GDExtension work as expected

[Bromeon]

While we're aware of one extension that depends on the old behavior

Do we know what the practical impact for godot-orchestrator would be?

---

Or, do we say this breakage is acceptable, and leave things as they are?

I'd probably tend to this. It sounds like we made a mistake with information hiding and accidentally "leaked" internal classes. My personal course of action would be to then reduce the blast radius, not extend it 😉

It's a bit of a pity that the ClassDB docs are so sparse here, e.g. can_instantiate is simply documented as "Returns true if objects can be instantiated from the specified class, otherwise returns false." An entire sentence with precisely as much information as the signature 😬 This may have encouraged more liberal interpretations of the provided semantics...

[dsnopek]

While we're aware of one extension that depends on the old behavior

Do we know what the practical impact for godot-orchestrator would be?

My understanding is that this affects how they load their custom resources representing scripts. So, if we keep things as they are, the existing binaries of the extension won't be able to load scripts (which is pretty fundamental to a visual scripting language :-)).

I don't think the necessary changes will be that big of a deal, but they will need to make a new version, and users will need to update to it

[Naros]

It's a bit of a pity that the ClassDB docs are so sparse here, e.g. can_instantiate is simply documented as "Returns true if objects can be instantiated from the specified class, otherwise returns false." An entire sentence with precisely as much information as the signature 😬 This may have encouraged more liberal interpretations of the provided semantics...

I cannot agree more. The more that folks decide to explore GDExtension as a way to provide editor tooling, the more we can document and clearly outline expectations without expecting someone to go look at the implementation is just going to save future headaches not only for extension developers, but Godot maintainers IMO.

Do we know what the practical impact for godot-orchestrator would be?

For us, it depends on what we intend to do moving forward.

On the one hand, we can align with the expectations that were broken in 4.2/4.3/4.4, and the changes in 4.5, by marking the classes that were internal as non-internal, which solves the issue with compatibility.

Fortunately for us, we aren't bound to the same limitations of serialization as .tres resources, which will no longer allow serialization of internal resources in 4.5 because we use our resource loader/saver. However, what it does mean is that we will need to find a way to register these classes so we can construct them by name.

This brings me to a question of whether we could have something like ClassDB::instantiate_internal. I only propose this because it prevents tooling developers from needing to find creative ways from having a secondary factory mechanism to instantiate internal classes, given that we'll be marking these classes with GDCLASS and GDREGISTER_INTERNAL_CLASS macros.

This may constitute additional methods, to resolve whether a name is internal or not, etc. But I'm just wondering if such a separate, explicit, and deliberate set of APIs makes sense for the broader masses.

And to @dsnopek's point about compatibility, we generally have x.y builds that are specifically tied to a specific Godot a.b version, due to API changes both in the engine and at the godot-cpp level, thus a new build would be necessary.

What concerns me more is a user opening their script, seeing that the script is empty because they're using an older version built for 4.4 or 4.3 (as they don't often realize they need to upgrade), accidentally saves the project and the resource gets cleared because none of the original internal resources were loaded.

[dsnopek]

This brings me to a question of whether we could have something like ClassDB::instantiate_internal.

I feel quite strongly that we should not expose a way to instantiate unexposed classes.

We have a way to do it within the editor, so that GDExtensions can use internal classes for editor plugins, but I don't think that scripts or other GDExtensions should have any way to instantiate unexposed classes. They should only be directly instantiate-able by the GDExtension that declares them, and not through ClassDB, otherwise they are exposed classes - not unexposed.

I only propose this because it prevents tooling developers from needing to find creative ways from having a secondary factory mechanism to instantiate internal classes, given that we'll be marking these classes with GDCLASS and GDREGISTER_INTERNAL_CLASS macros.

I don't think it's too hard to make a factory mechanism if an extension needs it.

Here's the (untested) C++ code for this that I shared on RocketChat:

typedef Object *(*CreationFunc)();

template <typename T>
Object *_creation_ptr_func() {
	memnew(T);
}

HashMap<StringName, CreationFunc> internal_creation_funcs;

template <typename T>
void register_internal_class(const StringName &p_name) {
	internal_creation_funcs[p_name] = &_creation_ptr_func<T>();
}

Object *instantiate_internal(const StringName &p_name) {
	if (internal_creation_funcs.has(p_name)) {
		return internal_creation_funcs[p_name]();
	}
	return ClassDB::instantiate(p_name);
}

// Call this by your GDREGISTER_INTERNAL_CLASS():
register_internal_class<MyClass>();

// Now you can call instantiate_internal("...") for both exposed classes and your extension's internal classes.

I don't think that's too complicated of a thing for the handful of extensions that need something like this

What concerns me more is a user opening their script, seeing that the script is empty because they're using an older version built for 4.4 or 4.3 (as they don't often realize they need to upgrade), accidentally saves the project and the resource gets cleared because none of the original internal resources were loaded.

And this PR should prevent that

[Repiteo]

The implementation is valid & codestyle checks out, so this is technically good to go. The next GDExtension meeting can make the call on if this should be merged

[dsnopek]

Discussed at the GDExtension meeting: we think we should go for this PR. The compatibility breakage may not end up affecting very many GDExtensions, but we don't know for sure how many, so we'd like to err on the side of preserving compatibiltity.

[spookto]

Tested on my project and it seems to fix pkdawson/imgui-godot#103

[Repiteo]

Thanks!

[Daylily-Zeleen]

I‘m sorry to leave a comment to this closed pr.
I found it this break my old GDExtension plugin (create with 4.3) after I using godot 4.5.

// HFSMEditorPlugin
HFSMEditorPlugin *HFSMEditorPlugin::instance = nullptr;
Ref<ImageTexture> HFSMEditorPlugin::empty_icon_for_state_node = nullptr;

HFSMEditorPlugin::HFSMEditorPlugin() {
	CRASH_COND_MSG(instance, "Should be instantiated only once.");
	instance = this;

	StateConfig::get_animation_list = &get_animation_list_for_state_config;
	StateNode::get_empty_icon = &get_empty_icon_for_state_node;

	...... 

I make an unexposed EditorPlugin become a singleton, it will be create twice, for adding editor plugin and generating document.

This is certainly not the best practice for now, but this pr is definitely break compatibility.

[dsnopek]

Oof. I'm assuming what's happening is the editor is creating (and then deleting) an instance of the class to get the property defaults?

I'm not sure what the right course of action is at this point? This PR unbroke compatibility for some extensions. I'm not sure if there's a way we can prevent breaking both?

[Daylily-Zeleen]

@dsnopek I was busy with work for several months and didn't pay attention to changes in Godot.
I don't quite understand the purpose of this pr. Why we revoke the feature of registering unexposed class from extension?

[dsnopek]

It was previously possible to use ClassDB.instantiate() to create unexposed classes. This wasn't intentional and made metadata reported by ClassDB about classes a little weird. So, we fixed it so that ClassDB.instantiate() couldn't create unexposed classes, but it turns out there were extensions that depended on that.

This PR was meant to make it so that unexposed classes from older GDExtensions were exposed (so they could still be created with ClassDB.instantiate()), but make it that unexposed classes registered in new GDExtensions (using classdb_register_extension_class5) were really unexposed. This would give those extensions that depended on the old behavior time to update.

At the time I couldn't think of a way that unexposed classes becoming exposed could actually break anything; only add entries to lists that users would need to ignore.

[dsnopek]

@Daylily-Zeleen Please let me know if PR #111090 fixes your extension!