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

Changes necessary for hot reload to work #1200

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Aug 5, 2023

These are the changes necessary to work with Godot PR godotengine/godot#80284

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Aug 5, 2023
@dsnopek dsnopek requested a review from a team as a code owner August 5, 2023 02:17
@dsnopek dsnopek marked this pull request as draft August 5, 2023 02:23
@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 5, 2023

Hm, this worked in my testing because my test classes declared their own constructors without any arguments. This is failing the tests because they don't, and just rely on the default constructor being inherited from the parent class. But now that this PR adds a secondary constructor, the default won't be inherited anymore. :-/

I'm not yet sure what a good solution would be...

@dsnopek dsnopek force-pushed the hot-reload branch 3 times, most recently from ee8180d to 102867a Compare August 12, 2023 15:47
@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 12, 2023

I came up with a "hack" to workaround the problem described in my last comment.

Rather than adding a new constructor that can handle recreating instances, it adds the info about any instances being recreated to a global list, and then the usual constructor will check if this instance is meant to be recreated, and if so, do that process instead. Since this is a little ugly, I've wrapped this in a HOT_RELOAD_ENABLED define, that by default will only be set for debug and editor builds. The thing I'm most unsure about is the data structure used to hold the list, so suggestions on better ways to do that welcome.

@dsnopek dsnopek force-pushed the hot-reload branch 2 times, most recently from fcd114b to 7fa6875 Compare September 1, 2023 21:46
@dsnopek dsnopek marked this pull request as ready for review September 1, 2023 22:33
@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 1, 2023

Taking this one out of draft because the Godot PR has also come out of draft, but it won't pass tests (and can't be merged) until Godot PR godotengine/godot#80284 is merged

@dsnopek dsnopek changed the title [DRAFT] Changes necessary for hot reload to work Changes necessary for hot reload to work Sep 1, 2023
@dsnopek dsnopek force-pushed the hot-reload branch 2 times, most recently from b78da0e to 645e607 Compare September 12, 2023 17:09
@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 12, 2023

Temporarily rebasing on PR #1239 to keep this compatible with the Godot PR. Hopefully, that one will be merged soon-ish and this can go back to being a single commit

@dsnopek dsnopek force-pushed the hot-reload branch 3 times, most recently from 5ed708a to 2f77bcf Compare September 15, 2023 17:49
@DmitriySalnikov
Copy link
Contributor

I have updated the library. Now there is no crash in godot::Wrapped::Wrapped, but there remained random crashes after several reloads.
I also noticed that only _enter_tree is called for EditorPlugin, and _exit_tree, _disable_plugin and _enable_plugin don't react to reloading in any way (but I can use a destructor to deinitialize). Perhaps it is implied that it is a Node and should simply replace the pointer to the new library. But if so, then somehow I need to know that it doesn't need to be deleted during deinitialization.

Also a re-registered singleton still causes:
(here I added the print of the singleton id from the library and script)

GD: <Freed Object>
SCRIPT ERROR: Invalid call. Nonexistent function 'clear_graphs' in base 'previously freed'.
          at: _process (res://examples_dd3d/DebugDrawDemoScene.gd:73)
[Wrapped:0]

@DmitriySalnikov
Copy link
Contributor

DmitriySalnikov commented Sep 16, 2023

I want to add information about singleton.
If I replace the standard singleton getting via the class name with Engine.get_singleton, then the calls work without errors.

# work
Engine.get_singleton(&"DebugDraw3D").draw_box(box_pos, Vector3(1, 2, 1), Color(0, 1, 0))
# doesn't work
DebugDraw3D.draw_box(box_pos, Vector3(1, 2, 1), Color(0, 1, 0))
godot.windows.editor.dev.x86_64.mono_cQLdGs8EOE-00.00.17.000-00.00.26.583.mp4

Also, if I use my bindings for C#, then after a reload everything also continues to work. This is because I check the validity of the cached instance and try to get it if it is null or invalid.

    private static GodotObject _instance;
    public static GodotObject Instance
    {
        get
        {
            if (!GodotObject.IsInstanceValid(_instance))
            {
                _instance = Engine.GetSingleton("DebugDraw3D");
            }
            return _instance;
        }
    }
godot.windows.editor.dev.x86_64.mono_ulWApN7KQa-00.00.21.000-00.00.32.683.mp4

It turns out that GDScript just doesn't update the singleton cache somewhere.

v4.2.dev.mono.custom_build [d1eba6a1d]
godot-cpp 90ad720

@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 16, 2023

@DmitriySalnikov: Thanks to all your testing and reports I've managed to find and fix a whole bunch of bugs!

I also noticed that only _enter_tree is called for EditorPlugin, and _exit_tree, _disable_plugin and _enable_plugin don't react to reloading in any way (but I can use a destructor to deinitialize).

This should be fixed with the latest versions of both PRs: you should get _exit_tree when the editor plugin is being deinitialized, before the destructor runs. I don't know that _disable_plugin and _enable_plugin are appropriate here? The config for the project hasn't actually changed (ie. the plugin would still be enabled if it was enabled before the reload). But we need to look at more editor plugins and see what they're expecting to happen with regard to those virtual functions.

I want to add information about singleton.
If I replace the standard singleton getting via the class name with Engine.get_singleton, then the calls work without errors.

Hm, interesting, good find! I haven't had a chance to look into this one yet, but I guess we'd need the Godot PR to signal to GDScript to clear its singleton cache somehow.

@DmitriySalnikov
Copy link
Contributor

you should get _exit_tree when the editor plugin is being deinitialized

Thanks, I've already checked it out. Now the plugin is removed as if the engine was closed in the usual way without a reloadable.

Copy link
Collaborator

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, need to wait on upstream merge and nitpicky rename :)

gdextension/gdextension_interface.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants