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.

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

Hot reloading does not reload nodes in editor viewport #1502

Open
Zacrain opened this issue Jun 23, 2024 · 5 comments
Open

Hot reloading does not reload nodes in editor viewport #1502

Zacrain opened this issue Jun 23, 2024 · 5 comments
Labels
bug This has been identified as a bug needs testing

Comments

@Zacrain
Copy link

Zacrain commented Jun 23, 2024

Godot version

v4.2.2.stable.official [15073afe3]

godot-cpp version

4.2 [9da6ecd]

System information

Windows 10, Intel Core i5-9300H

Issue description

Testing out the new hot-reloading feature, I have observed that changes do not appear in the editor view after recompiling a GDExtension. Or let's say, they only appear partly.

Two examples where I encountered this:

  • Attributes like changed text labels on buttons appear when running a scene, but not in editor view. This remains unchanged regardless of running the scene. (For example, set some text upon _ready() and change it every time a button is pressed.) The only fix I see so far is to reload the project.
  • Consider renaming a method binding for a signal: It will be shown in it's updated version when picking a method for a signal via the "Connect a Signal to a a Method" prompt, but not in the node's inspector. Even worse, Godot still considers the old name to exist and doesn't complain when running the scene, even though the method does no longer exist due to the renaming.

In order to visualize what I mean (I'll also provide a minimal reproducible example project), I'll detail those two examples now. Let's say we start with this in a button class:

void StartStopButton::_ready() {
    this->set_text("Start Moving");
}

void StartStopButton::_pressed() {
    if (!flipMode)
        this->set_text("Stop Moving");
    else
        this->set_text("Start Moving");

    flipMode = !flipMode;   
}

This is reflected in the editor view:
image

However, now change the text in the code, compile and see that the change is not reflected within the editor view. But, if the scene / game is run, the changes are correctly shown.
image

Now an example for renamed method bindings. Let's say we have a method like:

void GDExample::_on_startstop_button_pressed() {
	move = !move;
}

in the Sprite2D child. (And correctly bound in the _bind_methods() method.) This method is connected to the pressed() signal of the button. Rename it to something else, for example on_start_button_pressed, recompile and then we have this:

The old name of the connected method is still visible:
image

If I now double click on the signal in order to pick another method, and I'm looking for the renamed one, it is correctly listed:
image

Yielding two methods, if the connection to the old one is not removed:
image

Running the scene like that is totally fine. Which is good but also problematic as I would expect Godot to throw an error or display an error message if the outdated signal handler can not be called. Which it really can't as it does not exist anymore in the code and resulting library.

If the whole project is reloaded after recompiling the code, those issues disappear. But that's not what I would expect from hot reloading.

Steps to reproduce

  1. Create a C++ GDExtension project.
  2. Create some node which should have visible changes in the editor, like a labeled button or a method name for signal handling.
  3. Compile.
  4. Change something about it in the code, e.g., the text label or the method name.
  5. Compile again.
  6. See that the changes are not or only partly reflected in the editor, but running the game or scene is fine.

Minimal reproduction project

The minimal example, which I provided, should be fine to use for that. Note, that you need to change the env path in the SConstruct file as I placed the godot-cpp compiled C++ API in a different directory and left the path in a probably unusable state now. And of course, don't forget to compile otherwise Godot will not find the libraries as I have not bundled them with the zip archive.

GDExtension CPP Example V2.zip

@wakeofluna
Copy link

wakeofluna commented Aug 22, 2024

I had a similar, although not sure if it's the same, problem.

I have a C++ gdextension where I create a few Resource classes, and then a scene where I have several of those classes active. I observed the following issues after making changes followed by a hotreload:

  • If the scene is closed, everything works as expected
  • If the scene is open in the main editor, the resources that are active in the scene are not updated correctly and are showing odd behaviour, including the reported issue of callbacks not running.
  • If I then close the scene and reopen the scene, the resources (most of the time) work fine again with the new behaviour.

So I was doing some good ol' printf-debugging and found out the following with regard to the hot reloading sequence:

  • It correctly saves the exposed properties from my resources
  • It correctly calls the destructors of my resources
  • It correctly constructs new instances of my resources
  • It correctly assigns the properties back to my resources
  • I get a notification NOTIFICATION_EXTENSION_RELOADED firing on the new instances of my resources

So far so good. Especially that last step shows that the hot reload has succesfully tracked the new objects being created. Also when I compare values such as get_instance_id() it shows that my instances are correctly tied to the already existing Godot Object _owner. But whenever I create a Callable(this, ...) on my new instances then these fail complaining about objects being null pointers.

Long story, it turns out that godot-cpp calls into Godot with a call gdextension_interface_object_get_instance_binding() to lookup my instance pointer that is tied to the _owner, but this binding has been cleared by the unloading of library.

Possible solution

There is a small kludge in the Wrapped constructor that upon hot reload restores the link to the original _owner. I think at this time, we also need to restore the instance_binding to the new this-pointer. This is also required to call the correct destructor (free_callback) for the new instances.

I made a small change to wrapped.cpp to accomplish this:

diff --git a/src/classes/wrapped.cpp b/src/classes/wrapped.cpp
index ffca4f9..3728d25 100644
--- a/src/classes/wrapped.cpp
+++ b/src/classes/wrapped.cpp
@@ -61,6 +61,10 @@ Wrapped::Wrapped(const StringName p_godot_class) {
                while (recreate_data) {
                        if (recreate_data->wrapper == this) {
                                _owner = recreate_data->owner;
+                               if (likely(_constructing_class_binding_callbacks)) {
+                                       godot::internal::gdextension_interface_object_set_instance_binding(_owner, godot::internal::token, this, _constructing_class_binding_callbacks);
+                                       _constructing_class_binding_callbacks = nullptr;
+                               }
                                if (previous) {
                                        previous->next = recreate_data->next;
                                } else {

I don't know for sure if this fix is complete, but it at least fixes my problem.

EDIT: my version information:

  • Godot Engine v4.3.rc.gentoo.3978628c6
  • godot-cpp 4.3 [https://github.com/godotengine/godot-cpp/commit/8b80d9146bc4773e8d49b59ada64539972e3a4f0]

@wakeofluna
Copy link

Also see #1589

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 17, 2024

I just posted PR #1590 which should fix this! If anyone has time to test it with their project, I'd appreciate it :-)

@Zacrain
Copy link
Author

Zacrain commented Sep 21, 2024

I just posted PR #1590 which should fix this! If anyone has time to test it with their project, I'd appreciate it :-)

Nope, sadly did not resolve the issue. :(
Tested with a minimal example similar to the one provided in the issue here

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 26, 2024

Ok, thanks for testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug needs testing
Projects
None yet
Development

No branches or pull requests

4 participants