-
Notifications
You must be signed in to change notification settings - Fork 135
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
Fix 1631: Armor rating incorrectly persisted #1650
Conversation
… armor tech rating
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1650 +/- ##
===========================================
- Coverage 2.17% 2.16% -0.01%
Complexity 209 209
===========================================
Files 266 266
Lines 30801 30834 +33
Branches 5266 5276 +10
===========================================
Hits 669 669
- Misses 29975 30008 +33
Partials 157 157 ☔ View full report in Codecov by Sentry. |
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 doesn't really fix the issue. The bug can occur even when armorTechRatingChanged is never called (see: #1631 (comment)).
It's also wrong to setArmorTechLevel to a tech rating constant, the two are different. I think you got lucky in testing and happened to set the tech level to a value that makes broken code elsewhere behave right.
When I arrive home I will check that, but it does seen to be doing what is expected, since this is what the TestSupportVehicle exact weight calculator, so I may have been mislead for taking it as source of truth. |
I think a good place to start looking would be to load a support vehicle and immediately encode it to BLK, before anything in MML can happen to it, and see if there's a difference. If there is, then the bug is in BLK encoding and decoding (although there might still be something in MML causing related secondary bugs). |
I did exactly that, but I missed one thing, thanks, the solution I came up with could be "correct" by coincidence, but I am unsure... the unit is loaded and have the value for armorTechlevel to be [1,1,1,1,1,1,1] which is aparently set as the value for the armor_tech_level overall since this should not be any kind of special armor. This is basic Inner Sphere intro tech, it has to be 1. ( } else if (dataFile.exists("barrating")) {
megamek.common.equipment.ArmorType armor = ArmorType.svArmor(dataFile.getDataAsInt("barrating")[0]);
sv.setArmorType(armor.getArmorType());
sv.setBARRating(armor.getBAR());
sv.setArmorTechLevel(armor.getStaticTechLevel().getCompoundTechLevel(false));
} else { here armorTechlevel sets to all ones, which is correct. then when it runs testEntity.correctEntity(sb, unit.getTechLevel()); There we have the error happening in the first test line in the TestSupportVehicle in the "correctEntity" method. It then calls
The part that interest us is just this one: ArmorType armor = ArmorType.forEntity(this);
if (armor.hasFlag(MiscType.F_SUPPORT_VEE_BAR_ARMOR)) {
double total = getTotalOArmor() * armor.getSVWeightPerPoint(getArmorTechRating());
return RoundWeight.standard(total, this);
} The call for the code that adds the property and value which looks fot the the armorTechLevel of the armor of the unit in a pre-specified location (hardcoded to be location 1), not the tech rating, but the tech level... Currently, the value in this place is always 1, because it should be 1 since it is an Intro tech armor type If we load the rating 1 it will multiply the number of points of armor for the value of the wrong index: armor.weightPerPointSV = new double[] { 0.180, 0.088, 0.056, 0.045, 0.040, 0.037 }; which is the index 1, so 104 * 0.088 = 9.152, which rounds to 9.5 tons, and therefore breaks everything. If it reads the rating 3, the value is 104*0.045, which is 4.69, rounds to 5 tons, the expected amount. Thats why my initial assumption worked, the index match, however, there can only be one error here, either armor_tech_rating should be a value present and should be set at 3, or armor_tech should be present and should be at a value of 3. From all that I could get, the armorTechLevel is correct, and so are you, I missed in my fix here and the result was a coincidence. But this might mean that we have a problem with the BLK Encoder on MegaMek, encoding the wrong value there? |
The unit encoding process looks for a random (hardcoded the number 1) "armor tech level" inside the array of the armor_tech_level and picks that value as the armor tech level, however this value isnt being set when the armor is changed, the armor_tech_rating is set instead.
I need to validate why or how the BAR is set, but it is being correctly set, so this isnt as much a big of a deal.
Also, it had a piece of code right after the part where it should be setting it, but it wasan't correct and also was never called by any code anywhere, so I removed it.
Maybe I should take a further look in the persisting process because it is kind of a messs right now.
Fixes: #1631