godotengine / godot-proposals

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

Create high-level saving/loading API similar to high-level multiplayer API #7041

Open nlupugla opened 1 year ago

nlupugla commented 1 year ago

Describe the project you are working on

Godot core.

Describe the problem or limitation you are having in your project

Godot does not do much in the way of helping users save and load their games. On the other hand, it has a great high-level multiplayer API by way of SceneMultiplayer, MultiplayerSpawner, and MultiplayerSynchronizer, which make it possible to set up multiplayer through the editor. Multiplayer and saving/loading are similar tasks in that they both involve serializing/deserializing game data.

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

I propose creating a high-level save/load API similar to the multiplayer API to make it easier for users to save and load their games.

The high-level save/load system would consist of three main components:

  1. A global SceneSaver (analogous to SceneMultiplayer)
  2. A SaveSpawner node (analogous to MultiplayerSpawner)
  3. A SaveSynchronizer node (analogous to MultiplayerSynchronizer)

In any scene with nodes whose properties need to be saved/loaded, the user places a SaveSynchronizer. They configure the node to point to those properties. In any scene that will instantiate other packed scenes at runtime, the user places a SaveSpawner and configures it to point to those packed scenes. Finally, SceneSaver will expose global methods like save and load that can be called from anywhere.

One concept that appears in this API that doesn't in the MultiplayerAPI is the idea of layers. The idea is that you can configure a save_layer bit flag (much like the visibility_layer property of a CanvasItem) so that you can choose to save/load certain portions of your game.

I have tried to make the API align as much as possible with the analogous Multiplayer API. If you have questions about an item in the API below, refer to the Multiplayer API documentation for more information, or feel free to ask for clarification. This is a very rough draft, so comments, criticisms, and suggestions are very much welcome!

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

Edit: 14:30, 2023/06/12: Added some screen shots of how configuring the Multiplayer nodes works to give people a sense of the UI and make this proposal a bit more self-contained

Edit: 14:04, 2023/09/21: Added a link to my PR.

See https://github.com/godotengine/godot/pull/79644.

Here is the UI that selecting properties of a node to save would be based on:

image

Here is the UI that configuring a spawner would be based on:

image

My original discussion includes a mock-up of a scene tree for a simple game: https://github.com/godotengine/godot-proposals/discussions/7005, so here, I will focus solely on fleshing out an API for SceneSaver, SaveSpawner, and SaveSynchronizer.

First off, we have an abstract SavingAPI class, analogous to MultiplayerAPI. The default implementation will be SceneSaver, but having an abstract class leaves room for custom implementations.

Edit 22:10, 2023/06/07: added encode and decode methods. Edit 8:50, 2023/06/09: changed signatures of save, load, encode, and decode to include extra optional parameters object: Object and section: StringName. The object, section, and layer parameters should give developers a lot of control over what subsets of their game they want to save/load. In particular, this could help with creating save/load previews by letting the developer save metadata like a compressed image and date/time in a section called "metadata".

class_name SavingAPI
extends RefCounted

static func create_default_interface() -> SavingAPI
static func get_default_interface() -> StringName
func object_configuration_add(object: Object, configuration: Variant) -> Error
func object_configuration_remove(object: Object, configuration: Variant) -> Error
static func set_default_interface(interface_name: StringName) -> void
func save(path: String, object: Object = null, section: StringName = &"", layer_flags: int = 1) -> Error
func load(path: String, object: Object = null, section: StringName = &"", layer_flags: int = 1) -> Error
func encode(object: Object = null, section: StringName = &"", layer_flags: int = 1) -> PackedByteArray
func decode(data: PackedByteArray, object: Object = null, section: StringName = &"", layer_flags: int = 1) -> Error

Next we have the default implementation, SceneSaver.

class_name SceneSaver
extends SavingAPI

var allow_object_decoding: bool = false
var root_path: NodePath = ^""

# The implementations of object_configuration_add/remove use the configuration argument to pass the NodePath of a Synchronizer or Spawner.

# perhaps also methods that return a dictionary, resource, string, and/or byte representation of the save

Next up is SaveSynchronizer.

class_name SaveSynchronizer
extends Node

signal synchronized

var replication_config: SceneReplicationConfig
var root_path: NodePath = ^""
var save_layer: int = 1

func get_save_layer_bit(layer: int) -> bool
func set_save_layer_bit(layer: int, enabled: bool) -> void

Finally, we have SaveSpawner.

class_name SaveSpawner
extends Node

signal despawned(node: Node)
signal spawned(node: Node)

var spawn_function : Callable
var spawn_limit : int = 0
var spawn_path : NodePath = ^""

func add_spawnable_scene(path: String) -> void
func clear_spawnable_scenes() -> void
func get_spawnable_scene(index: int) -> String
func get_spawnable_scene_count() -> int
func spawn(data: Variant = null) -> Node

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

Saving and loading by script requires writing a lot of boilerplate code. The bigger your game, the more boilerplate you will have to write for saving and loading.

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

Ever since The Legend of Zelda, saving and loading has been core to creating games. Beginners should be able to set up a save system from the get-go, not have to comb through the asset library.

smix8 commented 1 year ago

I think a save/load system should focus on the generic serializing/deserializing and file preview and file management part.

This includes for me:

Those are all generic parts of a savegame system that all projects can make use of no matter their gameplay focus or scene buildup. Those are also the parts that basically all beginners struggle a lot with.

A save/load system should NOT focus on the gameplay / scene part because that is very game specific.

I can already tell that the API in this proposal will have the same flaws as the Multiplayer API, which is that as soon as you have a more complex project that is not fully node based it will fall apart. I really tried to wrap my head around the proposed API here and what parts I could replace from my custom saveload system, the answer is none of it. It does not cover anything useful for me as the projects I work on all have complex, dynamic scenes with only a small part of it actually based on nodes that are in the SceneTree so all this node bases approach does not work for such projects.

SlugFiller commented 1 year ago

Quicksave vs manual save is a UI issue. In the same way that InputMap doesn't provide a key configuration menu, a save API can't provide a save selection menu. While most games will have a structurally similar one, they still need a design and layout that matches the game's look and feel. This can also influence design decisions like the number of save slots. Many horror games and RPGs limit to 3 save slots, but that's because of the way they are laid out on the screen (large detailed slots), not due to any technical limitations.

For the same reason, autosave rotation is also something that can't and shouldn't be automated. You don't know if the game has unlimited save slots, 3 save slots, or even a single save-slot with forced autosave. Also, do you save at regular intervals, or at set checkpoints or triggered game events? These are all considerations that vary widely from game to game.

Meanwhile, a save-state may be configuration-wise game specific. But as emulators have demonstrated, there's nothing actually game specific about it. It's, in actuality, more generic than a physics engine. It's all about being able to bring a game to a state that it was previously in. All games, and indeed, all software, goes through states. The configuration is simply about how to minimally represent diffs in that state without doing full memory snapshots.

I would be interested in knowing what's "complex, dynamic scenes with only a small part of it actually based on nodes". Because that sounds like you couldn't use any of Godot's other features, like physics, graphics, navigation, audio, etc...

