Hot reload support #437

kkolyan · 2023-09-30T02:17:46Z

Essentially, that's the Rust version of godotengine/godot-cpp#1200
Targeted to fix #434

kkolyan · 2023-09-30T02:18:20Z

Not tested yet

GodotRust · 2023-09-30T07:37:51Z

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

Bromeon

Thanks a lot, great update! 👍

Some feedback:

Can you elaborate what create_rust_counterpart exactly does and maybe add a comment?
Do you think it's possible to test this somehow?
Please keep all symbols coming from the godot-ffi crate qualified with sys::, this is a deliberate choice to make FFI symbols explicit. So there should be no imports starting with godot_ffi:: or sys::.
This might need rebase onto master after #436 is merged, which fixes a few things.

kkolyan · 2023-09-30T17:46:14Z

Let's say that existing method create_custom (https://github.com/godot-rust/gdext/blob/master/godot-core/src/registry.rs#L336) consists of two steps:

create Godot object instance
create user's Rust object instance and attach it to the supplied Godot object instance to effectively subclass it in terms of Godot.

Second part does perfectly match what we need in recreate callback, expected by Godot for reloading. So I extracted it and called create_rust_counterpart. There is no new functionality there.

Is create_rust_part_for_existing_godot_part name better?

kkolyan · 2023-10-01T01:47:56Z

Do you think it's possible to test this somehow?

For now - manually is the most convenient. For future - maybe some unit tests could be added/updated, I'll look later.

Please keep all symbols coming from the godot-ffi crate qualified with sys::, this is a deliberate choice to make FFI symbols explicit. So there should be no imports starting with godot_ffi:: or sys::.

Done

auto_register_classes flow is updated too. register_classes was updated in first commit, but it's not clear if this flow is not dead.

Will make separate commits for now and squash everything when all is done.

kkolyan · 2023-10-01T22:25:56Z

added classes unloading and seems like it works. tested manually against godotengine/godot#82603. thinking about automated tests.

Bromeon · 2023-10-03T08:14:56Z

I think this would need some #[cfg(before_api = "4.2")] special casing, as the integration tests for 4.0.x currently fail.

cargo clippy/doc/test unfortunately also pull in the Godot 4.1.1 generated sources by default; as such they don't have this symbol yet 🤔 We either need to modify CI to accomodate for this, or make sure only the stable path (4.1.1) is run in cargo clippy/doc/test. I'll think about this...

Rebasing onto master should at least fix the doc issue.

Bromeon

Turns out I was completely wrong, your code was just missing some #[cfg(before_api = "4.2")] statements for symbols depending on new code.

I fixed this and also rebased onto master. Feel free to integrate my commit into yours, no need to retain my author information or so. I kept it separate for you to see what I changed, but there's no strict need to be like that in the merge.

Added some feedback, mostly smaller bikeshedding 😉 thanks a lot for everything, looks like solid work overall!

Bromeon · 2023-10-04T21:21:44Z

godot-core/src/registry.rs

 use std::{fmt, ptr};

+static LOADED_CLASSES: Mutex<Option<HashMap<InitLevel, Vec<ClassName>>>> = Mutex::new(None);


I know why it's needed, but I do hate Rust's excessive boilerplate with global variables...

Do you know if this can ever be accessed concurrently? Probably only in case of a Godot bug?

It's probably better to be safe and sorry, since this is not a critical path; as such I appreciate also your panicking and descriptive errors on unlock failure further below.

Probably yeah, due to some bug on Godot side (after some major rework on Godot side). Now they are called from one thread (register, unregister). I'm glad you share my position regarding panic in case of theoretically possible multithreaded access from engine side.

Thanks a lot for debugging this! Could you add as a comment:

The fact that they are only accessed from a single thread at the moment.

The rationale why we still use mutex + panic:

"better safe than sorry" part

hot reloading is very involved anyway, so it's not performance-critical or a hot loop.

Bromeon · 2023-10-04T21:24:47Z

godot-core/src/registry.rs

    }

    out!("All classes for level `{init_level:?}` auto-registered.");
 }

+pub fn unregister_classes(init_level: InitLevel) {
+    let mut loaded_classes = get_loaded_classes_with_mutex();
+    let loaded_classes = loaded_classes.get_or_insert_with(Default::default);


Using HashMap::new instead of Default::default is a bit clearer.

Bromeon · 2023-10-04T21:30:04Z

godot-core/src/registry.rs

+    let mut loaded_classes = get_loaded_classes_with_mutex();
+    let loaded_classes = loaded_classes.get_or_insert_with(Default::default);


Maybe name loaded_class_map to differentiate from the further down used loaded_classes representing a vector.

Bromeon · 2023-10-04T21:31:06Z

godot-core/src/registry.rs

+pub fn unregister_classes(init_level: InitLevel) {
+    let mut loaded_classes = get_loaded_classes_with_mutex();
+    let loaded_classes = loaded_classes.get_or_insert_with(Default::default);
+    let loaded_classes = loaded_classes.remove(&init_level).unwrap_or_default();


Naming both these things loaded_classes is a bit confusing. As above, I'd name the map loaded_class_map.

Bromeon · 2023-10-04T21:31:44Z

godot-core/src/registry.rs

+        unsafe {
+            #[allow(clippy::let_unit_value)]
+            let _: () = interface_fn!(classdb_unregister_extension_class)(
+                sys::get_library(),
+                class_name.string_sys(),
+            );
+        }


This could be extracted to a new function, next to register_class_raw.

Bromeon · 2023-10-04T21:34:01Z

godot-core/src/registry.rs

@@ -263,6 +366,7 @@ fn fill_class_info(component: PluginComponent, c: &mut ClassRegistrationInfo) {
            c.is_editor_plugin = true;
        }
    }
+    // c.godot_params.unwrap().


Is this needed?

Just in case you missed it 🙂

Bromeon · 2023-10-04T21:34:29Z

godot-core/src/registry.rs

        let _: () = interface_fn!(classdb_register_extension_class)(
            sys::get_library(),
            class_name.string_sys(),
            parent_class_name.string_sys(),
            ptr::addr_of!(info.godot_params),
        );
+        #[cfg(since_api = "4.2")]


Maybe empty line before this to highlight related blocks.

Also this one 🙂

Bromeon · 2023-10-04T21:36:14Z

godot-core/src/registry.rs

+        create_rust_part_for_existing_godot_part(T::__godot_init, object)
+    }
+
+    pub(crate) fn create_custom<T, F>(make_user_instance: F) -> godot_ffi::GDExtensionObjectPtr


This line shouldn't change -- use sys:: prefix everywhere, not godot_ffi::.

Bromeon · 2023-10-04T21:37:39Z

godot-core/src/registry.rs

+    fn create_rust_part_for_existing_godot_part<T, F>(
+        make_user_instance: F,
+        base_ptr: sys::GDExtensionObjectPtr,
+    ) -> sys::GDExtensionClassInstancePtr


Thanks for your elaboration in comment. I think this method would benefit from a short doc.

kkolyan · 2023-10-07T22:57:53Z

I've made some fixes after rebase. Commits are left for visibility, but I'll squash them later.

About auto-testing of this feature. I've checked test module itest and impressed that there is such a good coverage.

The problem about this feature is that it is editor-level, but itest is game-level.

I see the following approach:

create an editor script that does the following:
1.1. runs itests (editor-compatible part of them)
1.2. replaces library with pre-built slightly modified copy (maybe modification timestamp is enough), probably with delay.
1.3. runs itests again
1.4. exits
create a scene with that tool
create github action that runs the editor, waits for termination and checks output for errors

Whether the game is worth the candle?

Bromeon

Thanks for the update! There are a few unaddressed comments from an earlier review, I marked them.

Apart from that, looks mostly good, I think you can start squashing to a single commit. There are also a few questions for my understanding that don't warrant a change in the code itself.

Bromeon · 2023-10-09T14:58:03Z

godot-core/src/registry.rs

 use std::{fmt, ptr};

+static LOADED_CLASSES: Mutex<Option<HashMap<InitLevel, Vec<ClassName>>>> = Mutex::new(None);


