Fix 1631: Armor rating incorrectly persisted

[Scoppio]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 2.16%. Comparing base (34486a0) to head (55e4fc5).
Report is 11 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

[pavelbraginskiy]

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.

[Scoppio]

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.

[pavelbraginskiy]

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).

[Scoppio]

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. (
Bar7 says that its static tech level is 1, so this correct, so this should not be the problem).

} 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 warnOnInvalid(loadedUnit) called inside loadUnitFromFile, the validateUnit from UnitUtil gets the entity verifier and 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 calculateWeightExact() who calls getWeightArmor() who calls ```
@OverRide
public double getWeightArmor() {
return supportVee.getArmorWeight();
}

And here we have this:

```Java
 public double getArmorWeight() {
        if (hasPatchworkArmor()) {
            double total = 0;
            for (int loc = 0; loc < locations(); loc++) {
                total += getArmorWeight(loc);
            }
            return RoundWeight.standard(total, this);
        } else {
            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);
            } else if (armor.hasFlag(MiscType.F_BA_EQUIPMENT)) {
                return getTotalOArmor() * armor.getWeightPerPoint();
            } else {
                double armorPerTon = ArmorType.forEntity(this).getPointsPerTon(this);
                return RoundWeight.standard(getTotalOArmor() / armorPerTon, this);
            }
        }
    }

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 getArmorTechRating() is looking for the value in the property armorTechRating, which is persisted as armor_tech

the code that adds the property and value armor_tech is this part:
https://github.com/MegaMek/megamek/blob/f19b6591e62347036925e2aef488ca4e1c61ba01/megamek/src/megamek/common/loaders/BLKFile.java#L749

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
However, the number we are looking for in this case is 3, the rating (A, B, C, D, E, F), not the tech level (1).

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?
Please let me know what you think of that one.