C++ Hot Reload Regressions in 4.2.2 and 4.3

kromenak commented 5 days ago

Tested versions

Tested hot reload working successfully in 4.2.1 on Windows 10 and macOS 14.6.1
Tested hot reload failing to work successfully in 4.3 on Windows and macOS 14.6.1

System information

Godot v4.3.stable - Windows 10.0.19045 - Vulkan (Mobile) - dedicated NVIDIA GeForce GTX 1650 SUPER (NVIDIA; 31.0.15.3623) - Intel(R) Core(TM) i5-3350P CPU @ 3.10GHz (4 Threads)

Issue description

We are working on a project using C++ GDExtension. When we started the project in Godot 4.2.1, we found the hot reload functionality to work quite well on both Windows and Mac dev machines. All developers could iterate quickly by recompiling C++ code, switching back to the editor, and seeing changes immediately.

However, after updating to Godot 4.2.2, Windows devs complained that hot reload could cause the editor to crash, or could result in the new DLL not loading correctly, resulting in extension classes disappearing from the scene, or inspector hookups getting deleted/removed. On Windows, it seemed to sometimes work, sometimes not. On Mac, hot reload continued to work fine.

Now, in Godot 4.3, hot reload seems to be failing on BOTH Mac and Windows in a few ways:

As with 4.2.2, Windows developers are reporting occasional situations where the editor either crashes or seems to not load the updated DLL successfully, resulting in corrupted scenes that require an editor restart.
On Mac, hot reload now consistently doesn't work. On reload, all extension classes fail to load, leaving the scene in a corrupted state that requires an editor restart. A similar issue was reported on the godot-cpp side: https://github.com/godotengine/godot-cpp/issues/1573
When hot reload DOES work on the Windows side, the behavior is now wrong in a few noticeable ways:
- An EditorNode3DGizmo plugin that we use to visualize collision areas in the scene stops working on hot reload in 4.3, requiring closing and reopening a scene to get it working again. Back in 4.2.1, it still worked after hot reload.
- Inspector connections to Resources in the extension are reset after hot reload in 4.3, requiring an editor restart to fix. Back in 4.2.1, these connections were maintained after hot reloads.

In short, the hot reload behavior for C++ GDExtensions seems to have really deteriorated in the last two big Godot releases.

Steps to reproduce

In Godot 4.2.1

Open the Godot project in Godot_4.2.1/Res.
Open test_scene.tscn. Notice the blue box around the image (the gizmo plugin). Also notice the root Node has an inspector hookup called "Current Attack Data" that is hooked up to custom Resource.
Generate the C++ project in Godot_4.2.1/Cpp using CMake. There is a helper script in Cpp/Build to generate a VS solution quickly and easily.
Build the C++ solution. This should update the DLL at Godot_4.2.1/Res/addons/CppExtensionDebug.dll.
Switch back to the editor. Notice that the blue box gizmo still functions, and that the "Current Attack Data" hookup is still connected.

In Godot 4.3 Note that the ONLY difference about the 4.3 project is that it is meant to be opened in Godot 4.3, and it uses the 4.3 version of godot-cpp. It is otherwise IDENTICAL to the 4.2.1 version.

Open the Godot project in Godot_4.3/Res.
Open test_scene.tscn. Notice the blue box around the image (the gizmo plugin). Also notice the root Node has an inspector hookup called "Current Attack Data" that is hooked up to custom Resource.
Generate the C++ project in Godot_4.3/Cpp using CMake. There is a helper script in Cpp/Build to generate a VS solution quickly and easily.
Build the C++ solution. This should update the DLL at Godot_4.3/Res/addons/CppExtensionDebug.dll.
Switch back to the editor. Notice that the blue box gizmo disappears. Also notice that the "Current Attack Data" hookup has become "". Saving the scene at this point causes the connection to be lost and saved to disk.

Minimal reproduction project (MRP)

HotReloadBug.zip

kromenak commented 5 days ago

I've tried to investigate some of these problems in the source code for godot and godot-cpp. Unfortunately, I'm not very familiar with these systems, but here's some info I was able to find, in case it's helpful.

Inspector Hookup Failing on Hot Reload The exact case here might be when an extension class instance has a property that is also an extension class. In my sample project, the scene has an Actor node (an extension class that inherits Node3D). Actor has an exposed property for AttackData (also an extension class inheriting from Resource). This issue with "losing connections" has not occurred if the inspector hookup is for a built-in class, such as AudioStream.

I stepped through this in the debugger and found this:

In GDExtension::prepare_reload, the Resource does appear to be retrieved and stored in instance_state successfully.
However, in GDExtension::finish_reload, the call to obj->set DOES NOT successfully set the Resource back after hot reload, even though the state.second variable appears to be correct.
I ultimately traced the failure to set the property down to the "cast variant to Object*" call in variant.cpp of godot-cpp. It appears the call to internal::get_object_instance_binding is returning null, even though the object pointer is present.
```
Variant::operator Object *() const {
GodotObject *obj;
to_type_constructor[OBJECT](&obj, _native_ptr());
if (obj == nullptr) {
    return nullptr;
}
return internal::get_object_instance_binding(obj);
}
```
I don't fully understand the "object instance binding" system, so I'm unsure why this is failing.

akien-mga commented 4 days ago

CC @godotengine/gdextension

dsnopek commented 3 days ago

Thanks!

I'm able to reproduce the issue with your MRP.

It seems like the problem is in godot-cpp (rather than Godot) because switching to the godot-4.2.1-stable tag of godot-cpp but using Godot v4.3-stable seems to work. However, I'll understand it better after I have a chance to git bisect,

dsnopek commented 3 days ago

Git bisect points to godot-cpp PR https://github.com/godotengine/godot-cpp/pull/1446

I think we missed needing to set the instance binding when doing a reload. I'll work on a PR to fix it.

akien-mga commented 3 days ago

Transferred to godot-cpp.

dsnopek commented 3 days ago

I just posted PR https://github.com/godotengine/godot-cpp/pull/1590 which should fix this! I already tested with the MRP, but if anyone has time to test it with their project, I'd appreciate it :-)

godotengine / godot-cpp