Thanks a lot for debugging this! Could you add as a comment:

The fact that they are only accessed from a single thread at the moment.

The rationale why we still use mutex + panic:

"better safe than sorry" part

hot reloading is very involved anyway, so it's not performance-critical or a hot loop.

Bromeon · 2023-10-09T14:59:40Z

godot-core/src/registry.rs

@@ -263,6 +366,7 @@ fn fill_class_info(component: PluginComponent, c: &mut ClassRegistrationInfo) {
            c.is_editor_plugin = true;
        }
    }
+    // c.godot_params.unwrap().


Just in case you missed it 🙂

Bromeon · 2023-10-09T14:59:57Z

godot-core/src/registry.rs

        let _: () = interface_fn!(classdb_register_extension_class)(
            sys::get_library(),
            class_name.string_sys(),
            parent_class_name.string_sys(),
            ptr::addr_of!(info.godot_params),
        );
+        #[cfg(since_api = "4.2")]


Also this one 🙂

Bromeon · 2023-10-09T15:00:17Z

godot-core/src/registry.rs

+        create_rust_part_for_existing_godot_part(T::__godot_init, object)
+    }
+
+    pub(crate) fn create_custom<T, F>(make_user_instance: F) -> godot_ffi::GDExtensionObjectPtr


Bromeon · 2023-10-09T15:00:21Z

godot-core/src/registry.rs

+    fn create_rust_part_for_existing_godot_part<T, F>(
+        make_user_instance: F,
+        base_ptr: sys::GDExtensionObjectPtr,
+    ) -> sys::GDExtensionClassInstancePtr


Bromeon · 2023-10-09T15:03:03Z

godot-macros/Cargo.toml

+
+[build-dependencies]
+godot-bindings = { path = "../godot-bindings" } # emit_godot_version_cfg


This is needed, but unfortunately forces the godot-macro crate compilation to begin only after godot-bindings has finished, possibly increasing compile times.

We should probably (in a separate PR, out of scope here) extract the version detection and #[cfg] generation to its own crate, also in light of #281.

heh, so this feature support is not without downsides...

Bromeon · 2023-10-09T15:05:21Z

godot-core/src/registry.rs

+    #[cfg(since_api = "4.2")]
    pub unsafe extern "C" fn on_notification<T: cap::GodotNotification>(
        instance: sys::GDExtensionClassInstancePtr,
        what: i32,
+        _reversed: sys::GDExtensionBool,


No need to change things now, but do you think the user could in the future benefit from having this reversed flag available inside on_notification?

it seems, that it is necessary. We use GDExtensionClassCreationInfo2 now and it uses GDExtensionClassNotification2, which has this parameter.

Bromeon · 2023-10-09T17:01:34Z

I temporarily downgraded our CI to an older Godot version to unblock open PRs.

About auto-testing of this feature. I've checked test module itest and impressed that there is such a good coverage.

The problem about this feature is that it is editor-level, but itest is game-level.

I see the following approach:

create an editor script that does the following:
1.1. runs itests (editor-compatible part of them)
1.2. replaces library with pre-built slightly modified copy (maybe modification timestamp is enough), probably with delay.
1.3. runs itests again
1.4. exits

create a scene with that tool

create github action that runs the editor, waits for termination and checks output for errors

Whether the game is worth the candle?

I agree that testing this is likely more involved. I don't think this PR should be blocked on that -- if you did some manual tests, that's already a good start.

But we could think about how to do it in the future, thanks for your suggestions in that regard. This might be nice, but likely quite a huge effort to implement, our current #[itest] infrastructure is not designed for it either. Maybe there's a way that's not too crazy?

We could also check if/how godot-cpp has any tests in this regard...

Bromeon · 2023-10-10T16:06:32Z

Some minor grammar issues in comments, but nothing crucial, so I'll merge. Thanks a lot for your efforts!

Bromeon · 2023-10-10T16:20:37Z

Oh, not again this macOS spurious failure... Really need to fix this or see if there's an issue with Godot.

kkolyan · 2023-10-11T04:42:39Z

thanks for your guidance! I'm glad to take part.

Dan2552 · 2023-10-12T23:26:01Z

godot-core/src/registry.rs

+            class_name.string_sys(),
+        );
+    }
+    godot_print!("Class {class_name} unloaded");


