Not all air attacked, various other bugs

[airwalker1]

plane bug on ukraine.zip

My Operating System:

TripleA version: 7594

Map: Revised

Can you describe how to trigger the error? (eg: what sequence of actions will recreate it?)

Do you have the exact error text? Please copy/paste if so

Instead of this error, what should have happened?

Any additional information that may help:

Strange behavior on this release. Here is a savegame with an attack on Ukraine with 4 air units moved and only 3 were involved in combat. Other issues during same game were a problems with blitzing not working ( tank would not move back ) on Ukraine, and again on Trans-Jordan. Also my opponent had two subs from purchase disappear after placement. The other issues I believe were after the save so don't dig deep into the savegame.

plane bug on ukraine.zip

[ron-murhammer]

@airwalker1 Yeah, that is a strange error and definitely not good. I was able to take a save at the beginning of US turn and redo your moves to reproduce only 3 of the 4 fighters rolling.

Did you happen to play this save with any other versions of TripleA engine? There was a problem with the battle calc causing weird unit issues that was recently fixed.

[airwalker1]

Nope, it was all on 7594. I was testing it due to the calc being fixed. I'm back to 7378 again.

[ron-murhammer]

@airwalker1 Alright thanks for the response, we'll look into this.

@ssoloff @RoiEXLab If you have some time, this is a pretty serious issue that we need to identify. I was able to reproduce this with a save at the beginning of USA's turn.

[ssoloff]

I tried to follow @ron-murhammer's repro by producing a save game using the Save Game at this point (BETA) feature from the attached save game at the start of USA's turn in round 4. Using 1.9.0.0.7601, I sent two fighters from Caucasus to attack Ukraine S.S.R. and observed only one of them involved in combat, so I was able to repro the reported problem.

I then loaded the same save game in 1.9.0.0.3635 and performed the same combat move. In this case, I once again observed only a single fighter participating in combat.

TL;DR: I'm not sure this is a regression (or if it is, it was introduced before the 1.9 release). Therefore, it's not going to be as simple as bisecting looking for the breaking change but will require some brute-force debugging. @ron-murhammer, before I start doing that, I just want to confirm that the procedure I used to generate the save game is not producing any false negatives. Should I be doing something different?

[ron-murhammer]

@ssoloff Yeah, that was how I reproduced it as well. My gut says the save game data is the problem and that 1 fighter has some property set that is making it so it doesn't participate in battles (that was why I asked about using a version before the BC fix). That means whatever version of 1.9 you load it up in, the problem will exist. There is also a good chance that if you put all the players on DoNothingAI for a turn then try the same attack that it will work fine since if my theory is correct, the unit properties are cleared each round.

The question boils down to if there is some actual bug that caused the bad property or if the save is just corrupted or edit mode caused it.

[ssoloff]

One of the fighters from Ukraine S.S.R. is being classified as a non-combatant and subsequently removed from the battle. The following statement from MustFightBattle#removeNonCombatants() is where this particular unit is being removed:

// remove any allied air units that are stuck on damaged carriers (veqryn)
unitList.removeAll(Matches.getMatches(unitList, Match.allOf(Matches.unitIsBeingTransported(),
    Matches.unitIsAir(), Matches.unitCanLandOnCarrier())));

It looks like the unitIsBeingTransported() match would not normally match this unit, but it does because the fighter's m_transportedBy field is not null and set to "carrier owned by British." I haven't gone back through the history yet, but my guess is this fighter was on a British carrier at some point, but its m_transportedBy field was not reset to null when it moved to Ukraine S.S.R. (or some other land territory). So we'll probably need to reproduce that scenario to find where the root cause of the bad m_transportedBy handling is located.

[ssoloff]

I'm having a heck of a time just trying to get m_transportedBy set to a non-null value for an air unit. Moving a fighter to a (same or allied) carrier doesn't set it. Moving the carrier with loaded (same or allied) fighters doesn't set it. I've also tried moving the carrier and fighter in the same turn such that the carrier needs to move in order for the fighter to land after combat.

I'm a bit stumped. What triggers an air unit to be considered "transported by" a carrier? It seems related to having the carrier classified as a "dependency" of an air unit. Maybe that's the question I should be asking.

