search

triplea-game / triplea

TripleA is a turn based strategy game and board game engine, similar to Axis & Allies or Risk.
https://triplea-game.org/
GNU General Public License v3.0
1.34k stars 393 forks source link

Can't remove transported units without removing transport #4446

Closed drockken closed 5 years ago

drockken commented 5 years ago

Engine version

1.10.13296

My Operating System

Windows 10/64 bit Java 1.8.0_144-b01 according to TripleA Java 1.8.0_121-b13 according to the Java Control Panel

Map name

ww2global40_2nd_edition.xml

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

Open Saved Game Enter Edit Mode Click six infantry in Dzavhan Click Remove Selected Units, this generates the error "Can't remove transported units without removing transport"

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

"Can't remove transported units without removing transport"

Instead of this error, what should have happened?

I should have been allowed to remove the units via the edit. There are no transports in this land-locked territory.

Any additional information that may help

I forgot to "attack" the Russian units in Olgy so I rolled the dice after the fact and was manually going to move my units into the territory from Dzavhan and another adjacent territory. To do this I had to enter the edit mode and manually move them. It did not let me remove the units.

drockken commented 5 years ago

Once again didn't attach my Zip, trying again... no_transport.zip

panther2 commented 5 years ago

The first of the six Japanese Infantries can be removed as usual. Starting from the second one this is what happens: tra

Just to illustrate...

panther2 commented 5 years ago

I found out that the error is somehow caused by the Infantry units that came from Yenisey (to Dzavhan). As those from Tsagan Oloom can be removed without error when starting the game one round before.

Now I am tracing the history of those Yenisey units...

On round 9 Japanese NCM those three units met in Buryiatia. Two of them deriving from Amur, one deriving from Sakha. The two from Amur are those who cause the error. They came from Manchuria during NCM of round 8, brought there from Korea in round 7. During Combat Move of Round 6 they were brought from Japan to Korea by transport. The attached savegame starts the game after Japan 6. It can be easily verified, that the error started here by trying to remove those Infantries landed in Korea:

tra2 no_transport_test.zip

However the game history does not show anything remarkable at that point.

Referring to #4445 it should be pointed out, that this is again a game global_1940 involving the AI. Though likely totally unrelated, maybe @ron-murhammer can comment about the AI on the global_1940-map?

ssoloff commented 5 years ago

Java 1.8.0_144-b01 according to TripleA Java 1.8.0_121-b13 according to the Java Control Panel

@drockken Just wanted to note why there is a discrepancy here... It is likely you originally installed TripleA without having Java already installed. In that case, the TripleA installer will download and install its own Java runtime (JRE). Subsequent runs of TripleA (even if you update to a newer version), will always use that bundled JRE no matter which version of Java you've installed via Windows Installer.