Sorry if a dumb question (and not sure it's really worth opening a new issue for so I thought I'd do a small comment here)

Is there a way to suppress these lines from printing?

I have an godot project that I use for integration tests for my project (launched via godot cli) and with this new change I now get a big list of nodes at the end of the output which isn't ideal for me.

Make sense. #448

Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Sep 30, 2023

Bromeon reviewed Sep 30, 2023

View reviewed changes

kkolyan force-pushed the 434_hot_reload branch from 99d6c0d to 9775bf1 Compare October 1, 2023 01:55

kkolyan marked this pull request as ready for review October 1, 2023 22:08

kkolyan force-pushed the 434_hot_reload branch from 182c7b0 to de08d73 Compare October 1, 2023 22:12

kkolyan changed the title ~~#434 Hot reload: recreate callback implemented to make this feature work~~ Hot reload support Oct 1, 2023

kkolyan force-pushed the 434_hot_reload branch from daccdd5 to 5916387 Compare October 1, 2023 22:23

kkolyan force-pushed the 434_hot_reload branch from 5916387 to 6a46484 Compare October 1, 2023 23:18

Bromeon force-pushed the 434_hot_reload branch 2 times, most recently from d5a60f4 to 876f350 Compare October 4, 2023 21:19

Bromeon reviewed Oct 4, 2023

View reviewed changes

kkolyan force-pushed the 434_hot_reload branch 3 times, most recently from 0ace29f to 298e2b3 Compare October 7, 2023 21:53

Bromeon reviewed Oct 9, 2023

View reviewed changes

Hot reload support

51500d9

kkolyan force-pushed the 434_hot_reload branch from b72380f to 51500d9 Compare October 10, 2023 10:49

Bromeon added this pull request to the merge queue Oct 10, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2023

Bromeon added this pull request to the merge queue Oct 10, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2023

Bromeon added this pull request to the merge queue Oct 10, 2023

Merged via the queue into godot-rust:master with commit 3491d7b Oct 10, 2023
14 checks passed

kkolyan deleted the 434_hot_reload branch October 11, 2023 04:40

Dan2552 reviewed Oct 12, 2023

View reviewed changes

kkolyan mentioned this pull request Oct 13, 2023

Remove excessive godot_print-ing for classes loading and unloading #448

Merged

		use std::{fmt, ptr};

		static LOADED_CLASSES: Mutex<Option<HashMap<InitLevel, Vec<ClassName>>>> = Mutex::new(None);

		let mut loaded_classes = get_loaded_classes_with_mutex();
		let loaded_classes = loaded_classes.get_or_insert_with(Default::default);


		[build-dependencies]
		godot-bindings = { path = "../godot-bindings" } # emit_godot_version_cfg

Hot reload support #437

Hot reload support #437

Conversation

kkolyan commented Sep 30, 2023 • edited Loading

kkolyan commented Sep 30, 2023

GodotRust commented Sep 30, 2023

Bromeon left a comment

Choose a reason for hiding this comment

kkolyan commented Sep 30, 2023

kkolyan commented Oct 1, 2023 • edited Loading

kkolyan commented Oct 1, 2023 • edited Loading

Bromeon commented Oct 3, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkolyan Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkolyan commented Oct 7, 2023 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Oct 9, 2023

Bromeon commented Oct 10, 2023

Bromeon commented Oct 10, 2023 • edited Loading

kkolyan commented Oct 11, 2023

Dan2552 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkolyan commented Sep 30, 2023 •

edited

Loading

kkolyan commented Oct 1, 2023 •

edited

Loading

kkolyan commented Oct 1, 2023 •

edited

Loading

kkolyan Oct 7, 2023 •

edited

Loading

kkolyan commented Oct 7, 2023 •

edited

Loading

Bromeon commented Oct 10, 2023 •

edited

Loading

Dan2552 Oct 12, 2023 •

edited

Loading