Split Adventure save compatibility handling into separate methods#7799
Closed
Jetz72 wants to merge 17 commits into
Closed
Split Adventure save compatibility handling into separate methods#7799Jetz72 wants to merge 17 commits into
Jetz72 wants to merge 17 commits into
Conversation
Allow cards to be removed from auto-sell even if they aren't sellable in the first place for some reason.
# Conflicts: # forge-gui-mobile/src/forge/adventure/player/AdventurePlayer.java
Jetz72
commented
Jun 12, 2025
# Conflicts: # forge-gui-mobile/src/forge/adventure/player/AdventurePlayer.java
|
This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed |
# Conflicts: # forge-core/src/main/java/forge/util/ItemPool.java # forge-gui-mobile/src/forge/adventure/player/AdventurePlayer.java # forge-gui-mobile/src/forge/adventure/scene/AdventureDeckEditor.java # forge-gui-mobile/src/forge/adventure/world/WorldSave.java
Contributor
Author
|
@kevlahnota I've had this PR for a while to implement versioning and migration handling for Adventure Saves. Do you think it makes sense to roll your inventory item migration logic into something like this? |
|
This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed |
# Conflicts: # forge-gui-mobile/src/forge/adventure/player/AdventurePlayer.java
Jetz72
changed the title
Jetz72
added
Serialization
|
This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed |
|
This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed |
|
This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed |
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
1 participant
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.
Renamed this issue because A) I think the original issue may have been fixed incidentally by the deck editor rewrite, and B) because the save compat stuff has grown to be the main feature of this PR.
This adds version numbers for adventure saves, and some methods that are applied when loading a save from an earlier version than Forge expects. This lets us split a bunch of special case backwards compatibility logic out of the main
AdventurePlayer.load()method. In the future, any time we make a breaking change to the format of an Adventure Save, we can incrementWorldSave.ADVENTURE_SAVE_VERSIONand add the migration logic to the appropriate methods.Original issue text below:
Attempts to resolve the issue discussed here. A user posted a file where A) They owned a deck containing over a thousand cards that weren't actually in their collection, and B) several cards in their auto-sell list also were not in their collection. It's not certain how this happened, but my working theory is that the cards were in both a deck and the auto-sell list, got auto-sold, and weren't removed from the deck as part of this process (because there's no logic to do so).
This PR attempts to solve the issue a bit upstream from the actual issue by preventing cards in auto-sell from being moved directly into decks. I'll see about doing a more direct fix to the sell logic in #7519.
This PR also adds some logic to clean up this issue from existing saves. Any cards that aren't in the collection will be removed from the auto-sell list (factoring in quantities appropriately). Any cards that aren't in the collection but are in decks will be added to the collection. I was going to just remove these from the decks as well, but that would likely cause substantial confusion for other users in this situation. We can change this in the future if this turns into the source of a card duplication bug.
Partially to that end, I've added a value
ADVENTURE_SAVE_VERSIONfor adventure saves to track. Doesn't do much for now (And I haven't looked into whether the new dynamic deck count logic's legacy path can merge into it), but in the future we can increment it any time we need to modify the save system, and then check it to determine when migration logic should be applied. I've also added a field that stores the Forge version the file was saved with, for reference when debugging saves in the future.