godot-rust / gdext

Rust bindings for Godot 4
https://mastodon.gamedev.place/@GodotRust
Mozilla Public License 2.0
3.13k stars 198 forks source link

Hot reloaded nodes with "tool" enabled lose state and don't run initialization functions #908

Open Soremwar opened 1 month ago

Soremwar commented 1 month ago

Given the class

#[derive(GodotClass)]
#[class(base=CharacterBody2D, init, tool)]
pub struct Player {
    base: Base<CharacterBody2D>,
    #[init(val = 1)]
    x: i8,
}
#[godot_api]
impl ICharacterBody2D for Player {
    // Or enter_tree, it don't make a different
    fn ready(&mut self) {
        self.x = 0;
    }

    fn process(&mut self, _: f64) {
        godot_print!("{}", self.x);
    }
}

One would expect the console to spit out 0 all the time right? Well that would only be true when the scene is first opened, because if any change triggers hot reload x will be reset to 1 as if ready was never run.

This might not seem like a big deal, until you test this with something like

#[derive(GodotClass)]
#[class(base=CharacterBody2D, init, tool)]
pub struct Player {
    base: Base<CharacterBody2D>,
    #[init(val = None)]
    camera: Option<Gd<Camera2D>>,
}

This also applies for properties initialized with OnReady, so hot reload + tool is pretty much a no go

TitanNano commented 1 month ago

On hot reload, the engine does:

  1. create a backup of all class properties (you have to add #[var])
  2. destroys the old instance
  3. creates a new instance and runs fn init() on it
  4. restores the property backup by calling the property setters with the stored values
Bromeon commented 1 month ago

@TitanNano what's the takeaway? You don't mention #[class(tool)] (opposite of "runtime classes"), so are any differences expected in that regard?

TitanNano commented 1 month ago

@Bromeon looking at the implementation, @Soremwar is currently not defining his Godot class in a way that can be hot reloaded the way he expects. Data that should be persisted between reloads has to be exposed to the engine. But from user's perspective, I get that this is not obvious. Maybe that is just a documentation deficiency?

Tool classes are the only ones affected by this, as runtime classes do not have instances which need reloading. Hot reloading is also only supported in the editor, as far as I'm aware.

Soremwar commented 1 month ago

@TitanNano So in this example exposing my variables with #[var] should work as expected?

TitanNano commented 1 month ago

@Soremwar I just checked again, and you have to add #[var(usage_flags = [STORAGE])] to every field that you want to get restored.

Soremwar commented 1 month ago

If I'm reading the docs right, this would modify the scene file wouldn't it?

TitanNano commented 1 month ago

@Soremwar yes I think so too. It feels like a bug in the engine. It is unclear to me why this was chosen, but properties must have the storage usage flag so they get backed-up before hot reloading.

Soremwar commented 1 month ago

TBH it makes complete sense for a regular use case, since no Node would be expected to change their initial state after they have been instantiated. This is only true for @tool nodes, which makes me think that a way around this problem should be implemented on the side of the library.

I'm thinking of a hook (similar to _ready) that runs when the node is hot-reloaded, where it's left up to the user how to do state management between reloads. This would aleviate the problem of OnReady node references and similar approaches while preventing side effects on regular nodes

I'm up to implement this if you guys think it's a good idea

Yarwin commented 1 month ago

I'm thinking of a hook (similar to _ready) that runs when the node is hot-reloaded, where it's left up to the user how to do state management between reloads.

IMO this kind of hook that doesn't provide some kind of automatic re-evaulation of initial/onready values (a bit similar to init) would be pointless, since it already exists – Godot sends ObjectNotification::EXTENSION_RELOADED after reload which can be easily accessed on the user side. This way has been suggested in PR.

Albeit providing some kind of "on_recreate" function that behaves similar to "init" (read - automagically recreates all the values) could be very cool