refactor: Clean up world saving/loading codepath (Phase 1 of v2 save format) #5774

randombk · 2024-11-29T04:48:53Z

Checklist

Required

I wrote the PR title in conventional commit format.
I ran the code formatter. => Awaiting CI bot
I linked any relevant issues using github keyword syntax so it can be closed automatically.
I have committed my changes to new branch that isn't main so it won't cause conflict when updating main branch later.

Purpose of change

This PR is Phase 1 of #5771.

It refactors much of the world and player save read/write logic, with a particular focus on consolidating the business logic of what gets saved where. The purpose is to create a clean environment where we can add on support for different save formats in a subsequent PR.

This PR should not change the on-disk format. Everything should continue to work normally. The actual SQLite+compression implementation will happen on top of this refactored backend in a later PR.

Describe the solution

Clean up and refactor worldfactory and world.
- The old WORLD struct is now renamed WORLDINFO to better reflect its actual purpose.
- Get rid of the WORLDPTR = WORLD* convention. This isn't the Windows API, and nothing else in the codebase does this.
Move file saving logic into a new class called world, meant to encapsulate the idea of a loaded world. All reads and writes to world and player files now go through this class, which forms the foundation for future changes to the save format.
Simplified the I/O utility methods read_from_file* and write_to_file* found in cata_utility.cpp. This replaces the 2 write and 6 read methods with 1 write and 2 read. This simplification is needed because future save loaders will need to provide their own implementations of these methods.
- The _optional variants can be replaced with default arguments.
- The JsonDeserializer variants are never used.
- The _json variants do save some boilerplate (path) so I'll keep that in for now.

Describe alternatives you've considered

Testing

Awaiting CI signals.
I tested by loading & saving a couple of existing worlds, without obvious issues thus far.

Additional context

All the other binaries are listed except for the ones that cmake actually generates for tiles builds. Signed-off-by: David Li <jiawei.davidli@gmail.com>

Clean up and refactor worldfactory and world. * The old 'WORLD' struct is now renamed `WORLDINFO` to better reflect its actual purpose. * Get rid of the WORLDPTR = WORLD* convention. This isn't the Windows API, and nothing else does this. * Move file saving logic into a new class called `world`, meant to encapsulate the idea of a loaded world. I will begin consolidating more world loading logic into this class. Signed-off-by: David Li <jiawei.davidli@gmail.com>

Replace the 2 write and 6 read methods with 1 write and 2 read. This simplification is needed because future save loaders will need to provide their own implementations of these methods. * The `_optional` variants can be replaced with default arguments. * The `JsonDeserializer` variants are never used. * The `_json` variants do save some boilerplate (`path`) so I'll keep that in for now. Signed-off-by: David Li <jiawei.davidli@gmail.com>

Player file IO is still janky, particularly `mm` files - WIP. However, the game does work and the on-disk format should be unchanged. Signed-off-by: David Li <jiawei.davidli@gmail.com>

Clean up how the game determines where to place player- specific files, in preparation for the V2 save format. NOTE: This removes backwards compatibility for the pre-2020 map memory ('mm') format. This logic only triggered when loading a map that has not seen a save event since 2020 - I think it's safe to remove this now. Signed-off-by: David Li <jiawei.davidli@gmail.com>

autofix-ci · 2024-11-29T04:49:47Z

Autofix has formatted code style violation in this PR.

I edit commits locally (e.g: git, github desktop) and want to keep autofix

Run git pull. this will merge the automated commit into your local copy of the PR branch.
Continue working.

I do not want the automated commit

Format your code locally, then commit it.
Run git push --force to force push your branch. This will overwrite the automated commit on remote with your local one.
Continue working.

If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT.

scarf005 · 2024-11-29T04:52:15Z

gah astyle is going haywire

randombk · 2024-11-29T04:53:35Z

No, astyle is largely correct here. I'll pull and squash. I don't have a proper astyle setup locally so I'm just depending on the bot for now.

randombk · 2024-11-29T05:01:16Z

Actually, if there's no objections I'll the keep asyle commit without squashing - it leaves the individual diffs nice and self-consistent.

Signed-off-by: David Li <jiawei.davidli@gmail.com>

The consolidation of write_to_file methods introduced a subtle API change where passing nullptr to the third argument no longer caused write_to_file to eat exceptions. This largely only affected tests and a small number of edge cases. Alter the API so that the right way to eat exceptions is via passing in the empty string as the message. Signed-off-by: David Li <jiawei.davidli@gmail.com>

Plus misc bug fixes Signed-off-by: David Li <jiawei.davidli@gmail.com>

scarf005

the changes looks good to me, there's some nitpicks here and there but not critical.

@chaosvolt @RoyalFox2140 could you playtest them before merging to be extra sure?