[airwalker1]

@ron-murhammer The game was never a save game until I did so to upload it. @ssoloff Yes, at one point I had USA fighters on Brit carrier.

Where do I find the save game at this point option? I enabled beta features for this purpose but cannot find it.

[airwalker1]

Bleh, I found it :)

[ron-murhammer]

@airwalker1 If you open game history and right click at any point, you can then select to beta save game from that point in time. This works pretty well as long as you save at like the beginning/end of player turns not in the middle of battles.

[ron-murhammer]

@ssoloff It should set the property on fighters if you land say a US fighter on a UK carrier then move that carrier during the UK turn I believe. You may need to move the UK carrier into a battle, can't remember.

[ron-murhammer]

@ssoloff I fixed a similar case a while back in #1777 for reference.

[ssoloff]

You may need to move the UK carrier into a battle

That's the step I was missing: the carrier itself was not involved in a sea battle. Thanks!

[ssoloff]

Ok, I found a candidate where an allied air unit's transportedBy property is not being reset to null. The attackingUnits collection passed to MustFightBattle#clearTransportedByForAlliedAirOnCarrier() does not contain the allied fighter that was on the carrier when the carrier engaged in a sea battle. Therefore, the code in this method that searches specifically for allied air units on a carrier in order to reset their transportedBy property to null never runs.

There are two calls to clearTransportedByForAlliedAirOnCarrier() in MustFightBattle#endBattle(). The first passes m_attackingUnits as the attackingUnits parameter, and the second passes m_attackingUnitsRetreated. In my test scenario, neither of these collections contains the allied air unit. Does there need to be a third call that uses a collection that contains allied air units that were moved into the battle but, for whatever reason, did not participate?

[ron-murhammer]

@ssoloff Interesting. I added the second check to handle the retreated attackers in #1777. Did you test this in 3635? Just wondering if anything changed recently to not have the transportedBy fighters in the attackers list. If this is reproducable in 3635 then I'd say we do need to figure out what list the transportedBy fighters are in and add a check to clear them as well.

[ssoloff]

@ron-murhammer Yes, I just reviewed your #1777 changes and see where the new code could now be a problem because it previously considered all units in the battle site and now it only considers the union of m_attackingUnits and m_attackingUnitsRetreated.

What I tested in 3635 was the reported problem, which I was able to reproduce. However, that was after the transportedBy property had already not been reset to null. Let me try it again starting from a clean slate...

[ssoloff]

Confirmed that I cannot reproduce the problem in 3635. My allied fighter has its transportedBy property reset to null as evidenced by the fact that it now participates in subsequent battles.

[ron-murhammer]

@ssoloff Hmm. So we changed something since then that removed transportedBy units from the attackers list then?

[ssoloff]

@ron-murhammer Not sure. Per the rules, I would assume that allied air units on a carrier are considered cargo, and thus are excluded from the collection of attacking units when the carrier attacks. That is, that particular behavior should have been present since day one.

I'll re-run the test in the releases immediately prior to and after #1777.

[ssoloff]

