Fix: Ensure class servers methods are loaded after hot reload

[kang-sw]

This pull request addresses issue #629.

Background

Based on a preliminary analysis, the following observations were made (note that these findings may not be entirely accurate):

1. Upon the initial load of the DLL, gdext_on_level_init is invoked in the order of InitLevel::Servers followed by InitLevel::Scene.
2. When the DLL is rebuilt, the editor detects the updated DLL and initiates a hot reload.
3. Before hot reloading, all nodes created within the editor are removed (as verified by calling exit_tree on instances of classes registered through gdext).
4. Upon loading the new DLL, gdext_on_level_init(InitLevel::Scene) is immediately called for the new DLL memory space, skipping the call to InitLevel::Servers.
5. As a result, attempting to reference class_servers_api in the editor plugin after a hot reload leads to a panic due to the missed InitLevel::Servers call.

Workaround

The core issue appears to stem from upstream not invoking the initialization function after hot reloading. The ideal solution would involve upstream sequentially calling class initialization functions following a hot reload. However, as a temporary workaround, we ensure that the class servers API is loaded at least once by the library, even if InitLevel::Server is skipped.

Questions

• Following a hot reload, the call to crate::auto_register_classes(sys::ClassApiLevel::Core) is omitted. Should a similar lazy loading approach be applied to InitLevel::Core?
• gdext_on_level_init seems to be guaranteed to be called on the main thread. To prevent re-entry, is it acceptable to reduce overhead by using unsafe static mut or AtomicBool instead of std::sync::Mutex?

Given my limited understanding of the internal architecture, I am uncertain about the potential side effects this may entail. Your review and feedback would be greatly appreciated.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-636

[Bromeon]

Thanks a lot! Yesterday I tried your MRE from #629 and could reproduce the crash.

I added logging to ExtensionLibrary::on_[de]init():

#[gdextension]
unsafe impl ExtensionLibrary for MyExtension {
    fn on_level_init(level: InitLevel) {
        println!("on_level_init: {:?}", level);
    }

    fn on_level_deinit(level: InitLevel) {
        println!("on_level_deinit: {:?}", level);
    }
 }

With that, I observed the following:

<initial load>
Initialize godot-rust (API v4.2.stable.official, runtime v4.3.dev.custom_build)
on_level_init: Servers
on_level_init: Scene
on_level_init: Editor
<run editor>
on_level_deinit: Editor
on_level_deinit: Scene
on_level_deinit: Servers
on_level_deinit: Core
<hot reload>
Initialize godot-rust (API v4.2.stable.official, runtime v4.3.dev.custom_build)
on_level_init: Scene
on_level_init: Editor

So as you say, there is a discrepancy that Servers/Core levels are unloaded, but not loaded again.

I also looked at the "minimum level": it determines from which level Godot has to reload. I tried to lower the level to Servers:

#[gdextension]
unsafe impl ExtensionLibrary for MyExtension {
    fn min_level() -> InitLevel {
        InitLevel::Servers
    }

    ...
 }

And while it no longer crashes, it simply doesn't reload, not even the print statements are executed anymore. The C header documents:

	/* Minimum initialization level required.
	 * If Core or Servers, the extension needs editor or game restart to take effect */
	GDExtensionInitializationLevel minimum_initialization_level;

so hot reload will not work with any level below Scene.

In conclusion, I think it is a Godot bug that all levels are unloaded but only some reloaded. Would you like to report it upstream in the Godot issues and link to #629? Let me know if I can help somehow.

I'll write more about a workaround in a separate response.

[Bromeon]

Workaround

I agree it makes sense to solve this problem already, so people can use Godot 4.2 properly.

---

gdext_on_level_init seems to be guaranteed to be called on the main thread. To prevent re-entry, is it acceptable to reduce overhead by using unsafe static mut or AtomicBool instead of std::sync::Mutex?

You can definitely use AtomicBool, Mutex<bool> has no advantages here.

I would not go for anything unsafe, as this is not a hot path (hot reload doesn't happen 1000s of times per frame), and any other logic involved takes orders of magnitude more time.

---

Following a hot reload, the call to crate::auto_register_classes(sys::ClassApiLevel::Core) is omitted. Should a similar lazy loading approach be applied to InitLevel::Core?

Yes, that would be great! There is currently no class table loaded in Core, but we might add other logic and it would be cool if we didn't have to fix it again.

I wonder if we should change the logic, so we have

static LEVEL_SERVERS_CORE_LOADED: AtomicBool = AtomicBool::new(false);

fn try_load_level<E: ExtensionLibrary>(level: InitLevel) {
    // Workaround for https://github.com/godot-rust/gdext/issues/629:
    // When using editor plugins, Godot may unload all levels but only reload from Scene upward.
    // Manually run initialization of lower levels.
    if level == InitLevel::Scene {
        let lower_levels_loaded = LEVEL_SERVERS_CORE_LOADED.swap(true);
        if !lower_levels_loaded {
            try_load_level::<E>(InitLevel::Servers);
            try_load_level::<E>(InitLevel::Core);
        }
    }

    // Regular initialization routine (including user callback):
    gdext_on_level_init(level);
    E::on_level_init(level);
}

And unset the bool on unload again. Not sure if individual AtomicBool instances are easier, but I think this problem is specific enough to handle it in the above way.

[kang-sw]

Committed bcc82de.

• Remove Mutex usage, replaced into AtomicU8
• Since it's workaround for upstream behavior, add one more bulletproof logic, instead of binding cores/servers into single boolean flag:
  • Split global initialization levels into 3 steps => nothing loaded, core loaded, servers loaded.
  • When Scene level is initialized, it checks for Servers. For Servers, it checks for Core. (recursively)
  • Assume Editor level never be initialized before Scene level is initialized

[Bromeon]

About the logic -- do we gain anything by having 3 states (core, servers, scene) instead of 2 (core+servers, scene)? Because it seems like this bug explicitly skips both core+server states, not just either of them.

So we should maybe make the code embrace this specific case rather than general "forgot to init" scenarios? That would also signal us if there's another initialization bug, and allow to report it to Godot.

Then we could also use AtomicBool -- see my previous code snippet 🤔

[kang-sw]

So we should maybe make the code embrace this specific case rather than general "forgot to init" scenarios? That would also signal us if there's another initialization bug, and allow to report it to Godot.

Ah, now I got that point. Agree that this is a bug should be resolved from upstream. I'll update the code.

[Bromeon]

Thanks! Looks mostly good, one comment left 🙂
Could you also squash the commits?

[Bromeon]

Thanks a lot for your contribution! ❤️