Destroyed equipment not tracked properly on save/load

[RexPearce]

In 0.45.3, certain destroyed equipment is not tracked properly on a save/load.

E.g., if you salvage a weapon from a mech, then save/load, the weapon that was just salvaged will be available again for salvage from the mech.

My Campaign30250102.zip

To reproduce:

1. Load the attached campaign which has a mech ready to be salvaged. Note the warehouse is empty.

3. In repair bay, select the RA location of the mech, and salvage the Large Laser.

4. Save the campaign, then reload it. Note the warehouse correctly has a Large Laser.

5. **Bug:** Go back to the repair bay and note the Large Laser is available for salvage again.

6. **Bug:** Switch the mech to repair mode, note the Large Laser is marked as repairable (not replaceable), and can be repaired without consuming the Larger Laser in the warehouse.

I've noticed this seems to mainly affect the first (top) weapon installed in a location: if you repeat the above steps salvaging the RA Medium Laser instead, there is no problem.

Note: from diffing the two campaign files, it looks like the Large Laser is correctly marked as destroyed, so I'm guessing the problem is in the load routines:

-         <slot index="5" type="Large Laser" quirks="jettison_capable" isDestroyed="false"/>
-         <slot index="6" type="Large Laser" quirks="jettison_capable" isDestroyed="false"/>
+         <slot index="5" type="Large Laser" quirks="jettison_capable" isHit="true" isRepairable="false" isDestroyed="true"/>
+         <slot index="6" type="Large Laser" quirks="jettison_capable" isHit="true" isRepairable="false" isDestroyed="true"/>

MekHQ v0.45.3
Ubuntu 18.04.2
OpenJDK v1.8.0_191

[HammerGS]

We need the log files as well, and any custom units.

[RexPearce]

Logs attached. (No custom units.)

logs.zip

[RexPearce]

@HammerGS I looked into this a bit last night and I think I found the location causing the problem:

mekhq/MekHQ/src/mekhq/campaign/io/CampaignXmlParser.java

Lines 430 to 434 in 8206566

	// Remove existing duplicate parts.
	if (u.getPartForEquipmentNum(((MissingEquipmentPart) prt).getEquipmentNum(), ((MissingEquipmentPart) prt).getLocation()) != null) {
	removeParts.add(prt);
	continue;
	}

^ u.getPartForEquipmentNum() is returning a match here incorrectly, which causes the MissingEquipmentPart object for the Large Laser to be removed. The odd thing is this check for a Large Laser is returning a match for a MekGyro part.

---

mekhq/MekHQ/src/mekhq/campaign/unit/Unit.java

Lines 4071 to 4078 in 8206566

	public Part getPartForEquipmentNum(int index, int loc) {
	for(Part p : parts) {
	if(p.isPartForEquipmentNum(index, loc)) {
	return p;
	}
	}
	return null;
	}

^ u.getPartForEquipmentNum() just calls isPartForEquipmentNum() for each part

---

mekhq/MekHQ/src/mekhq/campaign/parts/MekGyro.java

Lines 253 to 257 in 90989f1

	@Override
	public boolean isPartForEquipmentNum(int index, int loc) {
	return Mech.SYSTEM_GYRO == index;
	}

^ isPartForEquipmentNum() return a true if the index matches, ignoring location.

---

So if I'm understanding this correctly, any MissingEquipmentPart object will get removed if it happens to have the same index number as the MekGyro (despite being if a different location).

Not sure what the correct fix is this though.

It seems like MekGyro.isPartForEquipmentNum() shouldn't be ignoring location, however, many of the part classes (e.g. engine, cockpit) also ignore location... so maybe these methods are just being used incorrectly here?

Also, why CampaignXmlParser even checking MissingEquipmentPart objects against the unit for duplicates? (I wonder if this duplicate check (L430-L434) is just a copy and paste error or something.)

[HammerGS]

@sixlettervariables @NickAragua Would best be able to answer those questions.

[sixlettervariables]

This is a dupe of a few other reports, but certainly the best researched one. I'll try tracking this down this weekend.

[sixlettervariables]

Adding a check for location fixes this specific issue. Whether that fix encompasses all the problems...don't know yet.

[RexPearce]

On related note, while debugging I noticed the same problem occurring for the EquipmentPart duplicates check a few lines above, here:

mekhq/MekHQ/src/mekhq/campaign/io/CampaignXmlParser.java

Lines 417 to 421 in 8206566

	// Remove existing duplicate parts.
	if (u.getPartForEquipmentNum(((EquipmentPart) prt).getEquipmentNum(), ((EquipmentPart) prt).getLocation()) != null) {
	removeParts.add(prt);
	continue;
	}

Basically, weapon EquipmentParts are incorrectly matching against core components (gyros, cockpits, engines, etc.) and being removed. However, they seem to be re-added again later (I think by InitializeParts()), so it went unnoticed.

A check for location should fix this unseen problem too.

[sixlettervariables]

Appreciate the assist, your hunch is correct. I've validated at least with your case that this is the correct fix.

[RexPearce]