scarf005 · 2024-12-01T14:30:53Z

src/fstream_utils.h

+using file_read_cb = const std::function<void( std::istream & )> &;
+using file_read_json_cb = const std::function<void( JsonIn & )> &;
+using file_write_cb = const std::function<void( std::ostream & )> &;


I think fn(function) would be clearer than cb(callback).

scarf005 · 2024-12-01T14:45:10Z

src/cata_utility.cpp

@@ -523,8 +530,12 @@ std::istream &safe_getline( std::istream &ins, std::string &str )
    }
 }

-bool read_from_file( const std::string &path, const std::function<void( std::istream & )> &reader )
+bool read_from_file( const std::string &path, file_read_cb reader, bool optional )


might be clearer to use enum class over boolean flag, e.g

enum class read_option { REQUIRED = 0, OPTIONAL = 1 };

scarf005 · 2024-12-01T14:51:58Z

src/game.cpp

+bool game::take_screenshot( const std::string &path )
 {
    return save_screenshot( path );
 }

-bool game::take_screenshot() const
+bool game::take_screenshot()


could you explain why was the const removed? does the function internals now mutate states when taking screenshots?

scarf005 · 2024-12-01T14:52:42Z

src/world.h

+         */
+        /**@{*/
+        void start_save_tx();
+        long long commit_save_tx();


might be clearer to use int64_t.

joveeater

Looks good to me. Great stuff.

Coolthulhu · 2024-12-01T16:31:18Z

src/game.cpp

@@ -2732,16 +2727,23 @@ spell_events &game::spell_events_subscriber()
    return *spell_events_ptr;
 }

-static bool save_uistate_data( const game &g )
+bool game::save_uistate_data()


Does it affect the state? Should be const if it doesn't

Coolthulhu · 2024-12-01T16:32:36Z

src/game.cpp

@@ -11413,12 +11418,12 @@ void game::quicksave()

 void game::quickload()
 {
-    const WORLDPTR active_world = world_generator->active_world;
+    world *active_world = get_active_world();


Does it have to be non-const?

Coolthulhu · 2024-12-01T16:35:47Z

src/world.cpp

+    // if( SQLITE_OK != ( ret = sqlite3_exec( db, sql, NULL, NULL, &sqlErrMsg ) ) ) {
+    //     dbg( DL::Error ) << "Failed to init db" << db_path << " (" << sqlErrMsg << ")";
+    //     throw std::runtime_error( "Failed to open db" );
+    // }


Commented out code - should be removed from the PR. It's easy to restore it from git history later.

Coolthulhu · 2024-12-01T16:39:43Z

src/cata_utility.cpp

@@ -523,8 +530,12 @@ std::istream &safe_getline( std::istream &ins, std::string &str )
    }
 }

-bool read_from_file( const std::string &path, const std::function<void( std::istream & )> &reader )
+bool read_from_file( const std::string &path, file_read_cb reader, bool optional )


Yeah, that bool is hard to understand there. Especially after function definition.
If not an enum flag, the following options would also work:

Split into 2 functions: mandatory and optional variant

Empty class used just as argument here, for overload

chaosvolt · 2024-12-01T18:16:39Z

I'd like to see if the various code nitpicks can be addressed before I test it. Are any of them deal-breakers or are they all just minor things that can be ignored if nessecary?

scarf005 · 2024-12-01T18:18:14Z

I'd like to see if the various code nitpicks can be addressed before I test it. Are any of them deal-breakers or are they all just minor things that can be ignored if nessecary?

they are pure C++ nitpicks and won't affect gameplay.

chaosvolt · 2024-12-01T18:20:19Z

In that case will see about testing it in a bit, later today if getting ready to head out and deal with errands doesn't leave be with enough time.

chaosvolt

Compile-tested.
Loaded a save from a previous compile-test, no errors seen.
Started a new world, saved and loaded, also worked fine.
Started a new character in this new world with one already in it, saving and loading the second character also works.

randombk · 2024-12-01T23:52:25Z

Addressed comments. The consts were a leftover from a previous iteration - I added them back in where appropriate.

The player data write methods cannot be const to make way for the Phase 2 patch. Because the saveid is not known when the world object is created, we must load the player sqlite file on-demand thus requiring some internal mutable state during player writes.

scarf005 · 2024-12-02T00:25:25Z

strange, MSVC is chocking on here