I mention this because there has been a lot of discussion about the problems these bundled JREs may cause (e.g. see #4358). If you ever want to use the version of Java you've installed yourself, you'll need to manually delete the bundled JRE. (Note that it's unlikely the version of Java you're using has any specific relation to this particular bug, but it's always good to be aware about these things. :smile:)

simon33-2 commented 5 years ago

I also have noticed this problem. Glad we have a test case, or at least I think we do.

ron-murhammer commented 5 years ago

@panther2 @drockken I just looked through the save games and there definitely is an issue with the 'transportedBy' field not being cleared for the infantry that amphibious assault Korea in round 6 (nice work tracing that back @panther2). I was able to reproduce it as well by taking a save before that battle and making the same attack on korea: test_after_cm.zip. After rolling that battle if I try to edit out those infantry left in Korea then I get the error.

I should be able to look through the code to see why that is happening.

ron-murhammer commented 5 years ago

So the issue is caused by a rare situation where a nation is recapturing a kamikaze/convoy sea zone and then amphib assaulting a territory adjacent to that sea zone. I also tested it on 1.8.0.9 and it exists there as well (probably has existed for years). Here are the steps to reproduce:

  1. USA moves through empty SZ 6 in round 5 so it gains control of it since its a blockade/kamikaze SZ.
  2. Japan moves its fleet to SZ 6 recapturing it during round 6 then loads inf and unloads them to amphibious assault Korea.
  3. Because SZ 6 is being captured by Japan it creates a pending FinishedBattle since its empty to represent that capture and the infantry unloading to Korea are dependent on that FinishedBattle before they can clear their transportedBy field.
  4. The pending FinishedBattle is run at the start of combat phase but it appears FinishedBattle doesn't consider unloaded units so doesn't clear the tranportedBy field.
  5. The engine then believes those infantry are being transported by the transport still and its never cleared.
  6. When you try to remove those units using EDIT mode, it wants the transport to be removed as well since it believes they are still on it.

@veqryn This appears related to when you created the concept of FinishedBattle and these 2 commits: https://github.com/triplea-game/triplea/commit/52807d90668169ef60e49ea312299e514e15aa1d https://github.com/triplea-game/triplea/commit/d7291802fbf73c3f671f14d44ff013a47e93ff04

Any thoughts on how this should work? My guess is either:

  1. If the dependent battle is FinishedBattle then shouldn't block clearing transportedBy: https://github.com/triplea-game/triplea/blob/master/game-core/src/main/java/games/strategy/triplea/delegate/TransportTracker.java#L131
  2. FinishedBattle should check for unloading amphib units like winning a MustFightBattle: https://github.com/triplea-game/triplea/blob/master/game-core/src/main/java/games/strategy/triplea/delegate/MustFightBattle.java#L2481
  3. FinishedBattle shouldn't be created for empty SZ so that the unload just occurs immediately.
  4. Sea Zone ownership should be handled differently so that Japan doesn't need to capture empty SZ 6 back from the USA.

I'm generally leaning towards option 1 as its the simplest but option 2 could work as well and is more or less equivalent. I'm not really sure what other impacts option 3 or 4 would have as I don't fully understand FinishedBattles.

simon33-2 commented 5 years ago

Given that battles with no defenders are now fought automatically in BattleDelegate.start(), is there a need for the concept of a finished battle?

ron-murhammer commented 5 years ago

@simon33-2 It appears to be used to mark territories that were blitzed/conquered for this XML property:

battle  values: checks if there has been a battle in the territory. Format is "attacker:defender:battleType:rounds:territory1", where attacker, defender, and battleType can all be "any", and rounds can be "currentRound". Territories can be any number of territories, and if more than one territory is listed then if any of them are true, it will return true.  You can also have multiple instances
example: <option name="battle" value="any:Japanese:any:2-3:Manchuria"/> or <option name="battle" value="Russians:Japanese:any:currentRound:Manchuria:Jehol:Chahar:Suiyuyan:Kansu"/>
simon33-2 commented 5 years ago

And therefore prevent building a factory/facility or landing planes? But couldn't those things be achieved with a NonFightingBattle?

veqryn commented 5 years ago

I believe FinishedBattle is also around to provide a history of conquered and blitzed territories (including sea zones), in addition to being used by various triggers and attachments.

I'm leaning towards option number 2 above: have FinishedBattle clear the transportedBy. Option 1 might be better or worse, but I'm unsure of what other ramifications it might have, so unless you all have added a lot more test cases for all the games' rule's logics', then I'm unsure about it. Option 3 and 4 would definitely affect a lot of games that use convoys, blockades, etc etc, so I'd probably not do those.

But I've also been out of the code for years at this point, so do whatever makes sense.

ron-murhammer commented 5 years ago

@veqryn Thanks for the input. Just wanted to see if you remembered anything around FinishedBattles since you had implemented them. I'll probably move forward with option 1 or 2 after I look over it a bit more.

ron-murhammer commented 5 years ago

@drockken Once #4475 is merged, this issue should be fixed in the latest pre-release. It won't fix your existing game but should prevent "Can't remove transported units without removing transport" from occurring for non-transported units in the future.