Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saveload API #79644

Draft
wants to merge 36 commits into
base: master
Choose a base branch
from
Draft

Saveload API #79644

wants to merge 36 commits into from

Conversation

nlupugla
Copy link
Contributor

@nlupugla nlupugla commented Jul 18, 2023

This PR introduces an API and editor interface for saving and loading games inspired by the high level Multiplayer API and based on the discussion in proposal godotengine/godot-proposals#7041. I have been testing the project with a small demo: godotengine/godot-demo-projects#935.

Usage Guide

instructions The workflow for the Saveload API should be familiar to anyone who has used the high level MultiplayerAPI before. Here are The steps I followed to add saving and loading to the demo project linked above.
  1. Start from a basic "game" without any saving/loading where the player can move and every so often, an "enemy" will spawn.
image
  1. Add a SaveloadSynchronizer to the main scene.
image
  1. Click the SaveloadSynchronizer and begin configuring it using the Save Load plugin that appears at the bottom of the editor.
image
  1. Click the "Add property to sync" button to bring up a list of nodes in the current scene.
image
  1. Select the "Player" scene to bring up a list of all of it's saveable properties. Almost any Variant type can be selected, including Dictionary and Array. However, Object is not allowed, as a bad actor could theoretically inject malicious code into the save data for an Object, which would run as soon as the harmful save file were loaded.
image
  1. Choose the position property and observe that the Save Load plugin now displays Player:position as a property to be saved.
image
  1. Open the "Enemy" scene and repeat.
image
  1. Back in the Main scene, add a SaveloadSpawner node. The node should show a configuration warning because we haven't yet assigned a Spawn Path.
image
  1. Use the inspector on the right to assign the Main node as the Spawn Path.
image
  1. Expand the Auto Spawn List in the SaveloadSpawner inspector pane on the right and click "Add Element" to add the "Enemy" scene to the Auto Spawn List.
image
  1. We are now down configuring the saving and loading system for this extremely simple "game". All that's left is to call Saveload.save and Saveload.load at the appropriate time. These might be called from a script like so:
func _unhandled_input(event: InputEvent) -> void:
	if event.is_action_pressed(&"save"):
		SaveloadAPI.save("res://TestSave")
	elif event.is_action_pressed(&"open"):
		SaveloadAPI.load("res://TestSave")

Additional Info

  • The data is saved in a binary format, so the savefile will not be human readable. However, you can use SaveloadAPI.serialize to save your game's data to a Dictionary in memory, which you can then print for debugging purposes.
  • Several SaveloadAPI functions take a configuration_data parameter, however, this parameter is not currently used, so should be left blank for now.

Edit 2023/July/18 19:36: added link to demo project.

Edit 2023/July/19 14:19: attached zip of demo project
saveload_api.zip

Edit 2023/July/24 18:07: added "finish cleaning up vestigial multiplayer code" to todo list.

Edit 2023/July/25 11:21: added "support multithreading to facilitate loading bars and save wheels" to todo list.

Edit 2023/September/09 11:22: added usage guide

Edit 2023/September/18 13:05: made usage guide collapsible

TODO:

  • Testing and feedback from other Godot devs
  • Testing and feedback from Godot users
  • Finish cleaning up vestigial multiplayer code
  • Documentation
  • Allow sub "sections" of the game to be saved and loaded
  • Support multithreading to facilitate loading bars and save wheels
  • improve the synchronizer configuration UX with ctrl and shift click multi-select
  • improve the synchronizer configuration UX with wild card pattern matching
  • create custom resource format saver and loader for saveload files

@AThousandShips
Copy link
Member

AThousandShips commented Jul 19, 2023

Translations shouldn't be added or edited manually, they are generated separately and synced from the webplate

Will take a deeper look when I have time!

@nlupugla
Copy link
Contributor Author

Thanks! At some point I did a refactor of Multiplayer -> Saveload and it must have touched more parts of the code than I realized. Should be easy to fix :)

@TheYellowArchitect
Copy link
Contributor

If you added the link to the demo project redirect directly to a .zip instead of another issue, there would be more feedback :P

@nlupugla
Copy link
Contributor Author

If you added the link to the demo project redirect directly to a .zip instead of another issue, there would be more feedback :P

Thanks for the interest! I've updated my original post with the zip file :)

@TheYellowArchitect
Copy link
Contributor

TheYellowArchitect commented Jul 20, 2023

I cannot run the zipped project in 4.1. The same bug applies when I git clone your your branch saveload_api
error-saveload-api

Edit: I tried on 4.2dev1 too, same error pretty much
saveload-error2

@nlupugla
Copy link
Contributor Author

This feature is still in draft stage, so it hasn't been merged into 4.1 or 4.2 yet.

Try running the demo using an exe compiled from this PR. The easiest way to do it would be to checkout my saveload-api engine branch, compile, then open the project with the newly compiled Godot exe.

I can see if I can attach my custom exe to this PR, but I'm guessing it will be too big and/or github might have security restrictions against uploading exes.

