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.35k stars 399 forks source link

2.1.20365: RemoveUnits#perform:44 - IllegalStateException #7026

Closed tripleabuilderbot closed 4 years ago

tripleabuilderbot commented 4 years ago

User Description

Dwarf turn

Map

WoW turn 20

Log Message

Remote method call exception: Not all units present in:Thandol Strait. Trying to remove:[Flying-Machine owned by Dwarves, Troopship owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls] present:[Fish owned by Trolls, Troopship owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Flying-Machine owned by Dwarves, Flying-Machine owned by Dwarves]

TripleA Version

2.1.20365

Java Version

11.0.6

Operating System

Windows 10

Stack Trace

java.lang.IllegalStateException: Not all units present in:Thandol Strait.  Trying to remove:[Flying-Machine owned by Dwarves, Troopship owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls] present:[Fish owned by Trolls, Troopship owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Footmen owned by Trolls, Flying-Machine owned by Dwarves, Flying-Machine owned by Dwarves]
    at games.strategy.engine.data.changefactory.RemoveUnits.perform(RemoveUnits.java:44)
    at games.strategy.engine.data.GameData.performChange(GameData.java:433)
    at games.strategy.engine.framework.ServerGame$1.gameDataChanged(ServerGame.java:92)
    at jdk.internal.reflect.GeneratedMethodAccessor42.invoke(Unknown Source)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at games.strategy.engine.message.unifiedmessenger.EndPoint.invokeSingle(EndPoint.java:136)
    at games.strategy.engine.message.unifiedmessenger.EndPoint.lambda$invokeMultiple$0(EndPoint.java:120)
    at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
    at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
    at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
    at games.strategy.engine.message.unifiedmessenger.EndPoint.invokeMultiple(EndPoint.java:121)
    at games.strategy.engine.message.unifiedmessenger.EndPoint.invokeLocal(EndPoint.java:111)
    at games.strategy.engine.message.unifiedmessenger.UnifiedMessenger.invoke(UnifiedMessenger.java:152)
    at games.strategy.engine.message.UnifiedInvocationHandler.invoke(UnifiedInvocationHandler.java:48)
    at com.sun.proxy.$Proxy16.gameDataChanged(Unknown Source)
    at games.strategy.engine.framework.ServerGame.addChange(ServerGame.java:638)
    at games.strategy.engine.delegate.DefaultDelegateBridge.addChange(DefaultDelegateBridge.java:91)
    at games.strategy.triplea.delegate.GameDelegateBridge.addChange(GameDelegateBridge.java:62)
    at games.strategy.triplea.delegate.battle.MustFightBattle.remove(MustFightBattle.java:603)
    at games.strategy.triplea.delegate.battle.MustFightBattle.clearWaitingToDieAndDamagedChangesInto(MustFightBattle.java:456)
    at games.strategy.triplea.delegate.battle.MustFightBattle$20.execute(MustFightBattle.java:1777)
    at games.strategy.triplea.delegate.ExecutionStack.execute(ExecutionStack.java:34)
    at games.strategy.triplea.delegate.battle.MustFightBattle.fight(MustFightBattle.java:722)
    at games.strategy.triplea.delegate.battle.BattleDelegate.fightBattle(BattleDelegate.java:242)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at games.strategy.engine.delegate.DelegateExecutionManager$1.invoke(DelegateExecutionManager.java:120)
    at com.sun.proxy.$Proxy12.fightBattle(Unknown Source)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at games.strategy.engine.message.unifiedmessenger.EndPoint.invokeSingle(EndPoint.java:136)
    at games.strategy.engine.message.unifiedmessenger.EndPoint.lambda$invokeMultiple$0(EndPoint.java:120)
    at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
    at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
    at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
    at games.strategy.engine.message.unifiedmessenger.EndPoint.invokeMultiple(EndPoint.java:121)
    at games.strategy.engine.message.unifiedmessenger.EndPoint.invokeLocal(EndPoint.java:111)
    at games.strategy.engine.message.unifiedmessenger.UnifiedMessenger.invokeAndWait(UnifiedMessenger.java:96)
    at games.strategy.engine.message.UnifiedInvocationHandler.invoke(UnifiedInvocationHandler.java:56)
    at com.sun.proxy.$Proxy12.fightBattle(Unknown Source)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at games.strategy.engine.player.DefaultPlayerBridge$GameOverInvocationHandler.invoke(DefaultPlayerBridge.java:152)
    at com.sun.proxy.$Proxy12.fightBattle(Unknown Source)
    at games.strategy.triplea.ai.AbstractAi.battle(AbstractAi.java:609)
    at games.strategy.triplea.ai.AbstractAi.start(AbstractAi.java:517)
    at games.strategy.engine.framework.ServerGame.waitForPlayerToFinishStep(ServerGame.java:537)
    at games.strategy.engine.framework.ServerGame.runStep(ServerGame.java:407)
    at games.strategy.engine.framework.ServerGame.startGame(ServerGame.java:297)
    at games.strategy.engine.framework.startup.launcher.LocalLauncher.launchInternal(LocalLauncher.java:61)
    at games.strategy.engine.framework.startup.launcher.LocalLauncher.launchInternal(LocalLauncher.java:34)
    at games.strategy.engine.framework.startup.launcher.AbstractLauncher.lambda$launch$0(AbstractLauncher.java:16)
    at java.base/java.lang.Thread.run(Thread.java:834)
