Allow registering "runtime classes" from GDExtension

[dsnopek]

Classes registered via GDExtension normally run in the editor (just like module classes) which is sort of similar to @tool scripts. This is the number 2 complaint from people who attempt to use GDExtension to write gameplay code (as opposed to people who are using GDExtension to extend the engine).

This PR allows registering "runtime classes" from GDExtension, where the real code doesn't run in the editor, similar to how non-@tool scripts work.

The idea is that when running in the editor, these special classes would have "placeholders" created, which can store the classes properties, so that the inspector and saving/loading still works (which is similar to how non-@tool scripts are implement too).

In order to do this, this PR refactors how Object handles extension instances. Previously, this was just a void * and everything in Object and elsewhere had to interact with the GDExtension interface directly. This PR adds a ObjectGDExtensionInstance class to centralize this work, so now we have the ObjectGDExtension and ObjectGDExtensionInstance classes which are analogous to the Script and ScriptInstance classes.

This makes it easy for the ObjectGDExtensionInstance class to switch over to "placeholder mode" when necessary.

UPDATE: I ended up rolling back the refactor, because I came up with a clean way to handle placeholders without it.

For this PR to work with godot-cpp, you need this companion PR godotengine/godot-cpp#1256

Still TODO

• Get the default values for the real classes properties (we may need to create a single instance the real class to do this - I think this should be possible to do)
• Set/get property values and ensure saving and loading works
• Correctly handle reverting properties via the inspector
• See if ObjectGDExtensionInterface can do something smart for the GDVIRTUAL*() macros
• Ensure that this works with hot reload (it should)
• Investigate the interaction between runtime classes and tool GDScripts
• Investigate the interaction between runtime and non-runtime classes from the same GDExtension

Fixes #54999

Production edit: closes godotengine/godot-roadmap#36

[dsnopek]

Actually, this seems to be mostly working? I haven't managed to get classes switching back and forth between "gameplay" and "not gameplay" on a hot reload, but otherwise the basics are working.

The implementation feels kinda messy, and I'm not sure if this is the right approach in general, so this'll need some more testing (to see if it meets expectations) and discussion.

[dsnopek]

I've been thinking about this PR over the weekend. I think what it's doing at a high-level is probably the right overall approach (ie. making placeholders when gameplay classes are instantiated in the editor). However, the implementation is kinda messy, and I'm not crazy about it.

Perhaps the way that Object interacts with its extension and extension instance needs to be refactored? For scripts, we have the Script and ScriptInstance classes, and the way Object interacts with them is pretty straight-forward. However, for extensions, we have the ObjectGDExtension struct and a void * for the instance, and all the interactions are done manually all over Object.

Perhaps if we wrapped at least the extension instance in a light-weight class, the interactions could be made cleaner and easier to maintain? Since performance is important in GDExtension, we'd probably want to make sure that all the hot-path functions are _FORCE_INLINE_ to try and keep the compiled code the same as it is now. But that would also make it much easier to optionally create a placeholder version of an extension object (and, of course, that additional code could be made to only exist in editor builds).

In any case, I may experiment with some ways to refactor how Object interacts with its extension and extension instance before continuing this PR.

[Bromeon]

As prior art, I worked around this issue in godot-rust: godot-rust/gdext#365

Basically, what you call gameplay classes is the default, and a #[class(tool)] attribute can be used to make classes run in the editor. "Not running" amounts to not having virtual callbacks _ready, _process, _notification etc invoked, but classes are still constructed and accessible (this is necessary because scene trees break otherwise).

I haven't built a larger game with this yet, so there's some research to be done on how useful that behavior is in practice. But so far, community feedback has been quite positive.

For a Godot-based solution to this problem, it would be good to outline the exact semantics that one would expect. For example, configuring exported properties is often still desired inside the editor.

[dsnopek]

Thanks for sharing your approach from the Rust bindings!

I think we want to go further than just not executing virtual methods, though. In the implementation in this PR, we don't actually create the real class (except for once, in order to get the property defaults), and all the placeholder can do is get/set property values (so you can still edit the object in the editor inspector). This fairly closely matches how non-@tool GDScripts work (except I think GDScript can get the property defaults without needing to create a real script instance), where not even getter/setter functions will run unless it's a @tool script.

[bluenote10]

In the implementation in this PR, we don't actually create the real class (except for once, in order to get the property defaults)

Would this still allow for preventing _init to be called whatsoever?

Use case: I often want to load certain external resources already at construction time, so that the class can consider "all external resources are loaded" as an invariant, i.e., not having to deal with partial initialization. If I remember correctly
godot-rust/gdext#365 only prevents calling virtual methods like _ready, _process, etc., but not _init, which means that when starting the Godot editor I see all my gdextension classes doing their initialization. Some of them perform some rather non-trivial tasks sometimes even with side effects (e.g. logger setup, singleton initialization, file system discovery...). These nodes/objects are internal and are not supposed to be used directly, so it would be great if construction by the editor could be prevented entirely.

[dsnopek]

Would this still allow for preventing _init to be called whatsoever?

Yes.

With the current implementation in this PR, each "real" class needs to be created exactly once in order to get the default values for its properties, but it's destroyed right after that, and all subsequent instances of the class are just "placeholders". The placeholders don't create an instance of your real class, they just have a hash map of the properties, so they can be edited in the inspector.

[Bromeon]

With the current implementation in this PR, each "real" class needs to be created exactly once in order to get the default values for its properties

Question regarding that, what if the class cannot be constructed (GDExtension doesn't provide a create callback, or marks it abstract)?

[dsnopek]

Question regarding that, what if the class cannot be constructed (GDExtension doesn't provide a create callback, or marks it abstract)?

Hm. I think the current PR doesn't correctly handle that situation, but I think what it should do, is also not allow creating a placeholder if the class can't normally be created. I'll add a fix for that when I come back to working on this one. :-)

[dsnopek]

I've finally finished my big refactor of how Object handles extension instances, mostly as described in my earlier comment.

Previously, Object just held a void * for the extension instance we got from the binding and everything in Object (and elsewhere) had to interact with the GDExtension interface directly. This PR adds a ObjectGDExtensionInstance class to centralize this work, so now we have the ObjectGDExtension and ObjectGDExtensionInstance classes which are analogous to the Script and ScriptInstance classes.

This makes it easy for the ObjectGDExtensionInstance class to switch over to "placeholder mode" when necessary.

The main difference between what I wrote in my comment and what I implement has to do with this:

Since performance is important in GDExtension, we'd probably want to make sure that all the hot-path functions are _FORCE_INLINE_ to try and keep the compiled code the same as it is now.

I originally did try using _FORCE_INLINE_ on everything, but the functions were just so long and so ugly, it felt dirty having them in object.h which most of Godot includes. So, they are normal functions in this PR currently, which does technically add another function call which could theoretically hurt performance, but I guess I'd argue that the existing functions aren't strictly "hot-pathes". Maybe set() and get()? I think the GDVIRTUAL*() macros are actual "hot-pathes" and those remain unconverted for now (although, I may do a thing with _FORCE_INLINE_, that's one of my remaining todo's.)

There's only a handful of things left to do on this (see the PR description) before it's ready for review!

[dsnopek]

I just pushed the following changes:

• Moved set(), get() and notification() to the header with _FORCE_INLINE_ since I think those could arguably be called "hot-pathes"
• Also added get_virtual() and call_virtual() (also _FORCE_INLINE_) to be used in the GDVIRTUAL*() macros
• Various little renames and clean-ups

I also tested using hot-reload with a gameplay class, and it seemed to work fine!

Assuming tests pass, this should finally be ready for review :-)

[dsnopek]

Actually, thinking about it some more... I think I know a way that I could do this without the refactor into ObjectGDExtensionInstance, but that isn't as messy as the older version. I guess it's a question of: Do we prefer to centralize the concept of an "extension instance" into a class? Or, have it scattered across the code base -- which is basically inlining, and consequently, good for performance?

[dsnopek]

@Ansraer brought up some interesting cases on RocketChat that I haven't accounted for:

1. Interaction between gameplay classes and tool GDScripts: With GDScript, if a tool GDScript creates an instance of a non-tool GDScript, it creates a real instance of the class. Whereas in my current PR, any time you create an instance of a gameplay class, you get the placeholder version. Users will probably expect the same behavior as GDScript. To do this, I think we need to only create the placeholders when loading a scene, but create the real version whenever instantiated directly
2. Interaction between gameplay and non-gameplay classes from the same GDExtension: If you have a scene whose root is a non-gameplay class, and it contains a child node that is a gameplay class, what do you get if you get_node() that child node? When run in game, you'd always get a real instance of the gameplay class, but in the editor, that would be a placeholder, which I think would be the type of the closest native parent rather than the type defined in the GDExtension. I think GDScript should have a different version of the same problem with tool and non-tool scripts, so it'd be interesting to see how it handles it.

[Rennan24]

@dsnopek

2. Interaction between gameplay and non-gameplay classes from the same GDExtension: If you have a scene whose root is a non-gameplay class, and it contains a child node that is a gameplay class, what do you get if you get_node() that child node? When run in game, you'd always get a real instance of the gameplay class, but in the editor, that would be a placeholder, which I think would be the type of the closest native parent rather than the type defined in the GDExtension. I think GDScript should have a different version of the same problem with tool and non-tool scripts, so it'd be interesting to see how it handles it.

In C# at least when you create a Tool class and try to reference or GetNode() a Non-Tool class, it creates an InvalidCastException because there is no actual Non-Tool class running in the editor. Although in GDScript it works perfectly fine which is a little weird to me but I don't know the inner workings of the C# or GDScript bindings.

There are multiple people that have run into this problem and its something that is pretty annoying because you then have to mark everything as [Tool] or just use the inherited class which loses some information about the class
(#80298, #40069, #85825)

I attached a MRP showing an example of what I mean if I didn't explain it properly! don't know if this necessarily applies to this new gameplay classes but worth bringing up :D
CSvGD Tool Scripts (MRP).zip

Hopefully its something that can be fixed in GDExtensions or C# since my preferred use case is to have a [Tool] script that can automatically get references with GetNode() or just perform some operations on its children that are NonTool classes

[AThousandShips]

The code looks good, makes sense, haven't tested ATM though

[reduz]

In general it looks good, save for missing unlikely() or likely() to help the optimizer. I marked a few but in the more critical functions it would be good to ensure they are there.
I also feel there is some overlap with MissingResource and MissingNode, but I am not sure if this is fine or intended (I guess its not exactly the same thing).

Other than that it looks great!

[reduz]

So, here is a thought about this.

If you are extending the engine and using GDExtension, then from your perspective anything that runs in the game will be gameplay.

But if you are using a GDExtension language (say eventually C# or Rust), you will most likely not want to extend the engine and use all classes for gameplay.

As such, I am not entirely convinced that this is something we should expose as "gameplay" rather than "tool". At least, as example, I am pretty sure that when C# is moved to GDExtension, it will still want to retain the "tool" concept (given again 99% of the people uses it to create gameplay), so it may want to do this in a flipped up way.

Has this been discussed? Maybe an alternative could be, at extension registration time (and this is mostly on the extension side?) have to specify what the default is (tool or gameplay), and then those that do not meet the default have to be specified.

[dsnopek]

@reduz Thanks for the review! All your code comments should be addressed in my latest push :-)

I also feel there is some overlap with MissingResource and MissingNode, but I am not sure if this is fine or intended (I guess its not exactly the same thing).

It's not really the same thing.

There's some high-level similarities in the implementation, but with placeholders for "gameplay classes" (at least if we want to match the behavior of non-@tool classes from GDScript) we want them to act like the parent class in the editor (so, if you sub-class Area3D, for example, you want it to have all the signals, properties, configuration warnings, gizmo, etc of Area3D and its ancestors) and only allow the class's actual custom properties as valid (we instantiate a real version of the class once in the editor to get the property list and all the default values).

As such, I am not entirely convinced that this is something we should expose as "gameplay" rather than "tool". At least, as example, I am pretty sure that when C# is moved to GDExtension, it will still want to retain the "tool" concept (given again 99% of the people uses it to create gameplay), so it may want to do this in a flipped up way.

We could still use "tool" for the naming, if we said all classes are "tool classes" by default, and you're opting out of being a "tool class" (and I guess call these "non-tool classes"?). However, I personally feel it's kinda awkward when it's the "non-tool classes" that are the special case, especially in verbal/written discussions and in godot-cpp (we already have ClassDB::register_class(), so I guess we'd need to have ClassDB::register_nontool_class()?).

So, personally, I still prefer using the "gameplay classes" naming, however, I'd be happy to go with whatever alternative gets the community/contributor consensus.

[Riteo]

@dsnopek at this point I wonder, why not making the tool stuff the non-default route? That's probably what reduz is proposing.

[dsnopek]

@dsnopek at this point I wonder, why not making the tool stuff the non-default route? That's probably what reduz is proposing.

Hm, well, even if we change the default for classes registered from GDExtension, we still can't really call them "tool classes" and "normal classes" (at least without some confusion) because "normal classes" means something different (in fact, the complete opposite) for classes registered in the engine. From a naming perspective I think we'd still call them "tool classes" and "nontool classes", or at least I would. :-) So, to me, the default is a separate issue - unless we're talking about changing the default for classes registered in engine too?

