Add optional autosave system with safe retry logic for Single Player#8497
Add optional autosave system with safe retry logic for Single Player#8497morfidon wants to merge 20 commits intodiasurgical:masterfrom
Conversation
Implement an autosave subsystem and safer save handling, plus related UI and hooks. - Add autosave state and logic (diablo.cpp): periodic timer, pending queue, priorities (Timer, TownEntry, BossKill, UniquePickup), cooldowns, combat cooldowns, enemy proximity safety checks, and helper APIs (QueueAutoSave, AttemptAutoSave, IsAutoSaveSafe, etc.). - Integrate autosave triggers: queue on town entry (loadsave), unique item pickup (inv.cpp), boss kills (monster.cpp), and mark combat activity from player actions and hits (player.cpp). - Add gameplay options to enable autosave and set interval (options.h/.cpp) and display countdown/ready label in the game menu (gamemenu.cpp/gmenu.cpp). Menu text retrieval updated to show remaining seconds or "ready". - Make SaveGame robust (loadsave.cpp): write hero and stash via new pfile_write_hero_with_backup() and pfile_write_stash_with_backup() that create backups and restore on failure. Add utilities to copy/restore unpacked save directories safely (pfile.cpp) and adjust stash path handling signature. - Minor fixes and cleanups: restrict mouse-motion handling to KeyboardAndMouse path, small reordering in player sprite width switch, and a few safety/formatting tweaks. Autosave only runs in single-player and when IsAutoSaveSafe() conditions are met. Backup save logic attempts to preserve the previous save on failure.
- Add RequestAutoSave() wrapper function with centralized filter logic - Add HasPendingAutoSave() helper for better code readability - Replace direct QueueAutoSave() calls with RequestAutoSave() - Consolidate multiplayer and enabled checks in one place - Improve code maintainability and separation of concerns
Sample the timestamp once after SaveGame and use it to set cooldown and next-timer values to avoid inconsistent timings from multiple SDL_GetTicks calls. Calculate the saved-game message duration based on elapsed time and clamp it to a minimum of 500ms so the confirmation is visible. Also change GetSaveGameMenuLabel to return std::string_view and return the saved label directly instead of a c_str(), simplifying lifetime handling.
|
tbh I'm not a fan - looks overengineered to me xD single player is "derp I can't play" mode anyway so no need to add any safeguards - if you get stuck, that's on you, I'd get rid of all things and just have it perform a regular save every X time |
I get the concern about overengineering. The reason I added the safeguards is that autosave without them can easily corrupt the save if the game crashes during write. That's especially painful in single player where the save is the only state. Diablo historically didn't have autosave and losing progress after a crash was a pretty common complaint. And also they didn't do it just because of the fact that it could lead to corruption. The goal here wasn't to make the system complex but to avoid partial writes or broken saves if autosave happens at a bad moment :) Now anybody can have fun in Single Player - the good thing is that you can switch it on/off. |
Having more options is not always good. Particularly for things that directly impact gameplay/balance. |
Autosave here doesn't change the gameplay loop or player power. It only protects against corrupted saves if the game crashes during write. Diablo already allows saving at any moment by exiting the game, so this doesn't introduce a new gameplay advantage. The safeguards are purely about data integrity. |
Disagree. Unless you implemented this in such a way as to preserve a separate "normal" save slot from the "autosave" slot (and have not mentioned it in the description). Autosave makes the game more similar to D2 where if you make a mistake you can't "undo" it by loading the game. So it definitely impacts how the game plays. Or is this what you meant by:
And if that's the case, how are you handling picking between the "autosave" slot and the "normal" slot? "Normal" slot is only loaded on explicit I also don't see a need to expose such things as "the autosave timer". That just makes the settings menu more convoluted for no reason. There is never a reason for a player to need to configure such an internal detail IMHO. The player should also not need to know what the timer is, so this also applies to exposing it on the main menu. |
That's fair - I was using "gameplay/balance" too loosely. What I meant is that the crash-safety safeguards themselves are about data integrity, not game balance. But you're right that autosave can still affect how the game plays if it changes the player's ability to roll back mistakes. My goal here was not to take away the existing ability to recover from a bad decision, only to make automatic saving safer and avoid broken saves from partial writes. That's why you can also change interval timer from 30s up to 360s - then the player can decide if he wants more often saves or just in case auto saves. I agree the exposed timer is probably too much. I'm fine with simplifying that to a player-facing autosave on/off option with a fixed interval instead of exposing internal timing details. |
Rename save backup and restore helpers to use location terminology, pass source and target locations explicitly, and reuse the copy helper for stash backups.
Fall back to the regular save flow when UNPACKED_SAVES builds have no filesystem support, compile backup-copy restore paths only when filesystem support is available, and keep demo helpers as documented stubs in that configuration.
|
Fixed the failing builds (I hope) - can't run them without approval. The issue was that the new backup copy/restore path relies on filesystem support, but some CI targets build with For those targets I now fall back to the regular save path instead of using the backup-based save flow, so supported targets keep the safer path while the no-filesystem targets still build correctly. |
Narrow the backup save helper to full game saves only. Replace pfile_write_hero_with_backup(bool writeGameData) with pfile_write_game_with_backup(), and always write full game data before validating the resulting archive with ArchiveContainsGame(). This makes the helper's name, behavior, and post-write validation consistent, and removes an API parameter that made the function look more general than its actual responsibility. Also update SaveGame() to use the new helper name and a clearer local variable.
Keep autosave interval controls available only in debug builds and remove autosave timing details from the release save menu.
Add SDL_TICKS_PASSED fallback definitions in the shared SDL compatibility headers so SDL1-family targets do not fail when the macro is missing from the platform SDL headers.
Split the save flow into explicit manual and auto save kinds and return a SaveResult instead of overloading gbValidSaveFile as the outcome of the last save attempt. Redesign backup handling so the persistent backup slot represents the last manual save, while autosave uses its own temporary restore copy and no longer overwrites the manual backup. Manual save UI now reports failure separately from preserved-save recovery, and autosave only reports success when the new save actually succeeds.
Run the repository-pinned .\\.tools\\clang-format-18\\clang-format.exe on loadsave.cpp and verify the change with the required Debug devilutionx build. This records the formatting step with the project-mandated formatter after the save failure fix.
Treat a missing stash archive as an acceptable state when classifying save failures. This preserves the previous game save result instead of reporting no valid save when the player never created a stash.
Autosave used SDL_TICKS_PASSED in diablo.cpp, but that macro is not available in SDL3, which broke the Linux SDL3 CI build. Switch the autosave timer state to the SDL_GetTicks return type and compare deadlines through a small local helper. The helper keeps SDL_TICKS_PASSED semantics on SDL1/SDL2 while using a direct comparison on SDL3, so this fixes the SDL3 compile error without broad timer refactoring in other code paths.
|
Fixed linux build in c1a6b61 |
4 participants
I've always hated how saving in single player in Diablo was handled, especially because I have ADHD and I lost so many times progress because of forgetting to save manually ^^
Diablo currently saves only on explicit save events.
If the game exits unexpectedly, the player can lose a noticeable amount of progress.
This PR introduces safe and optional autosave system for Single Player (MP not touched).
Design goals
The system was designed with the following goals in mind:
The goal is to reduce progress loss while keeping autosave unobtrusive and safe for the game state.
What this PR adds
Autosave is disabled by default and can be enabled in Gameplay settings.
Autosave triggers
Autosave requests can be generated by:
Safety conditions
Autosave only runs when the game is in a safe state:
Implementation notes
Autosave requests are queued and executed during the game loop when the state is safe.
Additional protections:
UI
When autosave is enabled, the Save Game menu entry shows a small countdown until the next autosave.
Example UI behavior:
Settings:
Manual testing
Tested in game with the following scenarios:
Why autosave is deferred
Autosave events only queue a save request.
The actual save operation is executed later from the main game loop once the game reaches a safe state.
This avoids saving during:
Compatibility
Autosave is fully optional and disabled by default.
Existing save files are not modified or migrated.
Scope
This PR keeps the scope intentionally limited to core autosave triggers.
Additional triggers (quests, level transitions, etc.) could be added later if desired.
Feedback and suggestions are welcome.
If this approach looks reasonable I can extend autosave triggers in a follow-up PR.
@qndel I’d appreciate your feedback on the autosave logic if you have time.