AThousandShips commented 1 year ago

I would be interested in knowing what's "complex, dynamic scenes with only a small part of it actually based on nodes". Because that sounds like you couldn't use any of Godot's other features, like physics, graphics, navigation, audio, etc...

That isn't really true, a lot of data could be placed outside the tree without depending on those things, like:

And quite importantly:

This data wouldn't be associated with any specific node, and would be best to not do so to preserve it across changing scenes etc.

Also for data not currently used you'd benefit from being able to offload it until you actually save, for example if you run through four different large levels of the game with dozens of enemies with dozens of data points to store each

SlugFiller commented 1 year ago

Inventory Journal Quests

Yes, these can be implemented with just a large array. But shouldn't be, as that would make adding new entries/items that much more difficult (Ever growing monolithic structure). Also, if you have inventory tetris, than making inventory items as nodes makes all the more sense.

Data from parts of the game not currently loaded

If it's unmodified from its initial state (area not entered in open-world-ish game), then there's nothing to save. If it is modified and unloaded, then you need to save the state of it prior to unloading, so this proposal actually helps - encode the contents of the section before unloading it, and keep the PackedByteArray. Then, on save, simply write all the kept PackedByteArrays.

Currently, unloading a section without losing the data inside it has no built-in mechanism. Given that any implementation of such a system would currently be roll-your-own, I can't give a comment as to how you could add saving to such an implementation with arbitrary complexity. I can only point out how to replace it with one that is not roll-your-own.

The closest real-world example I can come up with is Quake2, which retains the state of levels in the same section as you go back and forth between levels. (Hexen also had a similar system prior). It works by saving the game on level exit, and then loading it back, modifying only the spawn point, on re-entry. So, if anything, this is an excellent use case for a uniform save game system.

The other example I can think of is Soul Reaver. But it does significantly less state preservation. The same considerations as Quake2 apply, but with the added consideration that multiple "levels" (world sections) can be loaded at the same time. This is where save layers come in, although having a limited number of layers can be detrimental here.

AThousandShips commented 1 year ago

Also, if you have inventory tetris, than making inventory items as nodes makes all the more sense.

Not really, this breaks the principle of separating representation and data, the visual representation and interface isn't the same as the data, and having to use a node to access inventory data is cumbersome, having the inventory UI be the inventory is a very bad idea...

A trivial and obvious example to show how this is bad would be using a progress bar to represent health and literally using the value property of the progress bar to store the health, instead of making the progress bar reflect the health value

If it's unmodified from its initial state (area not entered in open-world-ish game), then there's nothing to save.

Obviously, but this proposal doesn't mention anything about default data and handling it

If it is modified and unloaded, then you need to save the state of it prior to unloading, so this proposal actually helps - encode the contents of the section before unloading it, and keep the PackedByteArray.

Yet as I said there's no mention of any of these situations in the proposal

So, if anything, this is an excellent use case for a uniform save game system.

Not as it is proposed here, as it lacks any flexibility, there's not a single mention or suggestion how to do any custom behaviour here, sure you point to the MP API, but you need to actually elaborate on this here because it's a pretty fundamental aspect no?

SlugFiller commented 1 year ago

Obviously, but this proposal doesn't mention anything about default data and handling it

You don't need to "handle" default data. An unmodified section/level is basically like doing a "new game". If you don't have a saved buffer, you just load the scene as normal.

Yet as I said there's no mention of any of these situations in the proposal

Yes there is. Notice the methods SavingAPI.encode and SavingAPI.decode. Of course, you don't have to do it this way. Quake2 actually saved level state to a file. A "save game" in Quake2 actually consists of multiple files - one for each level. But this proposal does offer a way to do it with a single file using SavingAPI.encode.

Not as it is proposed here, as it lacks any flexibility

There is every bit of flexibility needed, as I demonstrated above. At least for the two examples I could think of, Quake2 and Soul Reaver, this proposal offers everything that is needed, without having to add more than a single-digit of lines of code.

If you have a more complex example/use-case in mind, I'm listening.

AThousandShips commented 1 year ago

You don't need to "handle" default data.

Yes you do, that isn't trivial, and that very fact is important, MP doesn't care about this as it deals with synchronization (if you won't provide reasons for "you don't need to" then why do I?)

Yes there is. Notice the methods SavingAPI.encode and SavingAPI.decode

That isn't the same as addressing the various use cases that aren't just "it's in the current scene"

This proposal doesn't make any mention of changing scenes, retaining states, etc., the fact that the tools for handling that are nominally there isn't the same

From this proposal and the details given, and without doing my own extrapolation and guesswork (which I shouldn't have to do for a proposal, it's supposed to cover things, extensively, otherwise this should have stayed as a discussion), there's only really room here for trivial and standard setups, with all relevant data strictly on nodes, with no systems for custom loading, custom callbacks, etc.

This system also ties the saving and loading not only to the tree and nodes specifically, but also to a specific tree configuration, which isn't helpful or convenient, it deals with nodes and node paths, an inefficient setup, it doesn't handle changes to the tree layout.

I don't see how this offers anything, for any but the most basic user as a beginner setup, over serialisation with groups other than a mild convenience, at the cost of these issues

Using manual serialisation is far more flexible, and can be set up by even relatively inexperienced users, and is more reliable, doesn't add a lot of extra clutter to a scene, and arguably really important:

Doesn't depend on the implementation of the system here, only on the low level serialisation system

And additionally, the main justifications and benefits, as I see it, of the MP system is: 1) It benefits and centers the tree system and layout, as it deals with nodes that should be synchronized or spawned, and having a specific tree layout matters 2) It is highly performance sensitive, and especially working from GDScript it can be hard using the low level API to do things like synchronizing a large number of entities in an efficient and performant way 3) The bar for using the low level API with multiplayer is high, new users can't be expected at all to be able to deal with this, and even reasonably experienced users will have a hard time making things that work well and reliably

These three points really don't apply here, and imho even work against this kind of setup, unless it is much more flexible and easy to use out of the box than just making serialization with a few lines of code

nlupugla commented 1 year ago

Hi folks, thanks for your interest and providing such fast feedback :) I'll respond to people one by one as best I can!

I think a save/load system should focus on the generic serializing/deserializing and file preview and file management part.

This includes for me:

  • Autosave, Quicksave and ManualSave distinction.
  • Autosave rotation
  • Embedded save screenshot
  • Savegame metadata
  • Quick reads and previews to not lag any SaveLoad Menu.

I like a lot of these ideas! However, I want to start by focusing on what Godot currently provides the least help with. In an effort to avoid the dreaded scope creep™️ 💀 , I think it's best to limit this first proposal to laying out a strong foundation for further ideas that advanced features can build on.

With that in mind, I definitely agree that the focus should be on generic serializing/deserializing. The proposed abstract SavingAPI supplies encode and decode methods for generically serializing/deserializing layers of your game to/from PackedByteArray. Furthermore, I suggested that the default implementation, SceneSaver could provide helper methods for converting these bytes into other formats like JSON, a dictionary, or a Resource. What other functionality would you like to see to improve the experience of generic serializing/deserializing?