class SaveloadAPI : public RefCounted {
GDCLASS(SaveloadAPI, RefCounted);

static SaveloadAPI *singleton;
Copy link
Member

@AThousandShips AThousandShips Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It being RefCounted and having a singleton are mutually exclusive, singletons are unique, handled by the system, not created or freed by the user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip! I figured as much from reading through the error logs, but it's nice to have confirmation that my suspicion was right :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AThousandShips In changing from RefCounted to Object, will I have to add some logic to free SaveloadAPI to avoid a memory leak?

All I have for the destructor right now is

virtual ~SaveloadAPI() {}

Copy link
Member

@AThousandShips AThousandShips Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at how it's done in other modules on register/unregister, like navigation with the mesh generator there

You will want to clear the singleton pointer on destruction to be safe

@AThousandShips
Copy link
Member

I'd recommend using clang-format to ensure that the formatting is correct

@nlupugla
Copy link
Contributor Author

Doh! I set up clang on my desktop but forgot to on my laptop xD

GDREGISTER_CLASS(SceneSaveloadConfig);
GDREGISTER_CLASS(SaveloadSpawner);
GDREGISTER_CLASS(SaveloadSynchronizer);
GDREGISTER_CLASS(SceneSaveload);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this too needs to be abstract, as you should never be able to create one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, really? All of these are concrete classes though. SaveloadSpawner and SaveloadSynchronizer are nodes. SceneSaveload is an implementation of SaveloadAPI. SceneSaveloadConfig is just for configuring the saveload.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this error in the linux tests.

/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/SceneSaveload.cs(11,22): error CS0709: 'SceneSaveload': cannot derive from static class 'SaveloadApi' [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]

What even is a static class? 😅

Copy link
Member

@AThousandShips AThousandShips Jul 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SceneSaveload specifically, abstract here means limiting construction, the PhysicsServer2D/3D are abstract for example

I believe this is what breaks the mono things, worth testing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because SceneSaveload implements the abstract SaveloadAPI? I'll try it!

Thanks for all the support with the CI ❤️

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to update the documentation for @GlobalScope, see the CI, tough you'd need to match the way you name the singleton below, go with either SceneSaveload or SaveloadAPI but make sure you unregister with the same name

Edit: You will have to add documentation for the API for CI to pass

saveload_api = memnew(SceneSaveload);
GDREGISTER_ABSTRACT_CLASS(SaveloadAPI);
GDREGISTER_ABSTRACT_CLASS(SceneSaveload);
Engine::get_singleton()->add_singleton(Engine::Singleton("SceneSaveload", saveload_api));
Copy link
Member

@AThousandShips AThousandShips Jul 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Engine::get_singleton()->add_singleton(Engine::Singleton("SceneSaveload", saveload_api));
Engine::get_singleton()->add_singleton(Engine::Singleton("SaveloadAPI", SaveloadAPI::get_singleton(), "SaveloadAPI"));

The unit tests require that singletons be derived directly from Object, and you're using different names for the singletons here and when unregistering

Alternatively:

Suggested change
Engine::get_singleton()->add_singleton(Engine::Singleton("SceneSaveload", saveload_api));
Engine::get_singleton()->add_singleton(Engine::Singleton("SceneSaveload", SaveloadAPI::get_singleton(), "SaveloadAPI"));

But update the unregister

Copy link
Member

@AThousandShips AThousandShips Jul 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm testing these changes locally and running the CI on my repository just to check

Edit: It works mostly now it seems, with these changes

Though it does need a cleanup, and I'd suggest that you don't merge the upstream branch into your branch but rebase it, that makes the commit history much easier to navigate, I spent a bit of time trying to rebase it cleanly and got stuck because of the diverging history there, so wasn't able to test it right now without doing a lot of rebuilding for that reason

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks again for all the help!

I'll make those changes to unregister. What file do I need to edit to add documentation?

I'll clean up the git history soon. My first priority is getting a minimum viable product that people can try out, even if things are still a little messy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebasing it to make building more convenient is good, but remember to rebase and not merge in the future for updating the branch

You need to add documentation specifically for the module classes, try building and running --doctool

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it! Hopefully this one passes finally 🎏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it passed the .NET tests :D

The logs for editor w/ doubles confuse me. It seems to be failing on something completely unrelated to saveload.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah there's memory capacity issues on the godot-cpp build, sporadically, unrelated to this PR, is happening across different PRs currently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that explains the multiple attempts. Thanks for the info!

You can do it little builder! 🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like the CI is ready to rerun failed builds now. Feel free to give it a spin whenever you have time :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo! Passed the CI for real this time :D Thanks for the patience and persistence AThousandShips 💯

@nlupugla
Copy link
Contributor Author

@TheYellowArchitect You should now be able to test this feature by downloading the Godot exe artifact generated from the tests run on this PR.

void set_root_path(const NodePath &p_path);
NodePath get_root_path() const;

SaveloadSynchronizer(){};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SaveloadSynchronizer(){};
SaveloadSynchronizer() {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, something must be messed up with my Clang formatter. When I ran clang, it deleted that extra space, which I thought was funny considering it wanted to add spaces in other places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because you added a semicolon to it, it shouldn't have one here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see! That makes sense. Thanks :)

virtual Error save(const String &p_path, const Variant &p_configuration_data = Variant()) override;
virtual Error load(const String &p_path, const Variant &p_configuration_data = Variant()) override;

SceneSaveload(){};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SceneSaveload(){};
SceneSaveload() {}

virtual Error load(const String &p_path, const Variant &p_configuration_data = Variant()) override;

SceneSaveload(){};
~SceneSaveload(){};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
~SceneSaveload(){};
~SceneSaveload() {}

@nlupugla
Copy link
Contributor Author

nlupugla commented Sep 9, 2023

@AThousandShips When you get a chance, can you rerun the CI so I have some build artifacts for people in discord to use? Thanks!

@AThousandShips
Copy link
Member

AThousandShips commented Sep 10, 2023

The CI is outdated, you need to push again I think

@nlupugla
Copy link
Contributor Author

Okay, just pushed! Let me know if I need to do anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants