-
Notifications
You must be signed in to change notification settings - Fork 281
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
feat: SQLite+Compressed save format (Phase 2 of v2 save format) #5777
base: main
Are you sure you want to change the base?
Conversation
Autofix has formatted code style violation in this PR. I edit commits locally (e.g: git, github desktop) and want to keep autofix
I do not want the automated commit
If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT. |
d2b918c
to
f6aa348
Compare
while out of scope for this PR, I'm curious whether it's possible to
sequenceDiagram
Note left of BN: log "saving..."
BN-)saveThread: send save request
activate saveThread
alt on failure
saveThread--)BN: log "failed saving"
else compress save
saveThread--)BN: log "save success"
end
deactivate saveThread
|
I'm going to tweak this to make this opt-in during world creation (+ the convert-existing-world functionality). Same goes for tweaking compression algo. The database has the metadata to change compression or toggle it on/off even at the map tile grain, but it's more the config UI and adding in so much complexity into something that most players shouldn't have to worry about. |
makes sense, to most players keeping their saves not being corrupt would be much more important than saving 100ms. |
f6aa348
to
b67cb16
Compare
95874ef
to
e657e38
Compare
e657e38
to
7342db9
Compare
Additional TODOs: add in the required libraries into the CI environment, and update the relevant docs & wikis to document the V2 system and the new build dependencies. |
a8e32d0
to
72e7c4b
Compare
41e0e6e
to
54e115b
Compare
Any idea how to trigger the MSVC and Android checks? I'm not sure why the CI skipped those |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, except for the compiler warnings.
&compressedSize, | ||
reinterpret_cast<const Bytef *>( input.data() ), | ||
input.size(), | ||
Z_BEST_SPEED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed here, but it might be a good idea to extract it to an option later.
src/compress.cpp
Outdated
void zlib_decompress( const void *compressed_data, int compressed_size, std::string &output ) | ||
{ | ||
// We need to guess at the decompressed size - we expect things to compress fairly well. | ||
uLongf decompressedSize = compressed_size * 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An explicit cast here would be useful to silence the warning. Some compilers can be annoying about things like this.
src/world.cpp
Outdated
sqlite3 *db = nullptr; | ||
int ret; | ||
|
||
if( SQLITE_OK != ( ret = sqlite3_initialize() ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid assignments in if
, as per warning. They are harder to read, so it's best to only use them when they save a lot of space.
src/world.cpp
Outdated
} | ||
|
||
bool world::read_overmap_player_visibility( const point_abs_om &p, file_read_fn reader ) | ||
{ | ||
return read_from_player_file( overmap_player_filename( p ), reader, true ); | ||
if( info->world_save_format == save_format::V2_COMPRESSED_SQLITE3 ) { | ||
auto playerdb = get_player_db(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full typename preferred where known and not too bulky (ie. not an iterator or similar).
src/world.cpp
Outdated
} | ||
|
||
void replaceBackslashes( std::string &input ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning on missing declaration. Looks like you can make the function static
.
The `clang-tidy` CI step is breaking because it's trying to interpret all files in src/ and tests/ as CPP files. Thus when src/CMakeLists.txt is changed, it tries to parse that file as C++ and fails. Signed-off-by: David Li <jiawei.davidli@gmail.com>
Implement new SQLite-based save format. New worlds can be configured to use V2. Existing worlds can be converted from the main menu. Existing V1 worlds should load exactly the same as before until converted. Signed-off-by: David Li <jiawei.davidli@gmail.com> # Conflicts: # .github/workflows/manual-release.yml # .github/workflows/release.yml
511387c
to
15ef66f
Compare
Signed-off-by: David Li <jiawei.davidli@gmail.com>
15ef66f
to
c500571
Compare
Checklist
Required
main
so it won't cause conflict when updatingmain
branch later.Purpose of change
This PR is Phase 2 of #5771, and supersedes #5772.
This patch implements a new V2 save layout based on SQLite3 and gzip.
Describe the solution
The new save format consolidates and compresses certain high-volume files into a set of sqlite3 database files with gzip compression. Specifically,
map.sqlite3
replaces:maps/
directory (map tile info)o.*
files (overmap info)<player_id>.sqlite3
replaces:<player_id>.seen.*
files (player overmap visibility data)<player_id>.mm1/
directory (player map tile memory)All other files will remain unchanged. This should cover all the worst offenders while leaving the more manual-edit-friendly files untouched.
Each database consists of a single table:
Benefits:
Costs:
There is an interesting trade-off between file size and save performance. Even in the old format, save/load was not a particularly big issue. Only changed chunks were modified, and autosave triggered often enough that most save events only covered a small number of chunks.
The new format does introduce an additional delay to the save time, though events remained sub-second. All of this delay can be attributed to the compression process - turning off compression resulted in no measurable changes in save performance. This is to be expected - we're introducing an extra CPU-intensive step into a single-threaded save process.
Benchmarks & Comparisons
The below are some rough figures I captured during my testing. All of these were done on a
Ryzen 9 7900X
system runningArch Linux
with the save directory located on abtrfs
partition stored on a NVME SSD.Some figures using a small test save (after ~3hrs gameplay):
Using a medium-sized test save:
Figures were taken after saving at one end of a big town, taking a helicopter across the town to generate new tiles, then saving again.
(Not sure why the bigger map was a faster save; could be multiple things, but the point is that there's a small increase)
Gzip compression level 1 ("fast") seems to be optimal. I suspect a more modern compression scheme like
zstd
would be even better, but we'll need to look into the packaging story.Describe alternatives you've considered
Testing
Additional context