Thing I don't understand is why MissingEquipmentPart is being checked at all for duplicates, at least they way they are in L430-L434. The duplicate check doesn't seem to limit itself to checking for MissingEquipmentPart, so if getPartForEquipmentNum() finds an actual part installed at same location, the MissingEquipmentPart gets removed instead?

[sixlettervariables]

I think what has happened in prior versions is you get duplicate parts between the campaign and unit. Apparently initializeParts was likely what we relied (unknowingly?) on to "fix" the issues caused by that code.

That was based on the comments around:

mekhq/MekHQ/src/mekhq/campaign/io/CampaignXmlParser.java

Lines 449 to 452 in 90989f1

	// if actuators on units have no location (on version 1.23 and
	// earlier) then remove them and let initializeParts (called
	// later) create new ones
	if (prt instanceof MekActuator

[RexPearce]

I've been running a build of snapshot 0.45.3 but w/ L430-L434 commented out - seems to work well.

(BTW, I'm using 0.45.3 as base for testing this because a change made to master since then seems to have broken replacing weapons - at least for OmniMechs in my campaing. Do you want me to open another issue for that?)

[sixlettervariables]

Yes, did not know omni replacements were broken.

[RexPearce]

Ok, give me a bit to actually confirm it...

[sixlettervariables]

The code to remove "duplicate" missing parts was added 4 years ago: cc5bd6a

It references this issue:
https://sourceforge.net/p/mekhq/bugs/693/

Reloading the CPNX canopus-3.cpnx.zip results in this exception (excluding the missing equipment logic):

java.lang.ArrayIndexOutOfBoundsException: 5
	at mekhq.campaign.unit.Unit.initializeParts(Unit.java:1888)
	at mekhq.campaign.io.CampaignXmlParser.parse(CampaignXmlParser.java:676)
	at mekhq.campaign.CampaignFactory.createCampaign(CampaignFactory.java:88)
	at mekhq.gui.dialog.DataLoadingDialog$Task.doInBackground(DataLoadingDialog.java:200)
	at mekhq.gui.dialog.DataLoadingDialog$Task.doInBackground(DataLoadingDialog.java:1)
	at javax.swing.SwingWorker$1.call(SwingWorker.java:295)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at javax.swing.SwingWorker.run(SwingWorker.java:334)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)

[RexPearce]

Hmm... the last comment on that issue says "The problem was a MissingEquipmentPart that linked to a mounted with a -1 location." ... would that have been caught by L413? (I'm guessing LOC_NONE == -1 ?)

Would that exception be avoided if the mounted check is left intact, but the duplicate check is removed (for MissingEquip)?

[RexPearce]

Sorry, meant L425 in above comment:

mekhq/MekHQ/src/mekhq/campaign/io/CampaignXmlParser.java

Line 426 in 8206566

if (null == m || m.getLocation() == Entity.LOC_NONE) {

[RexPearce]

BTW, I'm getting the same NPE when loading canopus-3.cpnx on 0.45.3 and on current master. Might be some other problem with that campaign...

[sixlettervariables]

Yeah there is a unit with a bad entity (unrelated).

[NickAragua]

I wonder if this was fixed with the PR I just pushed up. I'll check it tomorrow.

[sixlettervariables]

@NickAragua I checked this AM, and it looks like it did. It does look like some additional checks may be helpful to prevent this in the future.

[RexPearce]

@sixlettervariables @NickAragua I just checked here as well on latest master and it I'm still seeing the bug.

This bug is different than the other two: this one is caused by false positives when checking for duplicates, whereas the other two were related to MASC.

[sixlettervariables]

@RexPearce can you check out either my PR branch or the CI/CD release: https://dev.azure.com/drivingguy/Megamek/_build/results?buildId=82&view=results

I believe it is fixed.

[RexPearce]

@sixlettervariables I did a quick glance at your PR and it looks like its addressing the right problem (the 'not comparing location' problem we discuss a few days ago. Let me see if I figure out how to pull your PR locally...

[RexPearce]

@sixlettervariables looks like PR #1130 fixes the problem for me.

[RexPearce]

@sixlettervariables FYI looks like pr #1130 didn't fully fix the problem - just looking into it now.

[sixlettervariables]

Missed your comment just now! Reopened.

[RexPearce]

It looks like the duplicates check is still getting false positives, here:

mekhq/MekHQ/src/mekhq/campaign/io/CampaignXmlParser.java

Line 429 in 0a31e1b

if (u.getPartForEquipmentNum(((MissingEquipmentPart) prt).getEquipmentNum(), ((MissingEquipmentPart) prt).getLocation()) != null) {

Good news is the false positives now come from the same location as the missing part, so your PR appears to have solved the location issue.

In my testing all the false positives were generated against actuators, so for some reason the index of the MissingEquipment part matches an actuator in the same location.

Here's my main campaign which is showing the problem across multiple mechs in the repair bay. For easier debugging, I also included a stripped-down version of the campaign '1109_test.cpnx' that removes all the units except a Thor which has this problem with its ER PPC

1109_test.zip

[sixlettervariables]

I have a tentative fix, but I'm not sure how it would function in practice:

// Remove existing duplicate parts.
                    Part duplicatePart = u.getPartForEquipmentNum(((MissingEquipmentPart) prt).getEquipmentNum(), 
                        ((MissingEquipmentPart) prt).getLocation());
                    if (duplicatePart != null
                        && prt.isSamePartType(duplicatePart)) {
                        removeParts.add(prt);
                        continue;
                    }

I believe this would work correctly in all cases, but I'm not certain.

[sixlettervariables]

Hrm, that does checks on sticker prices in some cases and probably isn't appropriate if we're considering missing equipment. I think a slightly simpler approach is better (i.e. just check their equipment types):

                    // Remove existing duplicate parts.
                    Part duplicatePart = u.getPartForEquipmentNum(((MissingEquipmentPart) prt).getEquipmentNum(), 
                        ((MissingEquipmentPart) prt).getLocation());
                    if (duplicatePart instanceof EquipmentPart
                        && ((EquipmentPart)prt).getType() == ((MissingEquipmentPart)duplicatePart).getType()) {
                        removeParts.add(prt);
                        continue;
                    }

[RexPearce]

Sorry if this is obvious, I'm pretty new to all this, but I'm not getting what part.EquipmentNum is supposed to represent. Originally, I thought equipmentNum was a location index (e.g. an equipmentNum of 4 would mean the part is installed at 4th critical slot of its mech location).

However, equipmentNum is first passed here:

mekhq/MekHQ/src/mekhq/campaign/unit/Unit.java

Lines 4071 to 4078 in 0a31e1b

	public Part getPartForEquipmentNum(int index, int loc) {
	for(Part p : parts) {
	if(p.isPartForEquipmentNum(index, loc)) {
	return p;
	}
	}
	return null;
	}

Then equipmentNum is compared to the MekActuator type (which is causing the false positive):

mekhq/MekHQ/src/mekhq/campaign/parts/MekActuator.java

Lines 303 to 305 in 0a31e1b

	public boolean isPartForEquipmentNum(int index, int loc) {
	return index == type && loc == location;
	}

is equipmentNum supposed to represent type, not location?

[RexPearce]

Also, I'm still wondering if checking for duplicate missing parts is the right thing to do at all. The missing duplicate check is not limited to checking against other MissingParts. (i.e. why check the existing-parts for duplicates of missing-parts?)

[sixlettervariables]

That sort of tribal knowledge is before my time, so this adventure has been discovery for me as well. My guess has been this is basically around fixing problems from older versions of MHQ.

[sixlettervariables]

Perhaps the better question is why are parts that aren't equipment parts, returning a value that they are in fact an equipment part?

[RexPearce]

That sort of tribal knowledge is before my time, so this adventure has been discovery for me as well. My guess has been this is basically around fixing problems from older versions of MHQ.

My guess is that when this bug: https://sourceforge.net/p/mekhq/bugs/693/ was found, the mount and duplicate check for existing-parts were both copied and modified to apply to missing-parts, however the duplicates check on missing parts wasn't actually necessary (and may not even make sense).

[RexPearce]

Perhaps the better question is why are parts that aren't equipment parts, returning a value that they are in fact an equipment part?

I think because equipmentNum is being compared to type (not sure if you saw my comment above re: why the MekActuator is matching against MissingEquipment - I posted two comments in a row so you may have missed it)

[sixlettervariables]

I've expanded on #1132 to include what we've discussed and found.

[RexPearce]

ah, I misunderstood what you were saying above. So MekActuactors (and gyros, engines, etc) should not be returning that they are equipment parts on these checks.

[RexPearce]

@sixlettervariables just tested PR #1132 with my main campaign and it seems to have resolved the issue.

Note: I found another issue with reappearing parts, but I believe the underlying cause is different. Should I post a new issue for it, or do you want to cover it here?

[sixlettervariables]

I'm fine tracking it here.

[RexPearce]

ok, so this one appears related to OmniMechs with missing locations.

If you load the full campaign (not the stripped down one) I posted earlier in 1109_test.zip, you can see the problem with the Dragonfly in the repair bay

First issue: Note that the Dragonfly's left arm is destroyed, however, the SRM 6 there can be salvaged. Shouldn't all equipment in a destroyed location also be marked as destroyed? (Except maybe equipment that spans multiple locations.)

Second issue: Scrap the LA SRM 6, and save/reload, switch back to the Dragonfly's LA in the repair bay. Note that the SRM 6 stays scrapped, however, the LA pod space can still be salvaged.

Third issue: Salvage the (destroyed) LA's pod space, and save/reload. Note that all equipment in the destroyed LA has been restored and can be salvaged/scrapped again.

[sixlettervariables]

The SRM6 is listed as damaged but not missing, as opposed to the FCS in that same spot that is listed as destroyed (MissingEquipmentPart).

[RexPearce]

Yeah, the first issue only shows up on the SRM6 on the test campaign. Though after repeating the steps for the 2nd and 3rd issues, all LA equipment will re-appear, including the FCS.

Were you able to see the same behaviour on your end after repeating all the steps above?

[Windchild292]

Closing as fixed by #1132