As for file preview and file management, this is one of the areas for which Godot already provides some nice tools. For example, you have the FileDialog node for very basic file management and the whole Control node UI system for building up a great saving UI/UX customized to fit the look and feel of your game.

The other things you mentioned can likewise be built on top of a strong serializing/deserializing foundation with the help of Godot's existing nodes. I think there is a nice opportunity for some of these features to be unified into one or more addons in the asset library.

A save/load system should NOT focus on the gameplay / scene part cause that is very game specific.

I disagree with you on this point. Indeed, the structure of your save/load data is highly game specific, so one way or another, the developer has to tell the game engine about their game's structure. Without the proposed API, this must be done using potentially hundred of lines of boilerplate like save_struct.player_hp = player_hp. With the proposed API, the developer gets to tell the engine about their game's structure through configuration data via an editor interface to the SaveSpawner and SaveSynchronizer nodes. As the saying goes, data > code.

I can already tell that the API in this proposal will have the same flaws as the Multiplayer API, which is that as soon as you have a more complex project that is not fully node based it will fall apart.

I was under the impression that the Multiplayer API was generally well-liked. Perhaps I'm mistaken though. It would be interesting to see a community poll on what people like/dislike about the Multiplayer API.

I really tried to wrap my head around the proposed API here and what parts I could replace from my custom saveload system, the answer is none of it.

Thanks for taking the time to think about the proposal :). If your game already has a custom saveload system that satisfies all your needs, you won't benefit from this (or any) high-level Saving API. By analogy, if you already have a game with great multiplayer features based on low-level multiplayer functions, you have no reason to switch to a high-level API. On the other hand, I think new users just starting out, or even experienced users that want to make a rapid prototype stand to benefit a lot from the high-level API.

SlugFiller commented 1 year ago

Yes you do, that isn't trivial

Please provide a concrete example. Otherwise, the only response I can give to your "Yes you do" is "No you don't". Especially explain how MP is different in practice. "It's about synchronization" is just buzzwords. It performs the same actions under the hood.

This proposal doesn't make any mention of changing scenes, retaining states

And the physics documentation doesn't explain how to spawn a time grenade that flies in a ballistic arc, but it still makes it possible. Some basic ability to combine tools to your specific use case is expected. From the get-go "retaining state" is a phrase with multiple interpretations.

The first is keeping data when switching scene roots. Which is already a common, e.g. for making background music play without break across different levels. And the solution is known - don't use get_tree().change_scene_to_file, instead, have a near-empty "root" scene, and spawn actual levels, menus, etc as children of said scene. Then switching a level is as simple as freeing the node of the previous level, and spawning a new one. Data that persists between levels can be stored in the common root. At any rate, this has nothing to do with this proposal, and is something anyone with experience in the engine should already know.

The second is keeping data from a section of the game you're unloading. In which case the answer is simple: Call SaverAPI.encode, and put the data in a place you can retain, e.g. the aforementioned root scene.