D:\a\Cataclysm-BN\Cataclysm-BN\src\game.cpp(2510,63): error C2589: ')': illegal token on right side of '::' [D:\a\Cataclysm-BN\Cataclysm-BN\out\build\windows-tiles-sounds-x64-msvc\src\cataclysm-bn-tiles-common.vcxproj]
D:\a\Cataclysm-BN\Cataclysm-BN\src\game.cpp(2510): error C2062: type 'unknown-type' unexpected [D:\a\Cataclysm-BN\Cataclysm-BN\out\build\windows-tiles-sounds-x64-msvc\src\cataclysm-bn-tiles-common.vcxproj]
D:\a\Cataclysm-BN\Cataclysm-BN\src\game.cpp(2564,117): error C2589: ')': illegal token on right side of '::' [D:\a\Cataclysm-BN\Cataclysm-BN\out\build\windows-tiles-sounds-x64-msvc\src\cataclysm-bn-tiles-common.vcxproj]
D:\a\Cataclysm-BN\Cataclysm-BN\src\game.cpp(2564): error C2062: type 'unknown-type' unexpected [D:\a\Cataclysm-BN\Cataclysm-BN\out\build\windows-tiles-sounds-x64-msvc\src\cataclysm-bn-tiles-common.vcxproj]
D:\a\Cataclysm-BN\Cataclysm-BN\src\game.cpp(2590,30): error C2589: ')': illegal token on right side of '::' [D:\a\Cataclysm-BN\Cataclysm-BN\out\build\windows-tiles-sounds-x64-msvc\src\cataclysm-bn-tiles-common.vcxproj]
D:\a\Cataclysm-BN\Cataclysm-BN\src\game.cpp(2590): error C2062: type 'unknown-type' unexpected [D:\a\Cataclysm-BN\Cataclysm-BN\out\build\windows-tiles-sounds-x64-msvc\src\cataclysm-bn-tiles-common.vcxproj]

randombk · 2024-12-02T03:31:08Z

Yea, there's something about enum syntax that MSVC really doesn't like.

scarf005 · 2024-12-02T03:36:42Z

weird, other enum classes compile fine.

RoyalFox2140 · 2024-12-02T10:14:31Z

the changes looks good to me, there's some nitpicks here and there but not critical.

@chaosvolt @RoyalFox2140 could you playtest them before merging to be extra sure?

If the saves are going to break they're going to break after days of playing the same save file for hours a day, And I can't play self-compile versions. No matter what settings are used my game lags heavily and we've gone over using Release without Debug symbols doing nothing to fix it, and proper releases not lagging. There's nothing for me to do here. If it's going to break everything it's going to do so in a way that won't get caught from standard testing.

MSVC and Ubuntu tests failed, so I'll at least re-run those.

Signed-off-by: David Li <jiawei.davidli@gmail.com>

randombk · 2024-12-03T05:04:13Z

I changed the enum back to a bool. There's something weird going on with MSVC and I don't have a local environment to test/debug that right now.

chaosvolt

Compiled and load-tested.
Loaded a save from previous compiling, loads as expected. Loading the two different characters in the save also work right.
Tested save/load, no oddities found.
Also quicksaved, debug quit to menu, then loaded with no issues.
Spawned in an APC and used autodrive to move a long distance before quicksaving, also worked.

Quicksave just standing in starting shelter:

Quicksave after a nice long drive (about 75 thousand blocks of things to save, whatever the number during save progress is counting):

randombk added 5 commits November 28, 2024 00:41

fix: Add 'cataclysm-bn-tiles' and 'json_formatter' to .gitignore

64e860d

All the other binaries are listed except for the ones that cmake actually generates for tiles builds. Signed-off-by: David Li <jiawei.davidli@gmail.com>

refactor: Consolidate more world loading methods into world

6414d3c

Player file IO is still janky, particularly `mm` files - WIP. However, the game does work and the on-disk format should be unchanged. Signed-off-by: David Li <jiawei.davidli@gmail.com>

github-actions bot added src changes related to source code. tests changes related to tests labels Nov 29, 2024

style(autofix.ci): automated formatting

21332e3

fix: Forgot to migrate some code in non-tiles build

0c7c648

Signed-off-by: David Li <jiawei.davidli@gmail.com>

randombk mentioned this pull request Nov 29, 2024

Early Discussion: New save format to reduce save size and number of files #5771

Open

randombk added 2 commits November 28, 2024 21:59

fix: Bug in initializing world dir structure

3a7edc3

Signed-off-by: David Li <jiawei.davidli@gmail.com>

randombk force-pushed the sqlite-saves-phase1 branch 2 times, most recently from 344b84e to 1229bfa Compare December 1, 2024 02:40

refactor: Remember save format in the WORLDINFO struct

e6dd533

Plus misc bug fixes Signed-off-by: David Li <jiawei.davidli@gmail.com>

randombk force-pushed the sqlite-saves-phase1 branch from 1229bfa to e6dd533 Compare December 1, 2024 03:16

randombk mentioned this pull request Dec 1, 2024