@ron-murhammer I tested 4445 (without #1777) and 4451 (with #1777), and they both behave as expected (i.e. the allied air unit has transportedBy reset to null). So your theory that, since the introduction of #1777, something removed the allied air units from m_attackingUnits appears to be correct.

I'm heading out soon and will be offline for several hours. If no further progress has been made by the time I get back online, I'll start looking for candidates that touched m_attackingUnits.

[ssoloff]

Here's a quick list of commits that touched anything named m_attackingUnits since #1777 (907a40d20):

$ git lg -S"m_attackingUnits"
...
* 03eb48bb7 (tag: 1.9.0.0.7321) Formatting and a few small refactor changes
...   
| * | 65adaaf30 Revert #2080
|/ /  
...
* 5b5db7817 (tag: 1.9.0.0.6706) Mostly cleaning up formatting and comments
...   
| * | b1bee6c18 Fix up and reinstate #1646 to not remove infrastructure units
... 
* | 8d28d28bc Checkstyle: Fix member name violations
...         
| * | | | | 43f84c27f Replace Match.allMatchNotEmpty usage
...     
| | * | 5e670f818 Remove m_/s_ prefix in swing classes
| |/ /  
|/| |   
...     
| * | | 0de4b90ba Remove logic that destroys all defenders so that they can be captured
|/ / /  
...     
| * | | 907a40d20 Remove transportedby for allied air units on retreating carriers

[ssoloff]

@ron-murhammer Please ignore my previous analysis. I was using GitHub releases to run my tests, and there's apparently something wrong with build 4451. The tags for builds 4451, 4452, 4453, and 4454 all point to the same commit. However, when I run my test using those four builds from GitHub releases, 4451 passes, while the other three fail. That shouldn't be possible if they were all built from the same commit.

I went back and re-ran my test directly against specific Git commits by building from source. The test fails when run against 6f87bcfc2 and passes when run against 7ba5d4a56 (which is 6f87bcfc2^). Therefore, I think it's safe to conclude that this issue was introduced in #1777. That would imply that, prior to #1777, the union of m_attackingUnits and m_attackingUnitsRetreated is not necessarily equal to battleSite.getUnits().getUnits().

[ron-murhammer]

@ssoloff Oh ok. In that case, I think just updating where #1777 replaced battleSite.getUnits().getUnits() with attackingUnits to the combination of attackingUnits and battleSite.getUnits().getUnits() should resolve this.

[ssoloff]

@ron-murhammer Just curious... Under what scenario would attackingUnits (i.e. m_attackingUnits or m_attackingUnitsRetreated) not be a subset of battleSite.getUnits().getUnits()?

[ron-murhammer]

@ssoloff During retreats which was the scenario that #1777 fixed. That was why I needed to change it to attackingUnits instead of battleSite.getUnits().getUnits(). I made the incorrect assumption that attackingUnits included all of the attacking players units from battleSite.getUnits().getUnits().

[ssoloff]

@ron-murhammer Got it. I didn't realize retreating units had already been removed from the battle site at this point.

[ron-murhammer]

@ssoloff Yeah, the whole MustFightBattle class is very hard to follow with many of the member variables lists being changed from all over the place. Before #1777, the same issue that we have here around transportedBy not being cleared happened if the attackers had a carrier with allied planes that retreated from battle.

So in the end, #1777 fixed the carrier with allied planes retreat scenario but caused the problem to occur if the carrier with allied planes wins the battle. So really need to check both attackingUnits and battleSite.getUnits().getUnits() for allied planes.

[airwalker1]

Here's another one guys. This time UK carrier out of picture but 3 US planes and one bomber attacking german BB and transport in SZ15. Have only 2 planes attacking + bomber. I saved it right at combat. This is on 1.zip today's release 7601.

[ssoloff]

@airwalker1 I believe this is the same issue. I dropped into the debugger and confirmed that one of the USA fighters has a transportedBy property of "carrier owned by British." So, as in your other save, that unit gets excluded from the collection of attacking units because it is erroneously considered a non-combatant.

The bug we've been discussing above fails to clear transportedBy after you moved that air unit off the allied carrier in a previous round. I don't think there's anything else in the engine that will trigger clearing that property. Thus, that unit is basically stuck in a limbo state where it won't be included in future attacks. The only way you may be able to recover at this point is to use edit mode to delete the existing unit and replace it with an equivalent new unit. The new unit should have an empty transportedBy property and will be able to participate in attacks.

[ron-murhammer]

@ssoloff Are you planning to submit the PR to fix this? You did the heavy lifting and testing so deserve the credit :) Otherwise I can submit it. I'd like to get this fix into the live stable ASAP.

As a side note, it may eventually be worth it to put some unit tests around this but that would probably require some significant effort.

[ssoloff]

@ron-murhammer I'm just about to shut down everything for the night. I can submit a PR in about 9-12 hours if that's early enough to get into tomorrow's proposed release.

[ron-murhammer]

@ssoloff That works for me. Have a good night.

[ssoloff]

@airwalker1 This should be fixed in 1.9.0.0.7611 or later.

As noted above, however, once you have a unit affected by this bug, upgrading to a new engine won't help you (the fix simply prevents the bug from happening in the future). You'll need to delete and recreate the unit via edit mode to clear out the munged transportedBy property.