godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Prevent loss of user-entered data in the Godot Editor #10694

Open marcinn opened 1 month ago

marcinn commented 1 month ago

Describe the project you are working on

Godot can lose user data very easily. Just create a base class (a custom node) with some properties, use the node in your scene file, then change a property name in the base class. Godot Editor will not read all values and with next save will remove stored data.

EDIT:

In https://github.com/godotengine/godot/pull/60597 Juan tried to solve some of related use cases. His implementation is not complete, because:

I would like to discuss extending this feature.

  1. Implement pre-save validation of the scene, which will detect missing resources and types, then covert these nodes to store missing data in the file.
  2. Add handling for missing properties only - when type or resource of a node remains valid.

Describe the problem or limitation you are having in your project

Our level designers are loosing important data without any warning, when interface of our custom nodes is changing. And we have no possibility to write conversion tools.

Example. We are using stay_open property in our custom node, and our editors have bunch of files with this property filled.

godot-lost-props

We're updating our plugin, where we decided to change the name of a property stay_open to open/wait_time. When our editors will update the plugin, they will lose data at the first save of the scene.

godot-lost-props-2

The problem is more important when using gdscipt. Our level editors can write some scripts to extend the functionality. Changing the property name (@export var) is very easy, and there are no warnings. So they will lose their data more often.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Just do not delete unused data from (t)scn files.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

  1. Do not delete data already stored in (t)scn files
  2. Show in the inspector a list / dictionary of unhandled properties. Let user decide what to do - remove, move or copy.
  3. Allow to access to unhandled properties from the code. Plugin authors will be able to implement a conversion tool.

If this enhancement will not be used often, can it be worked around with a few lines of script?

This is related to the core of the Godot Editor.

Is there a reason why this should be core and not an add-on in the asset library?

It applies to every single project. Nobody wants to lose data.

AThousandShips commented 1 month ago

See also:

And this comment:

marcinn commented 1 month ago

Thanks. Note that data loss issue shouldn't be solved by explicit marking the data to be preserved. No soft should delete data unless the user consciously performs a kind of "delete" action.

AThousandShips commented 1 month ago

This would create a lot of clutter and awkward workflows IMO, and would involve rewriting how the entire resource system works, which is a large undertaking, and would involve further changes to how the compatibility systems work with renamed properties, having a passive system that just stores invalid properties from files somewhere and then pops them back into the file when saving it would be inefficient and a IMO a very naive approach to doing compatibility and resource management

marcinn commented 1 month ago

would involve rewriting how the entire resource system works

Hm, I think not. It is about not removing data from files holding the data.

[node name="TrainSecuritySystem" type="TrainSecuritySystem" parent="."]
aware_system/active = true   <---- 
aware_delay = 60.0   <----
AThousandShips commented 1 month ago

That's not how files work though, all files are rewritten entirely, it doesn't seek through and replace only parts of the files, you still need to store the data somewhere

marcinn commented 1 month ago

So Godot is not able to store the data. In that case inspector in the editor is completely unusable (because it is unreliable) and should be removed.


To fix the issue:

  1. read all properties from file, store unknown props in a separate Dictionary in the Node instance
  2. while saving just dump this Dictionary to the file

To improve UI/UX:

  1. show unhandled values in the inspector
marcinn commented 1 month ago

Now I've lost whole scene. My custom plugin compiled successfully, but there was one issue - a missing symbol (missing method impl). I hit F5, so Godot Editor saved the scene. And my data now is gone. Just one F5 hit...

Godot Editor is not reliable and should not work that way.

AThousandShips commented 1 month ago

The main safeguard against that is to use version control, no software is 100% perfect and can't guarantee loss of data, having multiple layers of protection is always important

marcinn commented 1 month ago

Version control is not a solution. It could only prevent less or more of the data loss.

The tool should not force users to use other solutions just because of it's bug or design flaw. Imagine a hammer. It is designed to hit nails, but not your face. And if you use it properly to hit nails, it would never hit your face. And you aren't forced to wear knight's armor.

Imagine git with a working copy (not committed). User hits F5 in Godot, and he loses the data from this working copy. He was using vsc? Yes. The only one "solution" is to integrate Git with Godot to commit every change before every scene run (!), just to workaround a big issue of the Godot Editor.


Life hack: as a software developer never delete user data. Even when he ask you to do that (that's why trashcan was introduced in file managers).

AThousandShips commented 1 month ago

If you're doing major refactoring you should be prepared with VCS, that's what I always do, that's what VCS is for, you don't even need to commit it just staging it will protect you

Also with the hammer analogy, if I were to work with a lot of tools I'd wear eye protection because you never know, so it's actually a pretty good analogy: don't trust the tool you're using to be perfect if you care about the data, there's always the risk of unrecoverable errors in any software so add an additional layer of protection

But sudden data loss in general aside (pretty off topic for this) it's a good tool to ensure you don't lose data when refactoring

marcinn commented 1 month ago

You're trying to convince me that bad design (losing the data) is same as an error/accident. That's two different things. It is not the first time when I'm losing the data with Godot Editor. The issue is not related only to major refactoring.

Why not assume that Godot is doing this wrong and just fix it? Everyone will be happy and thankful.

AThousandShips commented 1 month ago

Well this isn't about that though, if you have a bug in the editor please report it here, data loss like that wouldn't necessarily be fixed by your proposed feature for refactoring your code, so please let's stay on topic for properties removed, as if the editor deleted properties from scene files when the properties are there still that's a bug

marcinn commented 1 month ago

If you're doing major refactoring

Imagine a case, where Godot can't load the dll/so plugin by some operating system error/lock/etc. Godot will open the project, and after hitting F5 it will will save the scene and remove all the data related to unavailable custom nodes. And it will change types of the nodes.

There is no need to do anything with the code :)

