Add a saving demo that uses custom resources

[nlupugla]

This demo shows how a game can be saved using custom resources. It will be referenced in an upcoming addition to the documentation tutorial on saving games.

This demo will be used to help address issue godotengine/godot-docs#7348, which relates to the tutorial https://docs.godotengine.org/en/stable/tutorials/io/saving_games.html.

• Implement json for comparison
• Implement config for comparison
• Get feedback on demo: ensure it conforms to quality standards

[dalexeev]

1. The scripts do not follow the GDScript style guide. For example, there are extra spaces before the colon.
2. In my opinion, this demo is overcomplicated. See CONTRIBUTING.md.
3. The .tres/.res/.tscn/... approach is not safe for save files, they can include arbitrary code (see Add mention that loading tres files allows users to write any code in there gdquest-demos/godot-demos-2022#6). I believe that we should not recommend an insecure approach. Or at least add a prominent warning about it.
4. Also, this approach is less reliable in terms of backwards compatibility. If in the future you decide to change variable names or paths to scripts, this will result in an error loading errors, you will not be able to load the data as is to convert it to a new format.

Points 3 and 4 can be fixed by implementing your own resource format (not .tres/.res/.tscn/...) using ResourceFormatLoader and ResourceFormatSaver. But this is not much different from using FileAccess.

[jtnicholl]

In addition to what was said above, we already have a saving and loading demo that showcases both ConfigFile and JSON. If we were to demonstrate another method of saving and loading, I think it'd be better to add to that demo than to make another one.

[nlupugla]

Hi @dalexeev, thanks so much for taking the time to review my demo and provide helpful comments! I'll respond to each of your points. Feel free to reply to me here or on RocketChat, whichever you prefer 😄

1. I'll fix up the script formatting in my next commit. By the way, is there an official GDScript code linter that will enforce the style automatically?
2. I did my best to make the demo as simple as possible while providing context and demonstrating a few things about saving that can be tricky. a) Saving time-sensitive data: "tagging" a character in the demo stuns them for a certain time and the time left on their stun timer is saved/loaded correctly. b) Node references: each character must remember know who is "it" so the AI can decide who to run from or chase. c) Nested data structures: the main scene stores the data of each character, which are themselves responsible for storing their own data. Do you have any suggestions on how to demonstrate these concepts more simply? Alternatively, do you have any thoughts on whether some or all of these concepts are too much for the demo?
3. One of the purposes of updating the current save tutorial is that it currently doesn't warn users about the security risks of using custom resources, despite saving via custom resources being a method that is mentioned frequently within the community (eg: youtube videos). The tutorial I write to accompany the demo will try to be very clear about these risks. I do like the idea of including some warnings within the demo as well. I believe there is a proposal to make custom resources more secure in the future using a "white list" approach. If you think it's best, we could wait on this tutorial/demo until that security measure is implemented.
4. You raise a good point about backwards compatibility. Wouldn't JSON suffer from a similar issue if variable names (ie: dict keys) changed?

Points 3 and 4 can be fixed by implementing your own resource format (not .tres/.res/.tscn/...) using ResourceFormatLoader and ResourceFormatSaver. But this is not much different from using FileAccess.

This is the first time I've realized that this was an option. The class reference for ResourceFormatLoader is pretty clear, but I don't see anything in the saving games tutorial https://docs.godotengine.org/en/stable/tutorials/io/saving_games.html that would have pointed me there. Also, no one in the in RocketChat mentioned it when I asked for input on the tutorial and demo. Maybe making it easier to find and learn about ResourceFormatLoader is it's own issue.

In addition to what was said above, we already have a saving and loading demo that showcases both ConfigFile and JSON. If we were to demonstrate another method of saving and loading, I think it'd be better to add to that demo than to make another one.

Thanks for the feedback @jtnicholl :)

Funnily enough, I didn't realize we had that demo until I was drafting this PR, 😆 . Should we link to this demo in the current saving tutorial documentation? I don't mind scraping this demo and building on top of that one (creating it gave me an excuse to learn about tile maps and collisions 😄), but having looked at that saving and loading demo now, I find it perhaps a little too simple. It doesn't provide any guidance on how to approach any of the potential pain points 2.a, 2.b, and 2.c I mentioned above.

[jtnicholl]

having looked at that saving and loading demo now, I find it perhaps a little too simple. It doesn't provide any guidance on how to approach any of the potential pain points 2.a, 2.b, and 2.c I mentioned above.