And a third still is keeping data across restarts of the game. This requires saving to a file. You can directly call SaverAPI.save, or use FileAccess using data gathered by previous calls to `SaverAPI.encode)

no systems for custom loading, custom callbacks

Well, there's SaveSpawner.spawn_function, if you absolutely need it. But the situation in which you'd need it is rare. You still haven't provided a concrete use case.

it doesn't handle changes to the tree layout

It deals with nodes being spawned and unspawned, and can deal with them in a cascaded manner. If you save a game in a specific tree structure, and load it back, it will come back in the same tree structure that it was saved.

If you expect changes to the tree layout that don't involve spawning nodes (How!?), then use case please.

I don't see how this offers anything, for any but the most basic user as a beginner setup, over serialisation with groups other than a mild convenience, at the cost of these issues

First, the serialization using groups cannot handle order of spawn. i.e. if you have a spawner, that spawns another spawner, that spawns a synchronizer, etc, then the order matters. A roll-your-own solution has a good chance of screwing it up.

Also, the groups based solution from the tutorial is simultaneously a relatively large piece of code, and contains a potential security flaw (load(node_data["filename"]).instantiate() where node_data["filename"] can be any string), and contains game-specific hacks that has to be modified for each game (new_object.position =). Not to mention the part where the properties to be saved are defined in a large dictionary. And also, it's not clear how you differentiate between dynamically spawned nodes (e.g. bullets), and nodes that are pre-spawned and only need to have some properties saved (e.g. moving platform).

Saying that this is somehow equivalent to the above proposal is like saying AnimationPlayer doesn't add anything over lerp.

Using manual serialisation is far more flexible

Possibly, to the same extent that writing your own game engine in assembly is more flexible than using the tools and APIs in an existing engine. But what's the use case?

can be set up by even relatively inexperienced users

No it can't. If you copy-pasta the tutorial, you'll miss the dozens of parts that need to be adjusted specifically to your game. You'd have an error prone, unmaintainable solution. And worst of all, even if you managed to perfectly follow the tutorial, it's actually a bad example, because it contains a security flaw. Like a sprintf example that doesn't contain guards for buffer overflow, it's good for demonstrating APIs and getting a point across, but is NOT code that people should use in production.

is more reliable

Security. Flaw. Also, no error handling for bad input whatsoever. Enjoy your null pointer exceptions.

doesn't add a lot of extra clutter to a scene

But adds a shitton of clutter to the scripts, which is more critical.

It benefits and centers the tree system and layout, as it deals with nodes that should be synchronized or spawned, and having a specific tree layout matters

I have no idea what you're trying to sayhere. Are you presenting this as an advantage or a disadvantage? And if so, relative to what?

It is highly performance sensitive

I don't think CPU is going to be the bottleneck for multiplayer, but okay. GDScript is fairly slow. Still, you presented the use case of loading and unloading parts of the scene at runtime. Does this not require decent performance?

The bar for using the low level API with multiplayer is high, new users can't be expected at all to be able to deal with this

The bar for sending a packet across the network is no higher nor lower than reading and writing a bunch of bytes to/from a file. You could absolutely use the "group method" in order to send scene states in JSON format from server to clients over the network. It's just a bad idea to do so, because the inherent security flaw would open the clients to an attack from the server.

And, mind you, there's also plenty of things you don't get even with the multiplayer API, like any form of dealing with lag.

Trying to pretend like the low level multiplayer API is any more complex than the low level file API is a fallacy. You're either overestimating the complexity of multiplayer or underestimating the complexity of file saving. Or both.

The entire high-level multiplayer API can be summed up as:

  1. Save the game on the server side
  2. Send saved game over the network to clients
  3. Load the game on the client side
  4. Repeat steps 1 through 3 every frame

With a mild asterisk that you also have client-authoritative entities (but that should usually be limited to input only, for anti-cheating reasons)

So, 1 and 3 equivalent to file saving. 2 is low level API that is no harder to use than FileAccess. And 4 is just _process (or _physics_process), which exists in any non-trivial game. So the hardest parts are precisely those that overlap with a save/load API.

AThousandShips commented 1 year ago

I don't have the energy to continue this back and forth, you talk past all my arguments and seem less willing to consider feedback than the person actually making the proposal, this isn't constructive, I don't see the point in arguing with no regards to actually considering feedback, nor someone demanding a higher standard of evidence from others than themselves

Have a nice day

nlupugla commented 1 year ago

You don't need to "handle" default data.

Yes you do, that isn't trivial, and that very fact is important, MP doesn't care about this as it deals with synchronization

I'll update my proposal when I get a chance to be more clear about how data still in its default state is handled. For now, I'll try to elaborate here by way of example.

The developer handles default data through effective organization of their game into scenes and use of SaveSpawner/Synchronizer nodes. Consider saving a simple game like Dodge the Creeps https://docs.godotengine.org/en/stable/getting_started/first_2d_game/index.html. Think about how much data is contained in the player scene: all its textures, animation, collision shape, position, scale, rotation and so on. When you open Player.tscn in the editor, all of these properties are loaded to the default value you chose when you created the scene. During gameplay, only a small amount of this data changes from its default state, namely position, animation frame, and rotation. As a result, to save the player, the developer only needs to create a SaveSynchronizer and configure it to point to those three properties. All the other data, which remains in its default state, is handled "automatically" because of how you set up Player.tscn. Likewise, the enemies that spawn at run time from Enemy.tscn contain a bunch of data, but only a small amount of it changes during gameplay. Supplying a SaveSpawner with Enemy.tscn takes care of all that "default" data and lets you focus only on the things that change during gameplay. I hope that explanation helped clarify the idea. Let me know if that helps and if you have any suggestions for how I can modify the proposal to make these ideas more clear :)

This proposal doesn't make any mention of changing scenes, retaining states, etc., the fact that the tools for handling that are nominally there isn't the same

I hope my explanation above gives you an idea of how you might handle changing scenes and retaining states. If it doesn't, I'm happy to elaborate further.

there's only really room here for trivial and standard setups, with all relevant data strictly on nodes, with no systems for custom loading, custom callbacks, etc.

I don't know about "trivial", but I would agree that this API does have a "standard" setup in mind. That "standard" setup is one that makes full use of Godot's scene tree and nodes. One of the cool things about Godot is that it actually allows you to access its low level engine functionality via servers without ever having to touch the scene tree. If you've taken that approach, then the default implementation of SavingAPI, called SceneSaver, won't suit your needs. However, you are welcome to provide your own implementation of SavingAPI that doesn't rely on the scene tree at all. This is why the signature of object_configuration_add/remove allows the configuration parameter to be a Variant, rather than forcing it to be a NodePath.

This system also ties the saving and loading not only to the tree and nodes specifically, but also to a specific tree configuration, which isn't helpful or convenient, it deals with nodes and node paths, an inefficient setup, it doesn't handle changes to the tree layout.

Using scene unique names can go along way towards making the system more robust towards changes in the tree layout. On the other hand, if you are radically changing the structure of your game (perhaps you are refactoring some parts of the code), then any save system will need to be informed about your game's new structure.

I don't see how this offers anything, for any but the most basic user as a beginner setup, over serialisation with groups

Here are some of the advantages I can think of right now:

  1. Much less boilerplate code
  2. Groups rely on strings, whereas this proposal uses save_layer bit flags. The editor can provide much better tooling for bit flags (eg: auto completion)
  3. Its easier to combine layers than groups. For example, compare units_layer = PLAYER_UNITS + ENEMY_UNITS with saveable_units = get_nodes_in_group("saveable_player_units").append(get_nodes_in_group("saveable_enemy_units"))

There are certainly others points that don't come to mind at the moment. It is probably worth compiling what exactly these advantages would be and adding them to the top proposal.

Using manual serialisation is far more flexible.

This is a point I'd like to make the proposal more clear on. The proposed API is manual serialization. The difference is that the "manual" part is shifted from writing boilerplate to configuring nodes. Unless you have chosen to forgo Godot's scene and node system, the proposed API should provide you with all the same flexibility that you would get from coding the serialization/deserialization yourself.

And additionally, the main justifications and benefits, as I see it, of the MP system is:

I would argue that your points 1. and 2. apply to saving/loading as well. Regarding point 3., I agree that the barrier to entry for multiplayer is higher than saving/loading. However, I think it's still worth making that barrier to entry as low as it can possibly be. In addition, there are probably many more games that use saving/loading than those that use multiplayer. So even if the barrier to entry isn't quite as high, the motivation to make it small is stronger.

Finally, thanks again for taking the time to contribute and give feedback :) I think good saving/loading features have the potential to make Godot better for everyone, so it's great to hear thoughts from as many folks as possible!

SlugFiller commented 1 year ago

@AThousandShips I've provided use case examples, you did not. I'm open to taking this to RocketChat if you don't like the forum long format. However, calling your comments "feedback" is a stretch. "Feedback" would provide suggestions on what to add or fix, or at least provide examples that would give a hint as to what might not be possible to achieve with the feature in its suggested form. Claiming that the feature adds nothing compared to a flawed, more complex alternative, or claiming that it doesn't do things that it clearly does, is not that.

AThousandShips commented 1 year ago

@nlupugla I'd love to continue this with you, but I simply do not feel welcome here with this agressive response, I hope my already provided suggestions and points will help

And I want to clarify that much of it was not directly aimed at you, and I'm sorry that I came across as excessively dismissive of your ideas due to the ongoing discussion getting heated, as while I don't really see the benefits of this proposal, I aimed to provide cases and contexts for where the proposal, and the idea, in my opinion needed work. And especially in the potential pitfalls of approaching it from a perspective that limits it, or assumes things apply that mostly apply to the MP side

Thank you for your response, and hope to talk on some other context in the future, but here I am clearly not welcome

Take care!

AThousandShips commented 1 year ago

@SlugFiller I have no problem with this format, what I do have a problem with is your hostile attitude, and your petty attitude dismissing my points as "barely even feedback" (paraphrasing), it honestly makes me sad when I don't feel like engaging any more because of this

Again, have a good day, and leave me alone when I tell you I'm leaving

and-rad commented 1 year ago

All in all, this seems overengineered to me. I don't think there needs to be a dedicated save/load API that is as complex as the proposed one. I mostly agree with @smix8 and @AThousandShips. I don't think it is too much of a burden on the engine users to build their own savegame system given a good documentation of the relevant classes.

nlupugla commented 1 year ago

All in all, this seems overengineered to me. I don't think there needs to be a dedicated save/load API that is as complex as the proposed one. I mostly agree with @smix8 and @AThousandShips. I don't think it is too much of a burden on the engine users to build their own savegame system given a good documentation of the relevant classes.

Thanks for sharing your thoughts!

To be clear, is your criticism that the proposed API is too complex, or is it that Godot shouldn't provide a Node-based save game system because it's not too hard for engine users to roll their own?

If the API seems too complex, do you have any suggestions on how it could be simplified?

If you don't think Godot should provide a save system for users, I politely disagree. Here are some reasons I think Godot should provide tools specifically for save systems.

  1. There are soooo many points of failure in a save system. Just think about how many speed runs are enabled by abusing glitches in games' save systems. Plus, there are the security concerns with saves that rely on custom resources. If Godot provides a save system for users, the entire community can come together and work on plugging points of failure.
  2. Having a standardized API makes it easier for the community to develop and share addons in the asset library related to saving. For example, if someone makes a cool save menu/preview screen and shares it on the asset library, you can easily integrate it into your game because you both used the same API.
  3. The current tutorials and documentation on building a savegame system aren't very good. As they say, the best code documents itself. Having a Node-based approach for configuring your save system with an intuitive user interface could greatly reduce the amount of documentation and tutorials you need to write in the first place.
and-rad commented 1 year ago

It's probably more that the proposed API is too complex.

For example, the layer system: I have a hard time to come up with use cases where I would want to save or load certain portions of the game and use layers for that. It's also still not quite clear to me what exactly SaveSpawner and SaveSynchronizer are supposed to do.

Also, there are too many assumptions implied in the system. take the signature of the save method:

func save(path: String, object: Object = null, section: StringName = &"", layer_flags: int = 1) -> Error

It expects an object, I assume it's the object that's going to be saved. How does the save system determine which of the object's properties to save? All of them? That would just bloat the save file. Only the @exported ones? I can tell you from experience that a lot of the data stored in a savegame is internal state.

I think what would help your case tremendously is some kind of reference implementation.

The points you raise don't seem too important to me personally.

  1. I'm not interested in speed running, so I don't know of a single case where the save system was exploited for that. From the limited exposure I have, it seems to me that collision and physics glitches are way more common. But again, I simply don't know enough to have a qualified opinion here.
  2. This sounds utopian to me. If there was a generic savegame metadata struct, it would probably contain a date, a name, and maybe a screenshot (and even that won't be needed by everyone). Everything else is game-specific, which means this is about the extent to which addons can incorporate the save API. Addons are useful when they're being more agnostic about the underlying technology, not the other way around.
  3. This seems to be purely a documentation issue. The system you're proposing will need documentation just as much as any other workflow for saving data. Look at the kinds of questions people ask on Reddit on a daily basis. Code that documents itself just doesn't cut it with a significant part of Godot's target audience.

I think the most productive way to create a usable, generic save system is to provide a singleton class that has methods for

  1. creating, serializing, and deserializing (preferably in text form as well as binary) savegame objects,
  2. providing getters and setters for what is considered the "current savegame",
  3. providing access to and information about all existing savegames

My guess is that this would probably work out to some fancy wrapper around ConfigFile.

nlupugla commented 1 year ago

Thanks for the detailed feedback! I can agree that the layer system might be overkill. The original idea was that if your game had a level editor and a player character, you might want to only save/load the level editor or the character, so you could put the level and the character on separate "save" layers. However, the same thing could probably be accomplished by just passing the character or level scene as the object parameter.

As for your points about which properties of the object will be saved, I could definitely make my proposal more clear and self-contained. The idea is to configure which properties you want to save via an editor plugin. The experience will be very similar to configuring a MultiplayerSynchronizer Node. When I get a chance, I'll add some screen shots to my OP to make this more clear. In the mean time, you can get a good understanding of the intended UI by adding a MultiplayerSynchronizer to any scene and playing around with it.

Edit: @and-rad I've added some screenshots to my original post. Hopefully that makes the intended system a little more clear!

Faless commented 1 year ago

@nlupugla I think you are making your life harder by trying to adapt the multiplayer code for saving. While the high level configuration (spawner/synchronizers) might be similar, the underlying requirements are too different:

I honestly think an initial implementation in GDScript would be better in this case but this is up to you. In general, what I had in mind was something on the line:

nlupugla commented 1 year ago

Thanks for the helpful comments @Faless! Taking your advice to put aside the multiplayer code for a while has been helpful. I've been focusing more on just extracting useful info from my SaveloadSynchronizer node and I've now implemented a get_state function which returns a dictionary mapping NodePath to Variant primitive (ie: non-object) property values.

Your points about serializing/deserializing in the correct order and save format are really good and I'd love to get some feedback on how I'm currently thinking about doing it.

Load order:

  1. From the top of the scene tree going down, spawn/despawn nodes.
  2. From the bottom of the scene tree going up, synchronize nodes. This method assumes that the properties being synched don't have any troublesome side effects like setting other synched properties or despawning other nodes.

Save format: I don't think we should be too proscriptive about the exact format users end up with. We can and should implement a "default" format that should work for the majority of cases, but I think something that would be great is having some kind of intermediate representation of the data which makes it easy to store to e.g. binary or JSON as desired. Here is what I'm imagining the intermediate representation might look like:

{
    &"Spawners" : {
        ^"Global/Node/Path/To/Spawner1" : {
            ^"Relative/Node/Path/To/Spawned/Node" : true,
            ^"Relative/Node/Path/To/Despawned/Node" : false,
            ^"Relative/Node/Path/To/Another/Spawned/Node" : true,
        },
        ^"Global/Node/Path/To/Spawner2" : {
            # similar to above
        },
        # and so on
    },
    &"Synchers" : {
        ^"Global/Node/Path/To/Syncher1" : {
            ^"Relative/Node/Path/To/Node:property" : 3,
            ^"Relative/Node/Path/To/Another/Node:with:different:property" : [2, "hi", false],
        }
        ^"Global/Node/Path/To/Syncher2" : {
            # similar to above
        }
        # and so on
    }
}

Let me know if you have any thoughts or feedback :)

KoBeWi commented 1 year ago

I did actually implement a super complex save system for Lumencraft (you can check out the game for more context). The saves are stored in custom binary format that contains all relevant data to restore the game state to a somewhat similar state. Here's more or less how it works:

Based on the above, a generic SaveLoad system that would fullfil this use-case needs:

  1. Some way to determine which objects are saved
  2. Some way to determine what part of the object is saved and how
  3. Probably some more stuff I have forgotten

For 1, I think synchronizers are enough. You can just go through all synchronizers and save their object. I don't get the purpose of spawners. I assume they are to restore objects that spawned during gameplay? What about destroyed objects that were originally part of a scene? Note that if the gameplay is complex enough, it's more efficient to store the whole state than the "difference" For 2, I don't see any way to customize how something is saved. I might have an object with a complex state which can be simplified, but can't be stored in variables. In my case I had the _get_save_data() methods, but I don't see equivalent in the proposed system (then again, you didn't explain the proposed API)

That said, such complex system is only required for games where much of their state is relevant, like FPS, RTS, sandbox or open world games. There are many genres that don't need such system at all:

Majority of the games fall in the latter category, i.e. games that don't need exact world representation. And games that do need it might have many special cases that make a general system insufficient.

But as emulators have demonstrated, there's nothing actually game specific about it.

AFAIK emulators just dump the whole memory. You can't do that efficiently with games that use 100+ RAM (and modern games use much more than that). I did once play a game inside a VM to have save states and don't recommend that.

EDIT: Ok one thing I have forgotten is compatibility. If I have a custom format, I have complete control over it and can migrate saves from any version. The save system needs to ensure it's possible.

SlugFiller commented 1 year ago

@KoBeWi For 2, the synchronizer allows configuring which properties are saved in the same way AnimationPlayer let's you choose which properties are animated. As in, a panel pops up from the bottom of the screen, where you add property paths to a list, and these are the properties that are saved. If a complex's state is so complex that it cannot be represented as a subset of its variables, you can replace it with a single exported variable with a custom setter/getter that represents a pre-packed version of the object. So long as it's a primitive type (e.g. a dictionary), it would still work.

On the topic of an RPG just saving the current room:

On the topic of only saving unlocked levels: It's still possible to use this system to save, say, the level selection screen. But it's overkill, of course. If all the info in your game can be saved in a single bitmask, you wouldn't even consider using a system for it, nor need to (Saving a bitmask is already a single line). Just like you wouldn't use a navigation agent if your game is set in space and there are no walls. It's okay to not use a system if you don't need it.

However, saving only unlocks is a design decision. The exact same game can choose to allow you to save a half-completed puzzle so you can come back to it later. There's no reason not to do it, other than it potentially being out of scope for your development time. Making it just a couple of clicks to add could change this decision. Maybe not always. Maybe a certain balance requirements requires no save-scumming. But there will be some cases where it isn't, and having a ready-amde system will indirectly benefit players.

Your post does introduce a few actual problems that need considering:

  1. Despawning items that were spawned with the scene. We had a long discussion about whether items inside a spawn root that are spawned with the scene need to be tracked or not. And this scenario was not considered. This is definitely a use case that needs to be considered.
  2. Serializing a texture. The API, as it is, only serializes properties that are elementary types (int, String, Array, Dictionary, Packed*Array, etc). It's possible to achieve this by making a custom property with a getter that returns the texture as a PNG. But if this could be considered a common use case, it might be worth to add specific non-elementary types that have custom built-in serialization, e.g. textures. This might be necessary for TileMaps regardless.
  3. Custom save format. This one was already discussed in the chat. But it's a complex subject. At the moment, the encoding returns a primitive (Dictionary, I believe), that, by default, is encoded with var_to_bytes, but this can be exchanged with a different encoder. If the format of the Dictionary is documented, this should also allow game-specific encoding (i.e. an encoding that uses intimate knowledge to save specific nodes or properties in specific ways), but I'm not sure if you get much of an advantage from using the system if you go that far, since you're basically doubling all the node-specific code inside the encoder.
KoBeWi commented 1 year ago

On the topic of an RPG just saving the current room:

I think you misunderstood this one. By "current room" I meant something akin to "current level". Upon load, the level is reinstantiated from a fresh scene, not from a saved state. Only a few objects (like the mentioned chests, switches etc.) are "saved", but this is simple array/dictionary of bools.

However, saving only unlocks is a design decision. The exact same game can choose to allow you to save a half-completed puzzle so you can come back to it later. There's no reason not to do it, other than it potentially being out of scope for your development time.

I don't remember playing any game that saved puzzle state. Almost always they are either finished or not. Yes, it's a design decision, which comes from the fact that there is no reason to save half-finished puzzle. Partially it's not worth implementing, but even if there was easy way to do it, most games would still reset the puzzle.

It's possible to achieve this by making a custom property with a getter that returns the texture as a PNG. But if this could be considered a common use case, it might be worth to add specific non-elementary types that have custom built-in serialization, e.g. textures.

Nah, this is just a very specific use-case. I think a custom getter would be enough to support it.

Custom save format.

By custom save format I just meant a way to handle compatibility. If the SaveLoad system would allow to somehow modify the saved data during loading it could be enough. The problem is that the data is basically a black box and compatibility sometimes requires sophisticated hacks to work, which are not possible without deep knowledge/access to the internals.

nlupugla commented 1 year ago

@KoBeWi Thanks for the detailed comments! I was looking at your game (which looks super cool by the way 🥳) and noticed all the destructible terrain. I'm wondering if you'd be open to share how you save the state of the map. The reason I ask is that I built the Spawner nodes under the assumption that it's more common to want to add entities during run time than remove them. This would suit a game that starts with an empty level and spawns enemies at runtime. However, a game with destructible terrain is a great use case for a system that despawns nodes, not just spawns them. This would also be an issue for multiplayer, and I wonder how your game handles synchronizing the destroyed terrain across different multiplayer clients. Do you use the high-level multiplayer api, or something lower level?

nlupugla commented 1 year ago

I'd also like to address concerns about loading a game in which the order that properties are set matters.

There are two ways I've thought about addressing this.

  1. In the Synchronizer configuration UI, give each property an integer "order" parameter that lets users configure the order the properties will be synchronized. For example, a property configured with order = 2 will be synchronized after all properties with order = 1.
  2. Explicitly document that the order properties are synchronized in is not guaranteed and force users to design their project in such a way that the order properties are synchronized does not matter. On the one hand, this approach feels kind of mean and restrictive, on the other hand, I do think it's best practice to avoid making setters be order-dependent. In that sense, forcing users away from that anti-pattern might actually be a good thing overall.
KoBeWi commented 1 year ago

Thanks for the detailed comments! I was looking at your game (which looks super cool by the way 🥳) and noticed all the destructible terrain. I'm wondering if you'd be open to share how you save the state of the map.

Well just as I said, we're saving the whole scene into a custom binary format using store_var() etc. All objects with their relevant properties and the map itself is saved as PNG. Which means that if we change a map and someone loads an old save, they will play the old version of the map, because it's copied inside the save file. Saving process takes so much time that we eventually moved it to a thread to at least show that something is happening, and part of the system was also moved to C++ (which didn't help too much). Long saving times is not unheard of in sandbox games though; one example would be Satisfactory.

This would also be an issue for multiplayer, and I wonder how your game handles synchronizing the destroyed terrain across different multiplayer clients. Do you use the high-level multiplayer api, or something lower level?

We don't have online multiplayer 😅 It's something we wanted to have, but the game wasn't properly designed towards multiplayer from the beginning and then it was too late. Can't tell what exactly approach we'd take, but it was doable.

I'd also like to address concerns about loading a game in which the order that properties are set matters.

Wouldn't it be enough to use the order of properties set inside synchronizer?

nlupugla commented 1 year ago

Wouldn't it be enough to use the order of properties set inside synchronizer?

That's another option, and now that I think about it, that is the order that the current implementation uses. I guess the only advantage of the "order" integer is that all the objects with the same "order" value could potentially be saved/loaded in parallel, potentially speeding up saving and loading.

We don't have online multiplayer 😅

Honestly, props to you and your team for putting couch co-op above online multiplayer! I'm so disappointed when I see that certain games have online multiplayer but not couch co-op.

Which means that if we change a map and someone loads an old save, they will play the old version of the map, because it's copied inside the save file.

Do you consider this a "bug" or "feature" of your system? In other words, if technology were no barrier, how would you want compatibility to be handled in your game? I do think compatibility is one of the main things that makes save/load different from multiplayer.

Saving process takes so much time that we eventually moved it to a thread to at least show that something is happening, and part of the system was also moved to C++ (which didn't help too much).

That's super interesting to me, especially that moving to C++ didn't speed things up much. I was thinking about whether the save/load system in Godot should come with its own profiler/debugger plug-in to help users figure out what parts of their game are taking the longest to save/load. Do you think a tool like that would have helped in your case? Do you know where the system spends most of its time saving/loading? Is it on loading the map? The other objects?

KoBeWi commented 1 year ago

Do you consider this a "bug" or "feature" of your system? In other words, if technology were no barrier, how would you want compatibility to be handled in your game?

I'd say it's a limitation imposed by the game design. The only way to update a map that can be modified per-pixel is to track whether each pixel was modified by the player and only replace unmodified pixels. Even then, the results could be very bad, because a player may suddenly get a new tunnel to their base or may have modified a terrain in such way that updating unmodified pixels would make the map unwinnable. Even moving objects could be harmful; if a player builds a defense around monster spawner and the spawner moves to another spot, they'd be screwed. Major changes in map would make mess if propagated to old saves, so it's just not worth to implement. That said, we did keep format compatibility, like updating property names or values to new range etc. In some versions we even had to perform full map conversion, because internal structure has changed. The game's version is loaded as the first thing, so if you know what version is being loaded, you can modify the order of loading things, skip some steps or modify the loaded data, which allows to handle basically any change. The system ensured that save from any older version is playable.

Do you know where the system spends most of its time saving/loading? Is it on loading the map? The other objects?

Most likely the objects. The _get_save_data() methods I mentioned were still written in GDScript, because most of the objects needed one and they couldn't be moved to C++, because it's part of the script. Also the system traversed the whole scene recursively and then checked each property in the storable objects. I guess this is where your system could bring improvements, since instead of going through everything, you could just go through a list of synchronizers and then their property lists. Though this is also where setters and getters would make things slower.

nlupugla commented 1 year ago

Most likely the objects. The _get_save_data() methods I mentioned were still written in GDScript, because most of the objects needed one and they couldn't be moved to C++, because it's part of the script. Also the system traversed the whole scene recursively and then checked each property in the storable objects. I guess this is where your system could bring improvements, since instead of going through everything, you could just go through a list of synchronizers and then their property lists. Though this is also where setters and getters would make things slower.

Huh, I had never really thought about the performance improvements of the save/load system before, but now that you bring it up, there is probably a lot to be gained from the fact that this save/load system is implemented in C++ and traverses a relatively flat list of properties compared to the way many people probably implement save/load, which is by traversing a highly nested scene tree in GDScript. I wonder if it would be possible for users to select an option that lets certain properties "bypass" setters and getters for extra performance.

KoBeWi commented 1 year ago

I mean, the setters/getters were suggested as a workaround for no way to customize the saved data, so they are necessary. But now that you mention it, there are probably cases where you don't need them, but bypassing may result in wrong value.

nlupugla commented 1 year ago

The bypass would certainly have to be optional. It's true that if you're not careful, bypassing the setters might put your game in an invalid state. On the other hand, most of the time you load, you are presumably loading a valid game state.

Edit: I wonder if bypassing setters is something that has ever been proposed for the multiplayer synchronizer for extra performance.

RudyFisher7 commented 11 months ago

I guess I maybe don't understand this proposal fully... Godot auto serializes any @export data of Resources types and can be used with ResourceSaver.save...() and ResourceLoader.load...() on that Resource. I like the idea of having a high level API like the multiplayer peer stuff with a synchronizer though. Is the synchronizer analogous to the network synchronizer where it is saving the properties in real time?

nlupugla commented 11 months ago

Thanks for the feedback :)

The synchronizer is analogous to the network synchronizer, although currently it's not set up to save every frame (although you could pretty easily do that if you wanted by calling SaveloadAPI.save in a _process method.)

Custom resources are very powerful, but they pose a security risk. Opening them runs arbitrary code. If a player downloads a save file with malicious code in it, their computer would be at risk.

Also, there are properties that aren't on resources that are still worth saving, such as the position and velocity of a KinematicBody. Sure, you could copy them in to a resource or dictionary, but that ends up requiring quite a bit of boilerplate.

Stwend commented 11 months ago

My 2 cents: Saving and loading games is so tightly intertwined with a game's mechanics that it doesn't make sense to try and write a one-size-fits-all API. From what I can derive from the posts ITT, my game's save and load cycles are similar to KoBeWi's approach but differ in a few spots, I wouldn't be able to use his system for a perfect fit and probably vice versa, and there are games where it just doesn't make sense to have anything more than a JSON file to store a few variables in. With all these approaches, we'd have the API and everyone would be writing their own API to fit their game or style of coding anyways.

nlupugla commented 11 months ago

Thanks for the two cents! I have heard the "saving and loading is super game dependent" line many times. I see where it's coming from: the kind of data that one game will save is very different than the kind of data that another game will. But at the end of the day, saving and loading boils down to the same, game-independent procedure: choose the properties and entities you want to persist, then serialize that data. The situation with multiplayer is totally analogous: each game will have a very different set of data that needs to be shared over the network, yet the MultiplayerSynchronizer + MultiplayerSpawner system has been working for many users.

In any case, I'd love to hear a bit more about saving in your game :) This work is still in its early stages and if there is a type of game that the system doesn't cater to well, I'm keen on seeing how the system can be adapted.

P.S. I don't want to put words in KoBeWi's mouth, @KoBeWi very generously took my system for a spin and was able to set up saving and loading in their game with it :)

Stwend commented 11 months ago

All right :) Keep in mind that since my game is an early WIP, the saving/loading procedure isn't final at all.

As of right now, I have a few Manager classes that handle game systems or a large group of entities with each one basically storing an array of serialized/deflated data. When saving occurs, all of the manager classes are called by signal to store their current information inside the savegame object which is then saved to disk on a separate thread (side note: I plan to offload the information storing to a thread too, this will require some "buffer swapping" logic in order to be sufficiently performant and game state safe).

Serialization isn't done at save time for the vast majority of the entities, each one of them decides when to "pre-serialize" and store/update its serialized duplicate inside its manager's array. Some don't serialize at all - if e.g. an item didn't change its state (not picked up, same position as spawned, etc) it won't serialize (and subsequently won't exist in the savegame) and on load time it'll be loaded from the level specific "standard savegame" where all default states, positions, and other variables are noted. I plan to extend this to be game-wide in order to be able to access any important variable of any item from any level ("Did the guy named "X" die by bullet 3 levels prior?").

Some entities like the player character never do this, their variables change often and serializing a character each frame/physics tick makes no sense. When their manager's "quick, store your info in this object" method is called, they serialize "then and there". All other objects just check if their serialized duplicate is in fact up to date and if not, they too serialize immediately.

Loading a savegame will load the base level first (everything that isn't considered an "entity", e.g. static geometry) , then each manager will first deserialize anything that's inside the savegame object (storing each entity's UUID) and after that everything inside the level's standard savegame that "hasn't been mentioned before" (= the UUID hasn't been registered when loading from the savegame).

Why I think this system is a bad fit for most games:

To sum it up, it forces a lot of structure and trade-offs - it makes sense for my game but makes no sense at all for a lot of other games.

Don't get me wrong - I do think a generic save game system could be very beneficial for a lot of people, I just don't think it should be core :)

KoBeWi commented 11 months ago

KoBeWi very generously took my system for a spin and was able to set up saving and loading in their game with it :)

To clarify a bit on that, it's not the same game I described before, but a very similar one. We are in early prototype stage and I just evaluated the proposed system whether it's viable. It works surprisingly well, the objects I wanted saved were automagically saved like I wanted. The only missing feature is object despawning, i.e. you destroy an object that was originally on the scene and don't want to load it again. But yes, this system is mostly useful for like half of the games (by genre; by popularity it's less, because e.g. not many people make RTS games). One concern is that if we have it in core, people will just see "SaveLoad system" and use it, whether they need it or not (and in most cases using a simpler custom system would be better for them).

SlugFiller commented 11 months ago

I still prefer people use this provided SaveLoad system over a custom one, for the simple reason that a custom one can be an easy vector for viruses spreading via Godot games. Besides the PR nightmare that such a thing would be, I would rather know that when I run a game made in Godot on my computer (say, if I bought one on Steam), I'm largely in good hands.

The fact that even the official tutorials are vulnerable shows just how easy it is to "get it wrong" and end up infecting the computers of unwitting users. So basically, I prefer "I don't know what I'm doing, let's use this pre-made component", over "I don't know what I'm doing, let's try to improvise my own thing and see if my users lose their precious data due to trusting my game".

Put differently, I rather they try to hammer in that screw in with a hammer than with a stick of dynamite.

KoBeWi commented 11 months ago

Except the proposed SaveLoad system can't be even used for some types of games. E.g. when you have a platformer with linear levels and you only save what levels are completed. How would you handle that? A custom save system can be a simple txt file with a list of finished levels. Perfectly safe and easy to write.

nlupugla commented 11 months ago

Thanks for the details Stwend! It's very generous of you to write out so much for your WIP <3 I think this is actually a great example of how much wheel-reinventing devs tolerate when it comes to save systems that I hope my proposal can solve :) I'll try and describe how I think this proposal could work in your case.

First of all, I plan to bake multithreading into the SaveloadAPI (though I haven't implemented that yet) so that all the complexity of saving and loading on different threads is handled for you.

Second, SaveloadSynchronizer and SaveloadSpawner are essentially taking the role of your manager classes. When the user calls SaveloadAPI.save each one of these Saveload nodes starts to serialize. There currently isn't a plan to "pre-serialize" or cache data, because I think that will rarely give a noticeable advantage (although if testing reveals that to be an incorrect assumption I'm happy to reevaluate; in fact, it would be fairly trivial for me to give each Saveload node a cache function and a dirty flag).

Third, this proposal leverages Godot's scene system to define the "standard save". Only the properties and spawns that the user registers with a Saveload node are serialized. Every other element of a scene will simply take the value specified in its scene file.

Fourth, my proposal naturally supports having a sort of "game-wide" data base like what you've described. The current save format is essentially a dictionary mapping NodePath to value. For example Main/Player:position:x -> 3.2. This means any Node can easily query a given property from the global SaveloadAPI if it knows the relevant NodePath.

Finally, a point that I think is crucial but I haven't stressed much before is that my system makes what is being saved completely transparent to anyone editing a scene in your game. You don't have to navigate through scripts to decipher what is being saved when and where: it is all laid out plainly in the editor. This is a benefit even for games with simple save states and logic.

I think as soon as you try this, you will appreciate the power and flexibility this system has to offer :) Feel free to try out the system by pulling my PR or downloading the relevant editor artifact. Alternatively, if you have a version of your project you'd be willing to share, I'd be happy to see if I can get saving/loading working on it with my system. I am absolutely in the market for more projects to test my system on :)

nlupugla commented 11 months ago

Except the proposed SaveLoad system can't be even used for some types of games. E.g. when you have a platformer with linear levels and you only save what levels are completed. How would you handle that? A custom save system can be a simple txt file with a list of finished levels. Perfectly safe and easy to write.

Easy! Add @export var current_level : int to your Main or Player node. Add a SaveloadSynchronizer as a child and configure it to synchronize current_level. That's it!

nlupugla commented 11 months ago

Note: this assumes that current_level has a setter which implements your logic for changing levels.

KoBeWi commented 11 months ago

ok xd That's not really what I meant, but it indeed can be made to work. I'm more thinking that you have a level select menu where there is list of levels that you can play. Rayman games are a good example. But I guess you could just save your level select scene, which can keep track of completed levels.

I usually keep such data separated. I have a Save singleton or something where I keep stuff I want saved. Your SaveLoad system basically inlines it; instead of keeping a data in a data place, you keep it inside your game and save the game scene instead. It's kinda backwards and not sure if it's a good approach, but maybe people implement saving like that, idk.

Stwend commented 11 months ago

Thanks for the explanation and thoughts, @nlupugla, very much appreciated! Right now the project is a huge mess with tons of loose ends, but once it's in a state where code is a bit more tidy and much more interchangeable than it is now, I'll try out your PR and send you some project files :)

About Point 2: Caching serialized objects is really only beneficial when there's a lot of them and a lot of them don't change much, I just implemented that to avoid a hickup at save time and it did speed up the process in my stress tests (hundreds of thousands of simple entities).

nlupugla commented 11 months ago

I usually keep such data separated. I have a Save singleton or something where I keep stuff I want saved. Your SaveLoad system basically inlines it; instead of keeping a data in a data place, you keep it inside your game and save the game scene instead. It's kinda backwards and not sure if it's a good approach, but maybe people implement saving like that, idk.

I think you've nailed it with that comment! Adopting this proposal does require a bit of a paradigm shift: save logic lives in/on the relevant game components, not in a global manager.

However, I'd argue the paradigm in this proposal aligns more closely with Godot's scene system. Scenes are meant to be self-contained: if I want to change something about the player character, I open Player.tscn and start making changes. If I also what to change how my player is save/loaded, I should be able to do that from within the scene editor, not dig through autoload scripts that might be in a totally different part of the file directory.

SlugFiller commented 11 months ago

For a "level selection" it's easy enough to save the data of which levels are unlocked in the level selection menu scene itself. Alternately, you can hold it in a "root scene".

Even without save-load, you need to keep that data in memory somewhere anyway. All that changes is that you expose that piece of memory via @export. If anything, this is a case where the solution is extremely simple.

And, it's probably easier than even using a text file, because no need to do any parsing.

As for "current level" in a linear game, if you use the sane pattern of spawning the level scene inside a common root node, instead of switching the entire tree (Pretty much necessary for stuff like keeping stats or playing music between levels), then you can simply place a SaveloadSpawner under that root node and have it save the spawn of the current level. Remember, a spawner only saves the fact a scene is spawned, not its current state. That is handled by additional nodes inside the scene. So, if you only want to save the level, but not your current position/checkpoint/etc, then this takes care of it.

nlupugla commented 11 months ago

I just realized my OP didn't link to my PR, so I've gone ahead and included a link to https://github.com/godotengine/godot/pull/79644 at the top.