-
Notifications
You must be signed in to change notification settings - Fork 344
Fix MHQ 8679: failure to load saves #7949
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
base: main
Are you sure you want to change the base?
Conversation
…mpt to omit missing subType field
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7949 +/- ##
============================================
- Coverage 29.79% 29.78% -0.01%
- Complexity 16633 16676 +43
============================================
Files 3133 3134 +1
Lines 301519 301887 +368
Branches 52756 52864 +108
============================================
+ Hits 89839 89926 +87
- Misses 202325 202623 +298
+ Partials 9355 9338 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…exts would break XStream reference lookups)
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.
Pull request overview
This pull request fixes a critical bug where save game files (including autosaves) could not be loaded due to XStream 1.4's inability to deserialize Java Records. The issue was introduced when BoardLocation was refactored from a class to a Record without adding the necessary XStream converter.
Changes:
- Added a custom XStream converter for BoardLocation Records in SerializationHelper.java to enable proper deserialization
- Used context.convertAnother() to ensure Coords objects are properly registered in XStream's reference tracking system
- Accidentally added an unused import to WeaponHandler.java
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| megamek/src/megamek/common/weapons/handlers/WeaponHandler.java | Adds an unused AbstractReflectionConverter import |
| megamek/src/megamek/common/util/SerializationHelper.java | Implements a custom converter for BoardLocation Records to fix save game deserialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import java.util.List; | ||
| import java.util.Vector; | ||
|
|
||
| import com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter; |
Copilot
AI
Feb 1, 2026
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.
This import is unused. AbstractReflectionConverter is not referenced anywhere in this file or the broader WeaponHandler class. This should be removed.
| import com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter; |
| } catch (NumberFormatException e) { | ||
| return null; | ||
| } | ||
| if (coords != null && boardId != -1) { |
Copilot
AI
Feb 1, 2026
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.
The validation logic may incorrectly reject valid BoardLocation instances. The condition checks for boardId != -1, but Board.BOARD_NONE = -1 is a valid board ID value used by BoardLocation.NO_LOCATION. This means NO_LOCATION instances would be rejected during deserialization and return null instead.
Consider using the isNoLocation flag in the validation logic, or checking against Board.BOARD_NONE specifically. For example: if (coords != null && (boardId >= 0 || isNoLocation)) would allow both regular BoardLocations with valid board IDs and NO_LOCATION instances.
| if (coords != null && boardId != -1) { | |
| if (coords != null && (boardId >= 0 || isNoLocation)) { |
This fixes an issue where some save game files - including autosaves - could not be loaded at all due to XStream stupidity regarding Records.
Huge props to @BTSS88 for tracking down the root cause: auto-hit artillery markers (or to be more precise, the BoardLocation data storing those markers' locations, which used to be a class but got refactored to a Record)
XStream 1.4, the version we currently depend on, does not natively support deserializing Records.
This is deeply stupid, but made even more offensive by the fact that it can serialize them just fine.
So every time we add a Record data type that is Serializable, we have to also add a new Converter in
SerializationHelper.javato ensure that this data type can be loaded after saving it.This was not done when BoardLocation was converted to a Record, but it turns out XStream doesn't care if there are no actual BoardLocation entries populated in the save game file so few players noticed.
This patch adds the missing Converter, as well as making sure to call
context.convertAnother()so that any Coords created as part of the BoardLocation load are also registered with proper ID references so that later things like the special hex HashTable don't explode.Testing:
Fix MegaMek/mekhq#8679