Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

C++ Hot Reload Regressions in 4.2.2 and 4.3 #1589

Closed
kromenak opened this issue Sep 15, 2024 · 6 comments · Fixed by #1590
Closed

C++ Hot Reload Regressions in 4.2.2 and 4.3 #1589

kromenak opened this issue Sep 15, 2024 · 6 comments · Fixed by #1590
Assignees
@dsnopek
Labels
bug This has been identified as a bug confirmed regression topic:gdextension This relates to the new Godot 4 extension implementation

Comments

@kromenak
Copy link

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: Problems with hot reload #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

  1. Open the Godot project in Godot_4.2.1/Res.
  2. 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.
  3. 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.
  4. Build the C++ solution. This should update the DLL at Godot_4.2.1/Res/addons/CppExtensionDebug.dll.
  5. 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.

  1. Open the Godot project in Godot_4.3/Res.
  2. 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.
  3. 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.
  4. Build the C++ solution. This should update the DLL at Godot_4.3/Res/addons/CppExtensionDebug.dll.
  5. 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
Copy link
Author

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 akien-mga added bug This has been identified as a bug needs testing topic:gdextension This relates to the new Godot 4 extension implementation regression labels Sep 16, 2024
@akien-mga
Copy link
Member

CC @godotengine/gdextension

@dsnopek dsnopek self-assigned this Sep 16, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Sep 16, 2024

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
Copy link
Collaborator

dsnopek commented Sep 16, 2024

Git bisect points to godot-cpp PR #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 akien-mga transferred this issue from godotengine/godot Sep 17, 2024
@akien-mga
Copy link
Member

Transferred to godot-cpp.

@wakeofluna wakeofluna mentioned this issue Sep 17, 2024
Open
@dsnopek dsnopek mentioned this issue Sep 17, 2024
Merged
@dsnopek
Copy link
Collaborator

dsnopek commented Sep 17, 2024

I just posted PR #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 :-)

@Bromeon Bromeon mentioned this issue Oct 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dsnopek dsnopek

Labels
bug This has been identified as a bug confirmed regression topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Correctly set instance bindings on reload dsnopek/godot-cpp
3 participants
@dsnopek @kromenak @akien-mga