I don't really the issue. It demonstrates the features it's meant to as simply as possible, that's generally what we want from these demos.

a) Saving time-sensitive data

What exactly is "time sensitive" data? If you mean data that changes every frame, there's nothing special about it, the enemy positions in the existing demo change every frame and they are saved just the same as anything else.

b) Node references

You can just save a NodePath.

c) Nested data structures

This is a bit more complicated, but not much. You can save arrays and dictionaries, including nested ones. You could also add more variables with a prefix or in a ConfigFile section. None of this is difficult to figure out once you know how the saving systems work, not to mention the demo does use arrays of dictionaries.

[nlupugla]

Thanks for the feedback :) I think overall you both have convinced me it makes more sense to build on the previous demo rather than use this one, but I do still have some thoughts on how the previous demo could be improved.

What exactly is "time sensitive" data? If you mean data that changes every frame, there's nothing special about it, the enemy positions in the existing demo change every frame and they are saved just the same as anything else.

Fair point

You can just save a NodePath

That is of course true, but I would argue that this may not be obvious for people new to Godot or game dev. The naive thing to do would be saving the node itself. When users find that doesn't work, they might not know what to do instead. That said, for the sake of keeping the demo simple, maybe the tutorial can just mention this in writing.

This is a bit more complicated, but not much. You can save arrays and dictionaries, including nested ones. You could also add more variables with a prefix or in a ConfigFile section. None of this is difficult to figure out once you know how the saving systems work, not to mention the demo does use arrays of dictionaries.

Fair point about the demo having arrays of dictionaries. One concern I have with the current demo's code is that the saving scripts are tightly coupled to the details of the player and enemies. I think a good demo should help communicate best practices, I'd like to modify the saving code to follow a pattern closer to the one I used in my code. The current demo saves everything in one monolithic script:

var save_dict = {
player = {
	position = var_to_str(player.position),
	health = var_to_str(player.health),
	rotation = var_to_str(player.sprite.rotation)
},
enemies = []
}

for enemy in get_tree().get_nodes_in_group(&"enemy"):
save_dict.enemies.push_back({
	position = var_to_str(enemy.position),
})

I think we could set a better example be writing it more like

var save_dict = {
player = player.to_dict(),
enemies = []
}

for enemy in get_tree().get_nodes_in_group(&"enemy"):
save_dict.enemies.push_back(enemy.to_dict())

With appropriate to_dict() methods implemented in Player and Enemy.

In summary, I'm happy to stick with the previous demo with a few modifications. In the written tutorial, I'd like to link to the demo and include a sentence or two about saving node references using node paths. What do you think about this approach?

[jtnicholl]

I think a good demo should help communicate best practices, I'd like to modify the saving code to follow a pattern closer to the one I used in my code. The current demo saves everything in one monolithic script

I'd agree if this were part of a university course or something, but it's not. Anyone using this already knows how to program, and hopefully knows good practices too.
We do want good code in the demos, but it's not our job to teach good coding practices. Doing everything "properly" is often overkill for really simple projects.
In university I remember reading a book where the author takes Hunt the Wumpus and rewrites it to use proper architecture layers, separations of concerns, etc. Then at the end said something along the lines of "This was just a learning exercise, the game was less than 200 lines in one file before, it didn't need all this."
Similarly, I'd rather not overcomplicate things here and risk distracting from the topic, which is just saving and loading.

[nlupugla]

In university I remember reading a book where the author takes Hunt the Wumpus and rewrites it to use proper architecture layers, separations of concerns, etc. Then at the end said something along the lines of "This was just a learning exercise, the game was only about 200 lines in one file before, it didn't need all this."

Lol :)

I definitely agree that one can go overboard when it comes to showing "best practices", but changing to player.to_dict() only adds one layer of abstraction and steers the reader towards a workflow that's much easier to maintain. I'd also argue it makes it easier to understand what is going because it reduces the lines of code (and cognitive load) between the crucial lines var file := FileAccess.open(SAVE_PATH, FileAccess.WRITE) and file.store_line(JSON.stringify(save_dict)).

Thoughts?

[nlupugla]

Hi folks! Thanks again for your feedback on my demo. I've paused working on this in favor of working on a high-level saving API inspired by the high-level multiplayer API. @dalexeev @jtnicholl I'd love to hear if you have any feedback on the API draft so far: godotengine/godot-proposals#7041.