godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Prevent data loss on reloaded instanced scenes (which use tool scripts) #1012

Open Error7Studios opened 4 years ago

Error7Studios commented 4 years ago

Describe the project you are working on: A combat system which makes heavy use of buttons on the screen.

Describe the problem or limitation you are having in your project: I have a custom "BTN" scene with a tool script. (The BTN node is actually a generic GUI node that has a child Label for text windows and other UI, but can also be clicked.) I'm instancing a bunch of these BTN nodes to create my game's GUI. It uses an exported setget function to set the text of the child Label in the editor to preview how the scene will look.

The problem is that, because it uses a tool script, whenever I make any change to the script and save it, all the instanced scenes are reloaded and their setget setter functions get called again, causing the button text to be reset. The sizes of the buttons also get reset, so the entire GUI layout gets screwed up.

Describe the feature / enhancement and how it helps to overcome the problem or limitation: Automatically re-apply all property settings changes on instanced scenes whenever they are reloaded.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: I found a workaround to this problem by doing the following:

  1. Set the Main scene (which contains the instanced BTNs) as a tool script.
  2. In the Main.gd _ready() function, grab its .tscn file using a Directory search. (see Notes section)
  3. Parse the text of the Main.tscn file and extract all of the saved properties of each instanced BTN.
  4. Find and grab each BTN in the Main scene using get_node() and the node paths in the Main.tscn file.
  5. Set each of those BTNs' properties again using the data found in the .tscn file.
  6. Call setter functions for each property, if they have one. (see Notes section)

Things that didn't work:

If this enhancement will not be used often, can it be worked around with a few lines of script?: This would benefit anyone that uses tool nodes to lay out a scene.

Is there a reason why this should be core and not an add-on in the asset library?: Users would expect stability of their node data without needing an add-on. It should "just work", and you should be able to quickly set up a scene in the editor for rapid prototyping/testing. We shouldn't have to worry about our scene setup being reset for inexplicable reasons and losing hours (or more) of work.

Also, this requires a lot of complex code to do manually. The fact that nodes can lose their data because they use tool scripts is dangerous and can be a huge setback. This had me pulling my hair out until I implemented my workaround (which is far from perfect.)

Notes: Step 2 (grabbing a scene's .tscn file) was not as simple as I had hoped. A function like Node.get_tscn() would have been helpful. (This could be a separate proposal) My method currently relies on the .tscn file having the same name as the root node, and there not being another .tscn file of the same name.

Step 6 currently relies on the setter function being named "set_" + , but obviously not everyone names their setters that way. If there is a built-in way to check if a property has a setter, so that it can be called automatically instead of directly setting the value, that would be useful here. (Vnen's proposal #844 might take care of step 6 automatically, but I'm not sure.)

I can provide an example project with this code if it will help, but it might be too specific to my project, and there's probably a much better way to do this than my workaround.

Zylann commented 4 years ago

This is a wide problem with tool scripts that I encounter every day in plugin development, both on nodes, resources or simple reference scripts. For some reason, when a tool script reloads because it was modified, ALL its member variables get reset to defaults (unless maybe exported ones, but these are quite rare in my case). So errors occur, objects leak and so on. So for now, if I modify a plugin, I constantly have to turn it off first, close scenes with tool scripts in them, do my change, and turn it back on. Sometimes I even restart the whole editor to be sure, because instances can still live in the inspector and undo histories, yay.

There is no way hot-reloading scripts can work flawlessly. It will break to some extent, but there are obvious ways to limit that.

What I wish would happen is for the script instance's member variables to be preserved temporarily, and re-applied after reload, without having to intrusively modify the script to take care of that somehow. The point being, "code" is stateless. It can be reloaded with no problem. Data on the other hand, is what matters, and data is found in member variables. So if they are backed up and set again, none of the references and values they were holding will be lost, which would make logic modification possible with a lot less issues. This goes beyond the STORAGE hint of exported variables (it should not rely on that) because onready variables and other members referring to nodes and other states also get reset in that process. Also, implementation is language-specific, as member vars may be stored differently.

Another approach is to reload the scene, but if we do that we loose the point of reloading the script, better just restart the whole system (scene, plugin, editor...), which can be partly automated with some plugins. Also states will remain reset, and still won't work if not a node. Invoking _ready again was mentionned too, however that only applies to nodes. Besides, these functions are often used to initialize and setup stuff, which goes against the idea of reloading the script in mid-air. It doesn't even have information about a "previous-life-state" to be able to restore things. It makes the function multi-purposed and no longer uniquely called, which is harder to use correctly because we'd have to keep that in mind when programming it and use workarounds. See also https://github.com/godotengine/godot/issues/16974 People think the problem is about _ready or _enter_tree but it's not, it's wider than that, and comes down to member vars.

Error7Studios commented 4 years ago

What I wish would happen is for the script instance's member variables to be preserved temporarily, and re-applied after reload, without having to intrusively modify the script to take care of that somehow.

It seems like for every script you create, there needs to be a linked backup script created automatically by the engine that's hidden from the user.

Whenever you save a scene or make a change to a script, the backup scripts should get updated as well.

Then, whenever the scene gets reloaded, the engine could check each reloaded script, and compare it to the backup.

Any properties in the original scripts that are different from the ones in the backup since the last save should get set to their value in the backup, and call any relevant setter functions.

I have no idea how much work that would entail, but I don't know how else this could be handled.

What I did was put the .tscn parser code into a function in a singleton, and just called that function in the beginning of Main's _ready() func, but that's just a workaround, and only works for my particular use-case (for now).

Zylann commented 4 years ago

Yeah that's the idea. It doesnt have to be a full-fledged "backup script", but may contain the "state" of members so they can be restored or discarded when the reload happens (could be a temporary Map<StringName, Variant>). You basically did that manually, though only on exported properties.

KoBeWi commented 1 year ago

I forgot about this, but in 4.0 scripts no longer lose data when saved. Can you check if it resolves the proposal?

DissonantVoid commented 1 year ago

what about godot 3.x? is it possible for whatever fixed this be back-ported?