feat: SQLite+Compressed save format (Phase 2 of v2 save format) #5777

Open

4 tasks

scarf005 requested a review from chaosvolt December 1, 2024 14:27

scarf005 previously approved these changes Dec 1, 2024

View reviewed changes

scarf005 requested review from Coolthulhu and joveeater December 1, 2024 15:26

joveeater previously approved these changes Dec 1, 2024

View reviewed changes

Coolthulhu reviewed Dec 1, 2024

View reviewed changes

chaosvolt previously approved these changes Dec 1, 2024

View reviewed changes

randombk dismissed stale reviews from chaosvolt, joveeater, and scarf005 via c48032f December 1, 2024 23:48

randombk force-pushed the sqlite-saves-phase1 branch from 8cb8210 to 1f4fa77 Compare December 1, 2024 23:50

randombk force-pushed the sqlite-saves-phase1 branch 3 times, most recently from d9ebad7 to 6cfa61d Compare December 2, 2024 02:29

randombk force-pushed the sqlite-saves-phase1 branch from 6cfa61d to 45e1ba9 Compare December 2, 2024 03:33

randombk force-pushed the sqlite-saves-phase1 branch 2 times, most recently from 45e1ba9 to 0f66a62 Compare December 2, 2024 06:02

refactor: Address code style and const in save refactor

b4c55df

Signed-off-by: David Li <jiawei.davidli@gmail.com>

randombk force-pushed the sqlite-saves-phase1 branch from 0f66a62 to b4c55df Compare December 3, 2024 05:02

chaosvolt approved these changes Dec 7, 2024

View reviewed changes

chaosvolt merged commit 374bd34 into cataclysmbnteam:main Dec 7, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Clean up world saving/loading codepath (Phase 1 of v2 save format) #5774

refactor: Clean up world saving/loading codepath (Phase 1 of v2 save format) #5774

randombk commented Nov 29, 2024 •

edited

Loading

autofix-ci bot commented Nov 29, 2024

scarf005 commented Nov 29, 2024

randombk commented Nov 29, 2024

randombk commented Nov 29, 2024 •

edited

Loading

scarf005 left a comment

scarf005 Dec 1, 2024

scarf005 Dec 1, 2024

scarf005 Dec 1, 2024 •

edited

Loading

scarf005 Dec 1, 2024

joveeater left a comment

Coolthulhu Dec 1, 2024

Coolthulhu Dec 1, 2024

Coolthulhu Dec 1, 2024

Coolthulhu Dec 1, 2024

chaosvolt commented Dec 1, 2024

scarf005 commented Dec 1, 2024

chaosvolt commented Dec 1, 2024

chaosvolt left a comment

randombk commented Dec 1, 2024

scarf005 commented Dec 2, 2024

randombk commented Dec 2, 2024

scarf005 commented Dec 2, 2024

RoyalFox2140 commented Dec 2, 2024

randombk commented Dec 3, 2024

chaosvolt left a comment

refactor: Clean up world saving/loading codepath (Phase 1 of v2 save format) #5774

refactor: Clean up world saving/loading codepath (Phase 1 of v2 save format) #5774

Conversation

randombk commented Nov 29, 2024 • edited Loading

Checklist

Required

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

autofix-ci bot commented Nov 29, 2024

scarf005 commented Nov 29, 2024

randombk commented Nov 29, 2024

randombk commented Nov 29, 2024 • edited Loading

scarf005 left a comment

Choose a reason for hiding this comment

scarf005 Dec 1, 2024

Choose a reason for hiding this comment

scarf005 Dec 1, 2024

Choose a reason for hiding this comment

scarf005 Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

scarf005 Dec 1, 2024

Choose a reason for hiding this comment

joveeater left a comment

Choose a reason for hiding this comment

Coolthulhu Dec 1, 2024

Choose a reason for hiding this comment

Coolthulhu Dec 1, 2024

Choose a reason for hiding this comment

Coolthulhu Dec 1, 2024

Choose a reason for hiding this comment

Coolthulhu Dec 1, 2024

Choose a reason for hiding this comment

chaosvolt commented Dec 1, 2024

scarf005 commented Dec 1, 2024

chaosvolt commented Dec 1, 2024

chaosvolt left a comment

Choose a reason for hiding this comment

randombk commented Dec 1, 2024

scarf005 commented Dec 2, 2024

randombk commented Dec 2, 2024

scarf005 commented Dec 2, 2024

RoyalFox2140 commented Dec 2, 2024

randombk commented Dec 3, 2024

chaosvolt left a comment

Choose a reason for hiding this comment

randombk commented Nov 29, 2024 •

edited

Loading

randombk commented Nov 29, 2024 •

edited

Loading

scarf005 Dec 1, 2024 •

edited

Loading