[Riteo]

@dsnopek, uh, so classes by default are always run in the editor? Makes sense, after all the editor is a game.

Rightly you noted that in the OP too (sorry, been a while since I read it).

This might require lots of further discussion, as this would definitely imply some deeper change in the way extension classes are handled.

Right now, since the extension API is mostly "low-level", I think that the most important thing would be to just expose a way of making "non-tool" classes. We can then discuss on making the default behavior different somehow.

If the naming is an issue, what about calling those those "inert", "project", or just "placeholder" classes?

After all, we're supposed to mimic modules more than scripting, right? We want them to work like normal module classes.

[dsnopek]

It seems like the primary remaining issue is what to name the different types of classes :-)

These seem to be the most popular options based on previous discussions here and on RocketChat:

1. "Gameplay classes" - the main downsides are that this invents a new term and may be confusing given that Godot can create apps, or that a "gameplay class" may not concern actual gameplay. The main upside, is that it's a term, not an attempt at a literal description of how the class works (which would likely be an incomplete description, and less memorable).
2. "Runtime classes" - the main downside is that this is a more generic description-y term. The main upside is that it removes any potential confusion related to making non-games or using these classes for non-gameplay.
3. "Tool vs non-tool classes" - the main upside is that this is an existing term, and the main downside is "non-tool classes" are the special case, and referring to them with a negative feels sort of awkward.

I'll ask again on RocketChat too...

[dsnopek]

Based on an informal poll on RocketChat, it appears that "runtime class" is the most popular name. So, in my latest push, I've re-named everything. Please let me know what you think!

[Riteo]

I looked at this slightly harder than usual and, outside of a small nitpick, this actually looks fine! :D

Also, as this part of the engine is one I'd like to know a bit better, I made a bunch of questions regarding how this works, as there are some details that I'm missing. Hope you don't mind.

[dsnopek]

@Riteo Thanks for the review! ❤️

[Riteo]

I haven't tested this but code wise looks great!

[Bromeon]

Are there more naming changes to be expected, like the "tool/non-tool" discussion? I'd like to give this a try but each API change comes with quite a bit of overhead (fetch again, recompile Godot, generate bindings, adapt code) 🙂

[dsnopek]

@Bromeon:

Are there more naming changes to be expected, like the "tool/non-tool" discussion?

I really hope not :-)

Changes done. Approved on RC.

[akien-mga]

Thanks!