marcinn commented 1 month ago

proposed feature for refactoring your code

Sorry, but you're talking about refactoring a code.. I'm talking about losing the data by Godot Editor. Please stay on the topic :)

AThousandShips commented 1 month ago

That's still a bug though, it shouldn't be saving the scene if the scene can't be loaded correctly, if the editor bugs somehow there's no guarantee it will write what ever data it writes correctly even with a system like your propose, so the solution is to prevent the error state to begin with and not save under those conditions, it won't work in general anyway

And please behave professionally, no need for that, I'm trying to help here and you're just mocking me

marcinn commented 1 month ago

Thank you for your help.

Please note that I'm also spending my time trying to improve Godot. Don't get me wrong, but I'm not asking for help here. I want to discuss the issue and get ack for solving it. I'm using vsc, etc. I know that my disk could be dead in a hour, etc.


Let's talk how to prevent losing the data by editor design. My idea is to store the data untouched, to avoid silent data removal. It is better to have a bunch of unused variables instead of nothing.

Files filled with the data coming from an user input (properties in Godot Editor) should not be cleaned automatically. Never. But Godot Editor is currently removing data in some cases. It could be 1, 10, 100 or X cases - we will never handle all of them, because we don't know many of them.

But doing the opposite, there are finite cases where data must/should be deleted. It is better to deal with those few cases rather than the many unknown ones that involve the risk of data loss.

This is a design/conceptual change. I know it could require many changes in the core (ie. handling "broken" nodes of an non-existing custom classes without switching node's type to a pure Node class), but it is doable.

Thank you, have a good day/night.

AThousandShips commented 1 month ago

Please note that I'm also spending my time trying to improve Godot. Don't get me wrong, but I'm not asking for help here. I want to discuss the issue and get ack for solving it. I'm using vsc, etc. I know that my disk could be dead in a hour, etc.

That's what I was discussing, if you feel my suggestions and feedback on the issues combining what you suggest with the way the engine works isn't something you consider helpful or constructive then I'm sorry I can't provide any other feedback. But having a perspective on how the engine works and why is fundamental to any new ideas or improvements.

marcinn commented 1 month ago

Sorry to sound harshly, I'm not native english speaker. I really appreciate your help and you're right about making copies and using vcs. But I am aware that the source of the problem lies elsewhere and I would like to solve it once and for all.

AThousandShips commented 1 month ago

So if we talk about "if I rename my_property to my_new_property the data is lost" then that's a problem of refactoring, which is a complex issue with many possible solutions

For loss of data from other sources the solution should be to identify what causes them and prevent writing to the file in those cases entirely, as it's unlikely that the files won't be bugged by other things at the same time

I think it's important to separate these two concepts, having the files just store any property that isn't found is complicated and has many complex issues to it, and the situation where the script doesn't load won't be fixed by this either, unless the file was loaded before the script somehow changed or broke, if the script or extension doesn't load it shouldn't load at all, say "something went wrong" and not open the scene or allow you to save it, but if the scene was loaded properly those properties won't be in any list of "unknown properties" in the first place

marcinn commented 1 month ago

The concept of "unknown properties" is to preserve data loss from any case. Other things like node types requires some additional handling.

Note that renaming props is just an example of a just one case. Later I mentioned other cases - there were no refactoring, but a loss of many data (hundreds of properties and connected signals). The source of the both cases is the same.

By having all the data stored you can handle common cases in a proper way, then after that you will able to remove properties from the "unused" set. In most cases unused set will be probably empty, but for other cases it will hold all the data, and user will able to decide what to do (he could also edit tscn files directly).

Please note that when talking about "unused set" I'm thinking about one Dictionary _unused; declared per node instance (we can consider some lazy initialization with pointers). In tscn nothing will change - just store the properties in a file as usual.

Let's assume a is handled and b is unhandled. The first is stored as usual, but the latter comes from _unused dictionary of variants. TSCN files should store both:

[node name="CustomNode" type="CustomNode" parent="."]
a = 1
b = 2

Nothing more, nothing less.

Here is an example project: data-loss.zip

The data_loss_demo.tscn contains a node declaration with one unhandled property b. Please open the project, open data_loss_demo.tscn, save it (ctrl+s), then open the data_loss_demo.tscn in a text editor. You will notice that there is no b anymore. This can be caused by renaming, changing the interface of base classes, bugs in base classes, failing to load gdextension plugins, and hundres other cases.

Please note that Godot Editor can also destroy node types in such cases.


Please also note that Godot Editor cannot always detect a problem with tscn file. The file can be properly loaded - please check the example I've attached, where the editor silently removes data during typical load-save workflow without any warning.

marcinn commented 1 month ago

I've found something similar - handling of some missing resources:

image

It looks like Godot is already storing some missing properties in a metadata of a node.

It looks like they should be stored in the scene files: image

Hm..... made by Juan. Maybe this system isn't always working? image

https://github.com/godotengine/godot/commit/0a57f964a357976e023b638e872397ba94123776 https://github.com/godotengine/godot/pull/60597


Juan wrote about two use cases about node and resource types, but nothing about missing properties. I think his concept simply needs to be expanded. Also could have some bugs. It's definitely worth checking out.

marcinn commented 1 month ago

If a Resource type is missing upon load and If a Node type is missing upon load. Upon load. My today was about automatic reloading a broken plugin - scene was already loaded into editor, and saving it destroyed all data related to custom nodes. Unfortunately his solution still isn't working for properties.