Clear invalid spell selections when converting Hellfire saves to Diablo#8507
Open
morfidon wants to merge 9 commits intodiasurgical:masterfrom
Open
Clear invalid spell selections when converting Hellfire saves to Diablo#8507morfidon wants to merge 9 commits intodiasurgical:masterfrom
morfidon wants to merge 9 commits intodiasurgical:masterfrom
Conversation
Expose ValidatePlayer directly instead of routing load-time validation through a one-off wrapper. Add a persisted-state regression test that rewrites a Hellfire-origin save under Diablo and verifies invalid spell selections stay cleared after saving and reloading hotkeys.
Move hotkey loading out of InitPlayer so it runs only after spell sources are rebuilt. Also make LoadHotkeys sanitize the selected spell and hotkeys itself, including when the sidecar hotkeys file is missing, and resync queuedSpell in the player load paths.
Add a slot-aware LoadHotkeys overload and use it in player load paths so hero data and hotkeys are always read from the same save number. This removes the saveNum/gSaveNumber mismatch risk in pfile_read_player_from_save while keeping the existing wrapper for current-player call sites.
Introduce SyncPlayerSpellStateFromSelections and call it in hotkey load paths so queued/executed spell metadata and spellFrom are reset consistently after sanitization. This centralizes post-load spell-state normalization, removes duplicated per-call-site resync code, and adds writehero regression tests for missing hotkeys data and legacy 4-slot hotkeys format handling.
Add a regression test that loads a valid scroll selection from legacy hotkeys data and verifies the first cast path still works with normalized spellFrom state by consuming the scroll. Also replace legacy hotkeys test helper casts from int8_t to int32_t for clearer and safer intent when serializing SpellID values.
Recognize legacy hotkeys blobs by their exact payload size so the loader does not misinterpret them as the newer header-based format. Update the legacy scroll regression test to validate selection preservation against engine-backed scroll availability without depending on UI redraw side effects.
Treat only the exact legacy hotkeys blob size as the old format and validate the new-format header and payload before reading them. Keep the legacy scroll regression test aligned with actual scroll availability so the first cast path is verified after load.
- Rename LoadHotkeysLegacyFormatPreservesValidScrollAndFirstCastConsumesIt to LoadHotkeysLegacyFormatPreservesValidScrollSelection to better reflect actual test behavior - Remove pfile_read_player_from_save_preserves_valid_spell_selections test as it's covered by other tests and is nice-to-have rather than critical - Keep core logic tests: IsPlayerSpellSelectionValidChecksSpellSources, IsPlayerSpellSelectionValidRejectsInvalidSelections - Keep important regression/integration tests: pfile_read_player_from_save_clears_invalid_spell_selections, LoadHotkeysLegacyFormatSanitizesInvalidSelections, DiabloRewritePersistsSanitizedSpellSelectionsFromHellfireSave
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3466
Summary
When a Hellfire save is converted to Diablo while a Hellfire-only spell is selected, that selection can survive in an invalid state.
In DevilutionX this can leave the player with a selected spell that has no icon but may still remain usable. In vanilla Diablo, loading such a save can crash.
This change clears invalid spell selections during load/conversion and keeps the player's spell state consistent afterwards.
What changed
ValidatePlayer()before recalculating inventory-derived spell state during loadCalcPlrInv()during loading so spell sources from inventory, scrolls, and charges are rebuilt before validating selectionsSpellType::Spellis no longer treated as automatically validLoadHotkeys()explicitly handle both legacy hotkey data and missing or malformed hotkey blobs safelyWhy this approach
This issue is triggered by Hellfire -> Diablo conversion, but the real problem is broader than a single Hellfire spell id surviving the rewrite.
After conversion, the selected spell and hotkeys may no longer be valid for the rebuilt player state. Because of that, this patch does not special-case only Hellfire spell ids. Instead, it validates spell selections against the player's actual available spell sources after loading:
This follows the intent discussed in #3466:
LoadHotkeys()The extra runtime-state normalization is intentional. Clearing only
_pRSpell/_pRSplTypewould still leave stalequeuedSpell,executedSpell, orspellFromstate behind, which could keep the player in a partially invalid state after sanitization.Updated load/conversion flow
Now, during Hellfire -> Diablo conversion:
ValidatePlayer()removes invalid Hellfire-only learned spell stateCalcPlrInv()rebuilds spell availability from inventory, including scrolls and chargesThis ensures that invalid Hellfire spell selections do not survive conversion as selected, queued, or executed spell state.
Tests
Added tests to cover: