Implement reloading of GDExtensions

[dsnopek]

This is a draft PR adding "live" reloading of GDExtensions!

This would (partially) fix #66231

In order to work with godot-cpp, you need to use the companion PR godotengine/godot-cpp#1200

What works in my limited testing on Linux/Windows

• After re-compiling the extension, if you unfocus and then refocus the editor window, the extension will unload and then reload, replacing all the classes with their new versions
• It will reuse the method bind objects if the same method is re-added, so if GDScript or some other scripting language cached the method binds, they should keep working!
• The cached virtual method pointers will be cleared, so the new implementation will be picked up. (In my test project, I have it printing something in _process() and after reload it'll immediately start printing the new text!)
• Object properties will be saved and restored after the reload

How to test

• Compile Godot using the code from this PR.
• Update your language binding to support hot-reload:
  • With godot-cpp, use this PR: Changes necessary for hot reload to work godot-cpp#1200
  • Other language bindings need to be updated to pass GDExtensionClassCreationInfo2::recreate_instance_func which is used to create a new object defined by the GDExtension for a pre-existing object on the Godot side.
• Add reloadable = true to your .gdextension file

Downloading pre-compiled binaries:

1. Login into github
2. Click the "checks" tab of this PR
3. On the right there's an "artifacts" button.
4. Download the appropriate build for your plaform

---

TODO:

UPDATE: I think the remaining TODO's can be done in follow-ups. What's in this PR should provide a working basis that can be built upon!

• Actual "live reloading" where the editor detects that the shared library has been updated and automatically initiates the reload. In the current PR, you have to manually call GDExtensionManager.reload_extension(path)
• With the previous point, I think we probably want to allow GDExtensions to mark themselves as not being safe to reload. Or maybe each GDExtension should opt in? It's really only the developer of the GDExtension that wants live reloading - users of third-party GDExtensions probably don't want or need this, especially given all the extra accounting necessary to make it work.
• When methods are gone after the reload, it marks the method binds as "invalid" and if you try to call them an error is printed and it won't be called. However, if something is caching method binds, it should probably check if it's still valid, and if not, stop caching it! If GDScript or anything else in the engine is doing this, we should update it. This could be handled in a follow-up PR - what is in this PR is at least safe.
• Right now, it will update an existing method bind that has the same name, regardless of any similarity to the original method. We should probably do some sort of "compatibility check" and only reuse method binds if they have the same signature or something?
• Right now, it's trashing all the Object::_instance_bindings when clearing out the GDExtension data from an Object before reloading, but we really should be keeping instance bindings from other extensions that may be wrapping the object
• This PR adds a new property on the GDExtensionClassCreationInfo struct in gdextension_interface.h in a way that breaks compatibility. We've already discussed how to handle these sort of situations at GDExtension meetings in the past, I just need to update the PR to do it the right way :-)
• I'm sure this implementation has thread safety issues - needs to be evaluated and fixed, but this could be done in a follow-up PR.
• I'd love for another binding (Rust, Python, etc) to try this out and make sure it works there as well
• Lots and lots and lots of testing!!

Differences from @reduz's proposal:

This implementation is based on this proposed implementation from Juan:

https://files.godot.foundation/s/WcyHmRjLbsggyGL

However, there are a number of differences in this PR, which I'd like to explain!

A small flag needs to be present somewhere (Engine?) that indicates extension reloading can
take place. It could be Engine::is_extension_reloading_enabled().

I didn't include this, because I'm not entirely sure what this is for and how it should work. What determines if Engine::is_extension_reloading_enabled() should return true or false?

Right now, all the extra accounting necessary to make this work will only kick in where a GDExtension is actually in use, so it shouldn't have any effect if you're not using any GDExtensions. If we do add a way for GDExtensions to opt into reloading, we could make sure it isn't adding anything in that case either.

UPDATE: This is included now.

If extension reloading is possible, ClassDB must indicate that reloading will take place. A
function ClassDB::class_reload_set(const StringName& p_class,bool p_enable) may need to
exist to mark this class as that it will be reloaded (hence be aware that re-registration will
happen).

There's quite a bit in the proposal around making sure the class remains in the ClassDB (but marked special) during the reload process. I don't think this is actually necessary:

1. Since the GDExtension class intercepts all the register_extension_class(), register_extension_class_method() and unregister_extension_class() calls from the GDExtension before calling into ClassDB, my PR handles all the special cases for re-registering in GDExtension - ClassDB doesn't need to know about it
2. I think the class disappearing from ClassDB and then re-appearing a moment later is actually fine. We're clearing out the Object::_extension and Object::_extension_instance on all instances, so during this window when the class isn't in ClassDB, an existing instances will simply act like their native parent class. So, if your extension class extends Node2D, it will revert to acting like a plain old Node2D for a moment. The main thing that changes by the class not being in ClassDB, is that it can't be instantiated, but I think that's actually a good thing.
3. If we kept the class in ClassDB but marked it special, we'd need to add special cases all over the place to make sure ClassDB didn't do something unsafe while the class was reloading. But if the class just isn't there, ClassDB is already checking for that everywhere.

Only testing and more time will tell if I'm right or not, though :-)

Inside GDExtension::Extension, we will need to keep track of all Godot classes that have
instantiated this gdextension. For this, we can hook up to ObjectGDExtension::create_instance
and ObjectGDExtension::free_instance (by adding our own static function wrapper in that
object that replaces the one returned by the extension and that then calls back to it).

I attempted to implement this, but it won't actually work. For ObjectGDExtension::create_instance this approach would work just fine, because the function pointer from the GDExtension will return a GDExtensionObjectPtr (which is really just an Object *), so we can get ahold of the Godot-side instance. But ObjectGDExtension::free_instance is passed a GDExtensionClassInstancePtr which is the GDExtension-side instance, and that isn't what we need.

So, instead I added some additional function pointers on ObjectGDExtension for tracking.

If a class is gone during a hot reload, we need to create a fake class that can take the data. This
means mostly disabling existing method binds registered to that class, creating a fake generic
class that can be used for all reloads that interfaces to ObjectGDExtension and
GDExtensionClassInstancePtr. The class can contain a generic dictionary that takes
reads/writes and can work similar to the placeholder classes (ie InstancePlaceholder).

The day after sending his proposal, Juan came back and said he thinks this might not be necessary, so my PR is leaving it out for now to see how it works.

Copy DLL files before opening

@vnen has a separate PR (#80188) implementing this.

[reduz]

Looks fantastic! Some comments on your text:

With the previous point, I think we probably want to allow GDExtensions to mark themselves as not being safe to reload. Or maybe each GDExtension should opt in? It's really only the developer of the GDExtension that wants live reloading - users of third-party GDExtensions probably don't want or need this, especially given all the extra accounting necessary to make it work.

Maybe this can be something in the configuration file, so its easy to change for development.

If GDScript or anything else in the engine is doing this, we should update it.

For GDScript maybe when running the editor, it should just not cache the MethodBinds of extension classes that are marked as reloadable (extension could mark this in ClassDB) and use the more traditional call pathway. This would avoid breaking them @vnen ?

I'm sure this implementation has thread safety issues - needs to be evaluated and fixed!

I think this is kind of unavoidable. In large part, this is why the editor avoids using threads for most stuff.

I didn't include this [flag indicating extensions can reload] , because I'm not entirely sure what this is for and how it should work. What determines if Engine::is_extension_reloading_enabled() should return true or false?

Well, its for when you run the game and not the editor, as examples:

• GDVIRTUAL_TRACK should not do anything on the running game. In my original proposal I put a bool in there.
• GDScript should not avoid the MethodBinds of reloadable classes for performance.

Unless, of course, you want to actually reload extensions on the running game, which may be cool but not clear if desired.

There's quite a bit in the proposal around making sure the class remains in the ClassDB (but marked special) during the reload process. I don't think this is actually necessary.

If you have another extension that inherits classes from this one, then you want to keep the inheritance chain properly in ClassDB and not remove it during reload.

[dsnopek]

Thanks!

Unless, of course, you want to actually reload extensions on the running game, which may be cool but not clear if desired.

I have tested it game, and it does work with my very simple test project. :-)

However, I agree that in game is harder with a more complex project, so, I'll add a flag to disable it there.

If you have another extension that inherits classes from this one, then you want to keep the inheritance chain properly in ClassDB and not remove it during reload.

Hm, this is a very tricky case to solve. What happens if the GDExtension that provides the parent class reloads, and that class doesn't come back? Can we even support reloading in this context?

The more I think about it, I think GDExtensions should have to opt in to reloading (I'm thinking a reloadable property in the .gdextension file), and we should mark the classes in ClassDB as reloadable (which will also allow GDScript to treat them special per your earlier suggestion) and then prevent other GDExtensions from extending a reloadable class.

I think this should be OK since reloading is only used when developing the GDExtension, and during that phase you probably don't actually want other GDExtension extending its classes. After you're no longer actively developing the "parent GDExtension", you can disable reloadability, and then other GDExtensions can extend its classes again.

[reduz]

Hm, this is a very tricky case to solve. What happens if the GDExtension that provides the parent class reloads, and that class doesn't come back? Can we even support reloading in this context?

If it does not come back, I would still leave the class around. This is why I also suggested leaving the MethodBinds around even though invalidated in ClassDB. But I think this is a pessimistic use case.

I mean, obviously it would not work properly if you remove the class but, if you think about it, this is a very corner case. The reason you leave it in ClassDB is mainly because the most common case is that you will reload the class and it will come back, hence those inheriting from it should continue working. The feature is more geared to the optimistic use case, which should be the norm.

[dsnopek]

The reason you leave it in ClassDB is mainly because the most common case is that you will reload the class and it will come back, hence those inheriting from it should continue working. The feature is more geared to the optimistic use case, which should be the norm.

Sure. But I'm not convinced that supporting this one use case (reloading a GDExtension that has another GDExtension extending one or more of its classes) is compelling enough for all the added complexity.

So, I'm going to leave this out of the PR for now, and focus on finishing all the rest of the things that need to get done (which is still quite a lot). If further testing/review shows that this is really needed, I can always add it later, before the PR is merged :-)

[reduz]

@dsnopek I think its not very difficult, but if it makes things easier for you go ahead, can always be added later.

[BastiaanOlij]

Went through the code together with David to do a bit of a deep dive.

This is looking really good, all makes sense and so far testing of this logic give a solid feel.

Suggest we merge this before the feature freeze, this is going to be a game changer for many GDExtension developers.

[akien-mga]

Thanks! Amazing work 🎉

[dandersch]

Unless, of course, you want to actually reload extensions on the running game, which may be cool but not clear if desired.

I have tested it game, and it does work with my very simple test project. :-)

However, I agree that in game is harder with a more complex project, so, I'll add a flag to disable it there.

@dsnopek Any way to experimentally enable this again to see how feasible game hot-reloading is with GDExtension?

[dsnopek]

@dandersch:

Any way to experimentally enable this again to see how feasible game hot-reloading is with GDExtension?

No, although, I think it would be fairly easy to implement. Here's what I think it would take:

1. Modify main.cpp to recognize a command-line argument that would lead to Engine::get_singleton()->set_extension_reloading_enabled(true) even when not in the editor
2. Add some project setting to cause Godot to run the game with that command-line argument
3. Add something to the debugger protocol so that the editor could tell the game to reload an extension when the editor has detected that it has changed

I suspect there's some areas of the code that do Engine::get_singleton()->is_editor_hint() when they really should do Engine::get_singleton()->is_extension_reloading_enabled() that may need to get fixed up.

If I can find the time, I'll throw together a draft PR. However, if anyone else is motivated to give it a try, that'd be awesome :-)

[dsnopek]

@dandersch I just posted draft PR #97991 which does the basics I described above, but needs testing and probably some fixes on Windows