-
Notifications
You must be signed in to change notification settings - Fork 285
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
feat: SQLite+Compressed save format (Phase 2 of v2 save format) #5777
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.
15ef66f
to
c500571
Compare
also related: #3150 |
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 # Conflicts: # msvc-full-features/vcpkg.json
Signed-off-by: David Li <jiawei.davidli@gmail.com>
c500571
to
b61ff6f
Compare
Signed-off-by: David Li <jiawei.davidli@gmail.com>
This now builds across all platforms, and I've tested that the Linux, Windows (MSVC), and Android binaries work and can save/load successfully. The MXE build is still broken and will likely remain so until BrettDong/MXE-GCC#1 is resolved, assuming we want to keep MXE in the first place. A batch of working binaries can be found at https://github.com/randombk/Cataclysm-BN/releases/tag/v0.6.0 |
Oh, very good thing we fixed the MSVC builds then. |
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.
let's goooooooo
(but @chaosvolt please playtest just one more time before we ship it)
(also we should probably make a major release first)
(also we should make an announcement a week before merging it first)
Re-running tests due to that error that kept popping up, but will test in a bit. |
That Clang-tidy check has been failing on a bunch of PRs recently. |
I might go ahead and update the PR from upstream before I test and that should fix it since I think we tackled this in a recent PR, but that will cancel Scarf's review. ;~; |
EDIT: I feex, just had to update vcpkg, compile-testing now. |
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.
- Compiled and load-tested.
- Started up an empty world created before compiling.
- Saved and loaded, no odd behaviors found.
- Started a save in build
2025-01-07
then transferred it over. - Loading the save works without issue, saving the old world in the new build also works fine.
- Hit convert to V2 format, Avast freaked the fuck out at me but conversion went as expected after I told it to fuck off.
This is uh...confusing though, and you can convert a save to V2 even after it's already been converted? Uhhh...
EDIT: Ah I see, you can just spam infinite backups by trying to convert the V1 backups made from converting, lel.
it's due to esm.sh not returning same file, this might be related: esm-dev/esm.sh#614 EDIT: how strange, it works on my machine
|
Ask the user to rename the backup world before they attempt another conversion. Signed-off-by: David Li <jiawei.davidli@gmail.com>
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.
let's goooooooooo
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.
Item
Identity.
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