trevan commented 4 years ago

wow-tr-19-end.zip

The save for this game was uploaded in the forum at https://forums.triplea-game.org/topic/1031/warcraft-war-heroes-official-thread/182?_=1593801653676.

trevan commented 4 years ago

The scenario to duplicate is to have a unit with AA attacks fighting against a sea transport and transported units. If the AA unit can target both the sea transport and the transported units, this issue can occur.

The problem is that during the AA attack rounds, it can attack not only the sea transport, but the transported units. They haven't been removed from the attackable list. If the transported unit is targeted and killed, then the list of units gets out of wack and this error occurs at the end of the battle.

The code that removes transported units is run after the AA phases but before the naval bombardment phase.

trevan commented 4 years ago

Once #7008 has been merged, I'll look into creating a new RemoveNonCombatant step that happens at the start of the battle. It will probably make the original one redundant but we'll need both of them to handle old save games.

trevan commented 4 years ago

The existing removeNonCombantants happens after the AA phase and the naval bombardment phase. It currently removes the following types of units (see MustFightBattle#removeNonCombatants at https://github.com/triplea-game/triplea/blob/c84ae442e78302c92984082d0a03f80b85aeea54/game-core/src/main/java/games/strategy/triplea/delegate/battle/MustFightBattle.java#L1363)

The NavalBombardment step currently remove the following types of units (see MustFightBattle#fireNavalBombardment at https://github.com/triplea-game/triplea/blob/c84ae442e78302c92984082d0a03f80b85aeea54/game-core/src/main/java/games/strategy/triplea/delegate/battle/MustFightBattle.java#L1226):

The AA steps limit their targets to units that are in their targetsAa and also remove infrastructure units (see FireAa#FireAa at https://github.com/triplea-game/triplea/blob/c84ae442e78302c92984082d0a03f80b85aeea54/game-core/src/main/java/games/strategy/triplea/delegate/battle/FireAa.java#L72 and FireAa#execute at https://github.com/triplea-game/triplea/blob/c84ae442e78302c92984082d0a03f80b85aeea54/game-core/src/main/java/games/strategy/triplea/delegate/battle/FireAa.java#L108)

Should the NavalBombardment phase and the AA phase be able to target any of those units that are removed in the RemoveNonCombatants phase? For example, should an AA or naval bombardment be able to target a unit with no defense? Should an AA be able to target a unit that is captured on entering the territory? Etc?

Cernelius commented 4 years ago

@trevan I assume you are explaining whatever it is in the code wrongly, or at least you need to clarify it.

If you say "It currently removes the following types of units", that literally means that every unit belonging to one or more of the following types is removed. This cannot be true for several of the "types" you listed.

If, instead, you meant "It currently removes every unit belonging to each of the following types", that is some less obvious that it can't be, but I still believe this would be the case.

I mean I believe you should clarify the "AND" and "OR" relationships amongst the types you mentioned, for a unit to be removed the way you say.

I'm also much unclear on what you mean by "units in a previous air battle". As far as I know, such units should not be allowed to be part of the normal battle only in case they refused to take part in the air battle, if we are talking about air battles preceding normal combat at all (as opposed to those preceding bombing raids).

Regarding "transported air units that can land on carriers", this is actually a wrong behaviour. In v1 you should be able to take allied air units as casualties, if this is what this type is referring to (I'm not a developer, so I can only infer the matter from the descriptions).

Cernelius commented 4 years ago

For example, I believe that a unit should be removed immediately after the AA phase if: isInfrastructure AND is without attack/defence power (respectively if in attack or defence, of course) AND gives no support (that can be currently received by at least one unit, of course) AND is not AA for combat OR has reached its maxRoundsAa.

Moreover:

It feels wrong to me that "land units in sea battle" are removed immediately after the AA phase (if this is what you mean), as I tend to think land units that are cargo of sea units should not be subjected to AA fire anyway. I would see such a behaviour as a wrong behaviour (bug). For example, if a fighter has AA shots against infantries, I would surely expect infantries that are cargo never being affected by this.

I've no idea what "sea units in land battle on defense" means. As far as I know, there is no legal way for a sea unit to be in a land battle on defence, as sea units can take part in land battle only on the offence (naval bombardment).

Regarding the "transported air units that can land on carriers", there is the known problem that the v1 ability of taking allied fighters as casualties in attack is not supported by the program, but, in any case, I'm not sure why there is such type, as, as far as I know, currently the engine always launches all fighters of the turn power and all defending fighters are launched too, therefore the only fighters that can be still "transported", during a battle, are allied fighters on attacking carriers, and, normally, allied units should not take part in battles offensively. So, I wonder why is the engine removing "transported air units" immediately after the AA phase. If this refers to air units that are still cargo (of carriers), then I tend to think they should be removed before the AA phase, as I think they should not be subjected to AA fire, while being cargo. But here I'm just very unclear and, not being a developer, I may easily miss something critical, on the matter.

trevan commented 4 years ago

@Cernelius I'm trying to fix a bug where transported units are targeted during the AA phase. The fix that I'd like to do is to move the RemoveNonCombatants phase/step/code to run before the AA phase. But by doing that, all of those units that I detailed will become unreachable to the AA guns and the naval bombardment.

So I'm asking for help in determining if AA guns and/or naval bombardment should be able to hit those units that would be removed by RemoveNonCombatants.

I mean I believe you should clarify the "AND" and "OR" relationships amongst the types you mentioned, for a unit to be removed the way you say.

I didn't do that because the logic is actually quite complicated. That's why I linked back to the code so that people could read it. It actually does a negate of multiple negates that are and'ed and or'ed together along with various if/else conditions. I'm not even sure I figured out all of the possibilities.

I'm also much unclear on what you mean by "units in a previous air battle". As far as I know, such units should not be allowed to be part of the normal battle only in case they refused to take part in the air battle, if we are talking about air battles preceding normal combat at all (as opposed to those preceding bombing raids).

You could be completely right. All I know is that the code is excluding units with the flag wasInAirBattle. Those units might not ever show up in these battles and this could be old code that is still left around. But I don't know.

Regarding "transported air units that can land on carriers", this is actually a wrong behaviour. In v1 you should be able to take allied air units as casualties, if this is what this type is referring to (I'm not a developer, so I can only infer the matter from the descriptions)

It is removing units that are transported AND air AND can land on carriers. The comment above it refers to allied air on damaged carriers.

It feels wrong to me that "land units in sea battle" are removed immediately after the AA phase (if this is what you mean), as I tend to think land units that are cargo of sea units should not be subjected to AA fire anyway. I would see such a behaviour as a wrong behaviour (bug). For example, if a fighter has AA shots against infantries, I would surely expect infantries that are cargo never being affected by this.

That is the bug I'm trying to fix.

I've no idea what "sea units in land battle on defense" means. As far as I know, there is no legal way for a sea unit to be in a land battle on defence, as sea units can take part in land battle only on the offence (naval bombardment).

No idea either. But the code is removing them if there is ever a possibility.

trevan commented 4 years ago

Another interesting thing is that AA guns can fire every round. So the first round, they can target more units than they can on subsequent rounds. Naval bombardment, though, only fires the first round.

So, I'm thinking that the existing RemoveNonCombatants is fine to be called before the AA phase. But is it also fine to call before the Naval Bombardment phase?

tvleavitt commented 4 years ago

On Mon, Jul 6, 2020 at 2:12 PM trevan notifications@github.com wrote:

@Cernelius https://github.com/Cernelius I'm trying to fix a bug...

I've no idea what "sea units in land battle on defense" means. As far as I know, there is no legal way for a sea unit to be in a land battle on defence, as sea units can take part in land battle only on the offence (naval bombardment).

No idea either. But the code is removing them if there is ever a possibility.

Is there a concept of amphibious units, that are primarily sea, but also land? Could there be an "assault ship" that participates in a land battle on a coastal space? Maybe that's what this is trying to address? -- Thomas Leavitt Internet enabled since 1990

trevan commented 4 years ago

Is there a concept of amphibious units, that are primarily sea, but also land? Could there be an "assault ship" that participates in a land battle on a coastal space? Maybe that's what this is trying to address?

The method that is doing most of this work is called Matches.unitCanBeInBattle. Going through its history, it was first added in 2012 with the comment "Fixing Battle Calc to not include units that are irrelevant to combat. (veqryn)".

Cernelius commented 4 years ago

By intended rules, before v5, aaGun units were supposed to be removed from the battle, but not from the zone of the battle (in theory, they would be removed from the battle and returned to the zone of the battle, but TripleA never actually removes units from the zone of the battle, to start with, thus the formulation I've given), on the "Remove Opening Fire Casualties" step of a battle, that is the step immediately after the "Conduct Opening Fire" step of the battle (this one being the step where the aaGun units, as well as any submarine and sea unit conducting naval bombardment, fire), that is the very first step of a battle after the generation of the battle itself (in theory, the first step would be adding the units to the "battle board", which is what you do when you click on the button to resolve a battle, if TripleA is not starting it automatically, thus the "Conduct Opening Fire" would be the second step and the "Remove Opening Fire Casualties" would be the third step).

In practice, what you are supposed to do in the "Remove Opening Fire Casualties" step, among other things, is to remove the aaGun units from the battle board and return them to the game board (obviously, inside the zone of the current battle). From that point on, such units are not any longer in the battle, and this is why you can never take them as casualties and you don't need to take them as casualties to win, or otherwise end, the battle (it is not that they have any special ability about not being taken as casualty, while being in the battle, it is just that they are never in the battle any longer, whenever that would matter).

CC: @panther2

Cernelius commented 4 years ago

So I'm asking for help in determining if AA guns and/or naval bombardment should be able to hit those units that would be removed by RemoveNonCombatants.

I mean I believe you should clarify the "AND" and "OR" relationships amongst the types you mentioned, for a unit to be removed the way you say.

I didn't do that because the logic is actually quite complicated. That's why I linked back to the code so that people could read it. It actually does a negate of multiple negates that are and'ed and or'ed together along with various if/else conditions. I'm not even sure I figured out all of the possibilities.

@trevan It is very good that someone is (finally) delving into fixing these sort of things. However, non-developers are much impaired in giving any help if I'm right that currently next to nothing of this is actually documented, which I can say it is also a substantial limit to mapmaking, in using all the possibilities, as you are mostly left with having to test them all, after having coded the game file (xml). Obviously, at this point, fully documenting such matters would be a huge effort, so I'm not telling you, or anyone, actually to do it.

trevan commented 4 years ago

By intended rules, before v5, aaGun units were supposed to be removed from the battle, but not from the zone of the battle (in theory, they would be removed from the battle and returned to the zone of the battle, but TripleA never actually removes units from the zone of the battle, to start with, thus the formulation I've given), on the "Remove Opening Fire Casualties" step of a battle, that is the step immediately after the "Conduct Opening Fire" step of the battle (this one being the step where the aaGun units, as well as any submarine and sea unit conducting naval bombardment, fire), that is the very first step of a battle after the generation of the battle itself (in theory, the first step would be adding the units to the "battle board", which is what you do when you click on the button to resolve a battle, if TripleA is not starting it automatically, thus the "Conduct Opening Fire" would be the second step and the "Remove Opening Fire Casualties" would be the third step).

In practice, what you are supposed to do in the "Remove Opening Fire Casualties" step, among other things, is to remove the aaGun units from the battle board and return them to the game board (obviously, inside the zone of the current battle). From that point on, such units are not any longer in the battle, and this is why you can never take them as casualties and you don't need to take them as casualties to win, or otherwise end, the battle (it is not that they have any special ability about not being taken as casualty, while being in the battle, it is just that they are never in the battle any longer, whenever that would matter).

Good to know but unfortunately, that doesn't actually help in this case. I'm talking about removing units before the aaGun fires. So before the "Conduct Opening Fire". Or, in between the aaGun part and the navalBombardment part. The sub part is not part of this as it is already after the existing unit removal code.

The question I'm trying to find an answer for is: if the existing removeNonCombatants code was moved before the aaGun part, would that cause a problem in the battle?

My current belief is no. The reasoning behind that is:

Is there a case where a navalBombardment can target a unit that NO OTHER unit well ever target? Something like a unit that has no defense ability. They are removed from the target list AFTER navalBombardment so it is currently possible for a naval unit to bombard to target these units. If a map does allow this, and players expect it, then I have to change how I fix the original issue (aa guns targeting transported units).

Cernelius commented 4 years ago

@Cernelius I'm trying to fix a bug where transported units are targeted during the AA phase. The fix that I'd like to do is to move the RemoveNonCombatants phase/step/code to run before the AA phase. But by doing that, all of those units that I detailed will become unreachable to the AA guns and the naval bombardment.

So I'm asking for help in determining if AA guns and/or naval bombardment should be able to hit those units that would be removed by RemoveNonCombatants.

isInfrastructure

During a battle, if this option is meant to identify all units that are v1 to v4 aaGun like, these units should be removed from the battle immediately after they have fired their special AA shots. However, since TripleA allows such units to fire also during regular attacks or over multiple combat rounds, the behaviour is expanded beyond the boundaries of the intended rules (for example, TripleA allows you having such a unit making regular defence fire, which would be strictly pointless, as such units would be removed from the battle before that step (what TripleA appears to be doing, instead, is keeping these units in the battle but making them not targetable by regular fire and ending the battle with a win for the other side if only one side has only units of this type left in it)).

units without attack/defense power

This is not a sufficient reason for removing any unit from any battle (you can have powerless fodder). I can see this is probably an additional requirement for removing isInfrastructure from the battle, even though it has no place in the original intended rules.

units that give no support

This is not a sufficient reason for removing any unit from any battle. I can see this is probably an additional requirement for removing isInfrastructure from the battle, even though it has no place in the original intended rules.

units that are not AA for combat

This is not a sufficient reason for removing any unit from any battle. I assume this is probably an additional requirement for removing isInfrastructure from the battle, but I've no idea why this is even mentioned for something happening after AA fire. I would expect this being relevant sometime before AA fire or never, if these units are not actively participating in such a step anyway.

units that have hit their maxRoundsAa

This is something that expands upon the original intended rules. It is reasonable to assume that these units should be normally removed from the battle immediately after they have fired their last.

land units in sea battle (this includes transported land units)

These units (land units being "cargo" assigned to sea units) remain in the battle for as long as the battle lasts, but are removed from the battle as soon as the assigned unit is removed from the battle. However, they should be completely unable to do anything and unable to have any hits assigned. If removing them from the battle is merely a way through which TripleA assures them doing nothing and being not selectable, then they should be removed from the battle as the battle starts (never be actually part of the battle). However, the user should be able to see all assigned cargo, upon selecting casualties among the units to which the cargo is assigned.

sea units in land battle on defense

I can almost assure you there is absolutely no way a sea unit can be defending in a land battle, besides maybe abusing some rules like scrambling (scrambling a sea unit into a land zone) (this would need testing). If anyone knows of a way for a sea unit to be on the defensive side in a land battle, I would be very curious to know it. Obviously, one could trigger a sea unit into a land zone before a battle, but this is not something that I believe anyone should do, and I would expect the game rather to crash.

units that can be captured on entering the territory

I've no idea how this can have anything at all to do with any battle, let alone happening immediately after the AA fire step, if I'm correctly assuming this is referring to the "Global" ability to overtake "friendly neutral" territories and all units in it (which normally actually happens during the Non Combat Move phase only).

transported air units that can land on carriers

This one really needs to be clarified on the meaning, before any non-developer can reliably say anything about it.

units in a previous air battle

If I'm understanding correctly that here we are talking only about units that have participated in air battles before a normal (land or sea) battle, these units should be present normally in the battle if they took part in it and survived (in this case, the unit should simply act normally, for the rest of the battle, once the air battle is over, so there is absolutely no need to track that it has been in the air battle). Instead, if they choose to retreat from the air battle, it is needed to document the implications of it, distinguishing between attackers and defenders, if needed, as I'm not currently aware that they are (I believe the relevant properties are Air Battle Attackers Can Retreat and Air Battle Defenders Can Retreat). In any case, I cannot see good reasons for removing them immediately after AA fire. What I'm saying is that here it is needed to clarify and document whether or not units that retreated during an air battle, before a normal battle, should be able to take part in the normal battle and, if so, what limits they have, due to having retreated from the air battle, if any. Once this is fully clarified, it can, then, be seen whether or not removing any of them immediately after any AA fire, instead of at some other point, may be correct or advisable.

Finally, I want also to point out that isInfrastructure should not be removed from the battle if they are attacking, as they should retain the ability to retreat (for example, relevant for "Feudal Japan").

CC: @panther2

Cernelius commented 4 years ago

The question I'm trying to find an answer for is: if the existing removeNonCombatants code was moved before the aaGun part, would that cause a problem in the battle?

I tend to advice against moving any such thing if the behaviour is not fully and openly documented in English somewhere outside the code. However, fixing this problem might be worth the risk.

Cernelius commented 4 years ago

The question I'm trying to find an answer for is: if the existing removeNonCombatants code was moved before the aaGun part, would that cause a problem in the battle?

I tend to advice against moving any such thing if the behaviour is not fully and openly documented in English somewhere outside the code. However, fixing this problem might be worth the risk.

A very simple example of what I mean: How am I (or any non-developer) supposed even to know which one between what you are calling the navalBombardment or aaGun steps, or whatever, happen before the other one, beside just starting a game and test it? Is this currently documented anywhere for any person that may not be able to go reading the code itself?

Testing a game now (is testing the only way non-developers are supposed to even get to know such things?), I'm seeing that TripleA makes the "AA Fire" happen before the "Naval Bombardment".

I wonder if this is actually correct. Since AA fire happens immediately before normal combat occurs, shouldn't AA fire actually happen after naval bombardment?

Cernelius commented 4 years ago
  • removeNonCombatants is called before the second round when the aaGun might fire again (with maxAaRounds). I don't see why aaGuns should be able to target some units on the first round only and not be able to target those same units on later rounds.

Are you saying that, currently, some units may be removed from the battle immediately after the AA fire on the first combat round, but none of these units can ever be removed this way immediately after the AA fire on the second and following combat rounds? If so, this seems like a possibly major legacy issue (something coded back when AA fire was possible only on the first combat round and never updated to the new possibilities).

As far as my understanding goes, this behaviour of removing "infrastructure" units from the battle seems to be purely to simplify combat display (avoiding keeping around units that have no point staying in the battle any longer) or something of a legacy behaviour, as the original concept of "infrastructure" units in battle was exclusively the "aaGun" unit, that was to be removed from the battle immediately after having fired, while now "infrastructure" units in battle can also have a whole other functions, so the general behaviour of always removing them immediately after the AA fire step cannot be generally applied to them (also since now there is absolutely nothing that makes "infrastructure" units more likely to have AA ability instead of almost any other ability, so there is really no correlation at all between being an "infrastructure" and being an "AA" unit (and in the latest basic games no "AA" unit is actually an "infrastructure")). So, instead of being removed from the battle at a point, now "infrastructure" units seem to be threated as units that have the following two properties:

Again, all I'm saying comes mostly out of testing experience, and I believe all these matters revolving around infrastructures should really get better documentation for map makers, if not for users, unless I'm missing something.

trevan commented 4 years ago

I wonder if this is actually correct. Since AA fire happens immediately before normal combat occurs, shouldn't AA fire actually happen after naval bombardment?

If it isn't correct, that should be moved to a different issue. According to git history, AA has always fired before NavalBombardment. The very first commit (back in 2002) has it that way. You can see it at https://github.com/triplea-game/triplea/commit/317bfa337478dcc0b26cb80211d39e6afcc7d567#diff-895095e078e7763b8b467774750abc45R265.

Are you saying that, currently, some units may be removed from the battle immediately after the AA fire on the first combat round, but none of these units can ever be removed this way immediately after the AA fire on the second and following combat rounds? If so, this seems like a possibly major legacy issue (something coded back when AA fire was possible only on the first combat round and never updated to the new possibilities).

No, I'm saying that the AA units can fire at units on the first round that they can't fire at on subsequent rounds.

trevan commented 4 years ago

I've done more work in trying to understand the code.

Most of the complicated decisions on removing units can be summed up as "remove if infrastructure". And luckily, the aaGuns and the navalBombardment do not target infrastructure either. So that leaves a few conditions on when a unit might be targeted by aaGuns for their first round or by navalBombardment.

For aaGuns, the following units can be targeted by aaGuns on their first round but not subsequent rounds:

For navalBombardment:

Cernelius commented 4 years ago

And luckily, the aaGuns and the navalBombardment do not target infrastructure either.

I strongly disagree. The last time I tested, infrastructures were able to target other infrastructures, during the AA fire step, and I surely believe this should be the case. I've not tested recently, but if an infrastructure air unit attacks a territory with an infrastructure AA unit, the infrastructure air unit should be fired at (and possibly shot down).

Cernelius commented 4 years ago

Are you saying that, currently, some units may be removed from the battle immediately after the AA fire on the first combat round, but none of these units can ever be removed this way immediately after the AA fire on the second and following combat rounds? If so, this seems like a possibly major legacy issue (something coded back when AA fire was possible only on the first combat round and never updated to the new possibilities).

No, I'm saying that the AA units can fire at units on the first round that they can't fire at on subsequent rounds.

Then I'm back having virtually no idea of what you mean or what is the matter at hand.

Cernelius commented 4 years ago

I wonder if this is actually correct. Since AA fire happens immediately before normal combat occurs, shouldn't AA fire actually happen after naval bombardment?

If it isn't correct, that should be moved to a different issue. According to git history, AA has always fired before NavalBombardment. The very first commit (back in 2002) has it that way. You can see it at 317bfa3#diff-895095e078e7763b8b467774750abc45R265.

The point is not even this, really. The point is missing documentation or that I need to be directed where this documentation is. That was an example asking "how is a non-developer supposed to know whether the AA fire happens before naval bombardment or the other way round?". Arguably, this is off topic, as well. I was just trying to make clear how hard I believe is for a non-developer to follow an issue such as this one, and, while trying to make an example about it, I just so happened to notice something that may be actually wrong. That was mostly a side note for @panther2 or anyone deep into the intended rules.

trevan commented 4 years ago

And luckily, the aaGuns and the navalBombardment do not target infrastructure either.

I strongly disagree. The last time I tested, infrastructures were able to target other infrastructures, during the AA fire step, and I surely believe this should be the case. I've not tested recently, but if an infrastructure air unit attacks a territory with an infrastructure AA unit, the infrastructure air unit should be fired at (and possibly shot down).

I'm just going by what the code says. And aaGuns can't target infrastructure units.

I went through the history to see how old this restriction is and I can only go back to a commit from last year (c6c28dd377342b49d8eeb7bdd098ecb6816a3ced). It was during a refactoring of FireAa by @ron-murhammer . It looks like it might have been a copy/paste mistake? @ron-murhammer , can you double check the commit c6c28dd377342b49d8eeb7bdd098ecb6816a3ced and see if line 53 in FireAa (https://github.com/triplea-game/triplea/commit/c6c28dd377342b49d8eeb7bdd098ecb6816a3ced#diff-25aa0626c92230a945be0a2f8cde18c5R59) is correct? I can't find where the attackableUnits were previously limited to be not infrastructure.

And thanks for pointing this out @Cernelius . If this is true, then I can't just move the RemoveNonCombatants step and I'll need to create a new removal step that doesn't remove infrastructure units.

Cernelius commented 4 years ago

And luckily, the aaGuns and the navalBombardment do not target infrastructure either.

I strongly disagree. The last time I tested, infrastructures were able to target other infrastructures, during the AA fire step, and I surely believe this should be the case. I've not tested recently, but if an infrastructure air unit attacks a territory with an infrastructure AA unit, the infrastructure air unit should be fired at (and possibly shot down).

I'm just going by what the code says. And aaGuns can't target infrastructure units.

Can they at least target them if you specifically set targetsAA to the name of a unit that is an infrastructure? I tested this in the past and I believe it worked (as I believe it should, even though it would never actually happen in the basic games, since AA only target air units and none of them are infrastructures).

For example, if you have an "airTransport" unit that is an infrastructure air unit, it should get AA fire before it can paradrop any transported units (this is not part of the basic games, but I believe it should be the correct behaviour, if the game is properly coded).

Cernelius commented 4 years ago

Anyway, what the original rules of pre-v5 say is that what TripleA calls infrastructure units are removed from the battle immediately after the AA fire (but this is obviously just because in those rules this is all that combat infrastructures can do, which is by far not the case in TripleA). Consequently, infrastructures can surely hit other infrastructures during AA fire. The reason why, in the basic games, this never happens is not related to being an infrastructure, but a consequence of the fact that, in such games, the AA fire targets only air units, and none of such units happens to be an infrastructure.

trevan commented 4 years ago

I'm just going by what the code says. And aaGuns can't target infrastructure units.

Can they at least target them if you specifically set targetsAA to the name of a unit that is an infrastructure?

The code removes infrastructure units before it checks the targetsAA. So, as coded, it won't allow that.

Also, even if the infrastructure removal is a bug that was introduced last year, the AA units would only be able to hit the infrastructure units on the first round. All infrastructure units are removed from the target list after the navalBombardment on the first round.

Cernelius commented 4 years ago

I'm just going by what the code says. And aaGuns can't target infrastructure units.

Can they at least target them if you specifically set targetsAA to the name of a unit that is an infrastructure?

The code removes infrastructure units before it checks the targetsAA. So, as coded, it won't allow that.

Also, even if the infrastructure removal is a bug that was introduced last year, the AA units would only be able to hit the infrastructure units on the first round. All infrastructure units are removed from the target list after the navalBombardment on the first round.

Well, shit. I've a game in development (that is currently on hold) that had that working. I guess I'll have to rethink it. I'll do a quick test on it anyway, to see if it fails to works on 2.1, as you say.

Cernelius commented 4 years ago

Yes, I confirm, it seems that units that are infrastructure are removed from the list of targets of AA fire. I can assure you (for having tested it) it didn't work this way in the past and I believe infrastructure units should be hit by AA fire, if they are eligible targets (according to my interpretation of the rules, this should be the case on the first combat round, but, in the moment you expand over to multiple, then I think it would make the only sense to work all the same for each of them).

Since infrastructure are the aaGun units, in the basic rules, the reason no infrastructure is hit by aaGun fire is because aaGun doesn't target other aaGun, but I believe this has nothing to do with being infrastructure (aaGuns cannot be hit by other aaGuns because they can never attack and, even if they could, they would not be hit because they are not eligible targets, which are all air units only), thus I believe that being an infrastructure shouldn't make you unable to be hit by AA fire (it should only make you unable to be hit from normal fire, according to the fact that such units are normally removed before it happens), thus I believe the current behaviour of having infrastructure units removed from the list of targets of AA fire is wrong or at least baseless.

Meaning that I believe that if one has a unit called "A" that has this option:

<option name="isInfrastructure" value="true"/>

And a unit called "B" that has these options (for example):

<option name="maxRoundsAA" value="1"/>
<option name="damageableAA" value="true"/>
<option name="isAAforCombatOnly" value="true"/>
<option name="typeAA" value="whatever"/>
<option name="targetsAA" value="A"/>
<option name="maxAAattacks" value="1"/>
<option name="attackAA" value="1"/>
<option name="mayOverStackAA" value="false"/>

If the unit A attacks a zone occupied by the unit B, the unit A should be targeted by AA fire from B, as neither unit is going to be removed from the battle at least before that step happens.

I've tested that this is not happening (while I'm certain it was happening in the past) and I see the current behaviour as not according to my understanding of the rules.

@ron-murhammer I'm curious why did you remove infrastructures from being possible targets of AA (targeted) fire? I realize that this is a matter subjected to interpretation and irrelevant for the basic games, so nothing that can be clearly called as surely (or "officially") right or wrong one way or the other, as far as I know, but I don't see any particular reasons for not allowing infrastructures to be targeted (a mapmaker can just not have them in list, if not wanted), especially in the moment the behaviour used to be that they were allowed to.

trevan commented 4 years ago

I've tested that this is not happening (while I'm certain it was happening in the past) and I see the current behaviour as not according to my understanding of the rules.

I checked pos2 and it says

isInfrastructure          values: means a unit does not participate in combat, and they will be captured if the attacker is successful

So it does look like the documentation states that infrastructure doesn't participate in combat.

Cernelius commented 4 years ago

I've tested that this is not happening (while I'm certain it was happening in the past) and I see the current behaviour as not according to my understanding of the rules.

I checked pos2 and it says

isInfrastructure          values: means a unit does not participate in combat, and they will be captured if the attacker is successful

So it does look like the documentation states that infrastructure doesn't participate in combat.

The aaGun, for all basic games from v1 to v4, is at least currently an infrastructure and the aaGun is, since always, surely participating in combat. Moreover, since many years, infrastructures having normal "attack" or "defense" values, or both, are also intentionally supported.

Cernelius commented 4 years ago

I wonder if maybe in the past (like 10 years or more before now) the isInfrastructure was an option assigned to the basic factory unit, but not to the basic aaGun unit. That would at least explain going as far as saying "does not participate in combat" (which makes no sense for the aaGun).

I also believe that it is substantially wrong that factory and aaGun units are both "infrastructures", as the behaviour for the first is never to be added to the battle board (thus the pos2 definition would be correct, for it), while the behaviour for the second is to be removed from the battle board immediately after the Opening Fire step of combat. I tend to think they should have been two different options.

Side note, it makes no sense to call an aaGun an infrastructure (it seems that the term was aimed at describing the "factory" only): https://www.oxfordlearnersdictionaries.com/definition/english/infrastructure?q=infrastructure

Cernelius commented 4 years ago

To be clear, I would agree that it should be impossible to target a basic "factory" with AA fire, but I also believe it should be possible to target a basic aaGun with AA fire (if we have an offensive unit that is able to make AA fire and has the aaGun in its list of AA targets (say, you give the "fighter" the ability to make offensive AA fire against the aaGuns)), because the basic aaGun is (still) in the battle, at that moment.

trevan commented 4 years ago

I've tested that this is not happening (while I'm certain it was happening in the past) and I see the current behaviour as not according to my understanding of the rules.

I checked pos2 and it says

isInfrastructure          values: means a unit does not participate in combat, and they will be captured if the attacker is successful

So it does look like the documentation states that infrastructure doesn't participate in combat.

The aaGun, for all basic games from v1 to v4, is at least currently an infrastructure and the aaGun is, since always, surely participating in combat. Moreover, since many years, infrastructures having normal "attack" or "defense" values, or both, are also intentionally supported.

Interesting. I ran a "classic" game in 1.9 and modified the map to give aaGuns attack/defense values. The gun only fired its aa shot once. After that, it did regular firing. Nothing could target it, though. If I remove the attack/defense values, then the gun fires the aa shot once and then is removed from the battle.

So, it looks like in 1.9, only hits during the aa phase could hit an infrastructure unit. In 2.0, that was removed (accidentally?). An infrastructure unit with attack/defense can fire during the regular attack phase but can't be targeted in either 1.9 and 2.0.

To be clear, I would agree that it should be impossible to target a basic "factory" with AA fire, but I also believe it should be possible to target a basic aaGun with AA fire (if we have an offensive unit that is able to make AA fire and has the aaGun in its list of AA targets (say, you give the "fighter" the ability to make offensive AA fire against the aaGuns)), because the basic aaGun is (still) in the battle, at that moment.

Do you need the aaGun to be "isInfrastructure" anymore? I'm not well versed in all of the possible options so I don't know why you would use "isInfrastructure" for a combat unit, but is there options in 2.0 that allow you to do what you want?

trevan commented 4 years ago

Also, is there any map that has canBeCapturedOnEntering units that you have to attack first? I'm not sure if this is possible but it is the last part of the RemoveNonCombatants that I'm not sure about. I've tried creating a scenario where I have to attack canBeCapturedOnEntering units but I haven't been successful so far. I've checked a few maps so far and it appears that canBeCapturedOnEntering is only used as friendly neutrals that join your side upon entering.

Cernelius commented 4 years ago

Also, is there any map that has canBeCapturedOnEntering units that you have to attack first? I'm not sure if this is possible but it is the last part of the RemoveNonCombatants that I'm not sure about.

I really wonder, as well, and I very much doubt anyone is going to answer you here. I guess you are only left with getting it out of the code.

I've tried creating a scenario where I have to attack canBeCapturedOnEntering units but I haven't been successful so far. I've checked a few maps so far and it appears that canBeCapturedOnEntering is only used as friendly neutrals that join your side upon entering.

That's all I believe it should ever do.

Cernelius commented 4 years ago

So, it looks like in 1.9, only hits during the aa phase could hit an infrastructure unit. In 2.0, that was removed (accidentally?). An infrastructure unit with attack/defense can fire during the regular attack phase but can't be targeted in either 1.9 and 2.0.

The 1.9 behaviour, I believe, is the correct one. Even though TripleA allows infrastructures to remain part of the combat past the opening fire step, the fact that such units are normally removed immediately after it should, in my opinion, be interpreted as them being not available as targets only from that point on (thus being an infrastructure should make the unit a non-eligible target for normal fire only).

However, I'm curious whether or not infrastructures were possible AA targets during the second and following combat rounds, in 1.9. Under the rules, they should not, but I think this is just because basic AA only fire during the first combat round, thus I believe it would be advisable that they do, instead.

To be clear, I would agree that it should be impossible to target a basic "factory" with AA fire, but I also believe it should be possible to target a basic aaGun with AA fire (if we have an offensive unit that is able to make AA fire and has the aaGun in its list of AA targets (say, you give the "fighter" the ability to make offensive AA fire against the aaGuns)), because the basic aaGun is (still) in the battle, at that moment.

Do you need the aaGun to be "isInfrastructure" anymore? I'm not well versed in all of the possible options so I don't know why you would use "isInfrastructure" for a combat unit, but is there options in 2.0 that allow you to do what you want?

For the rules sets from v1 to v4 and as far as the basic games only are concerned, the aaGun needs something assuring that it is removed from the battle and returned to the zone of the battle once the opening fire has been done.

trevan commented 4 years ago

However, I'm curious whether or not infrastructures were possible AA targets during the second and following combat rounds, in 1.9. Under the rules, they should not, but I think this is just because basic AA only fire during the first combat round, thus I believe it would be advisable that they do, instead.

If they have an attack/defense value or a support attachment or maxAAAttack == -1 or maxAAAttack >= current round, they would stay around and fire in the AA round and be targetable in the AA round.

Cernelius commented 4 years ago

However, I'm curious whether or not infrastructures were possible AA targets during the second and following combat rounds, in 1.9. Under the rules, they should not, but I think this is just because basic AA only fire during the first combat round, thus I believe it would be advisable that they do, instead.

If they have an attack/defense value or a support attachment or maxAAAttack == -1 or maxAAAttack >= current round, they would stay around and fire in the AA round and be targetable in the AA round.

Assuming that by "AA round" you mean "AA fire step" of any combat round, then I believe 1.9 was both correct and sensible and I wonder why it was changed to the current state. I suggest reverting back to the behaviour you describe ("infrastructure" disabling the unit from being targeted by normal fire but not influencing AA fire in any way). As said, nothing changes for the basic games, one way or the other, as the basic "AA fire" is against air units only and no air units are infrastructures, in those games.

trevan commented 4 years ago

I've looked into this some more and have discovered that the units that are shown in the attack dialog are filtered through the removeNonCombatants method.

So, currently, an AA unit can hit a unit that isn't shown in the attack dialog. The unit will show up in the casualty selection dialogue, though. Same with naval bombardment. This is what happens when an AA unit can hit a transported unit. The attack dialogue only shows the transporter but when the casualty selection dialogue opens, the transporter AND the transported items are selectable.

I think that moving the removeNonCombatants code in front of the aa phase shouldn't introduce any new bugs. If a unit can be targeted, it should already be visible in the attack dialogue. This code hasn't changed in a long time so if there are issues in it, they are pre-existing and aren't affected by the reordering of removeNonCombatants.