Undo placement bug

[DanVanAtta]

How can the problem be recreated?

(Possible repro, to be verified)

Repro

• Purchase a carrier and a fighter (with v2 rules)
• Place fighter onto factory
• Place carrier adjacent to factory
• Move fighter onto carrier per prompt
• Undo fighter placement

Notice:

• stack trace
• fighter undo placement disappears

Game Version: 2.5

Is there anything else we should know?

Stack trace:

(4:31:01 p.m.) Bot103_Dallas:  WARNING: Remote method call exception: Not all units present in:Greece.  Trying to remove:[Adv.Fighter owned by Italians] present:[AAGun owned by Italians, Factory owned by Italians, Fighter owned ...
(4:31:01 p.m.) Bot103_Dallas:  java.lang.IllegalStateException: Not all units present in:Greece.  Trying to remove:[Adv.Fighter owned by Italians] present:[AAGun owned by Italians, Factory owned by Italians, Fighter owned by Ita...
(4:31:01 p.m.) Bot103_Dallas:  	at games.strategy.engine.data.changefactory.RemoveUnits.perform(RemoveUnits.java:44)
(4:31:01 p.m.) Bot103_Dallas:  	at games.strategy.engine.data.CompositeChange.perform(CompositeChange.java:48)
(4:31:01 p.m.) Bot103_Dallas:  	at games.strategy.engine.data.GameData.performChange(GameData.java:433)
(4:31:01 p.m.) Bot103_Dallas:  	at games.strategy.engine.framework.ServerGame$1.gameDataChanged(ServerGame.java:92)
(4:31:01 p.m.) Bot103_Dallas:  	at jdk.internal.reflect.GeneratedMethodAccessor1119.invoke(Unknown Source)
(4:31:01 p.m.) Bot103_Dallas:  	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
(4:31:01 p.m.) Bot103_Dallas:  	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
(4:31:01 p.m.) Bot103_Dallas:  	at games.strategy.engine.message.unifiedmessenger.EndPoint.invokeSingle(EndPoint.java:136)
(4:31:01 p.m.) Bot103_Dallas:  	at games.strategy.engine.message.unifiedmessenger.EndPoint.lambda$invokeMultiple$0(EndPoint.java:120)
(4:31:01 p.m.) Bot103_Dallas:  	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
(4:31:01 p.m.) Bot103_Dallas:  	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
(4:31:01 p.m.) Bot103_Dallas:  	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
(4:31:01 p.m.) Bot103_Dallas:  	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
(4:31:01 p.m.) Bot103_Dallas:  	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
(4:31:01 p.m.) Bot103_Dallas:  	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
(4:31:01 p.m.) Bot103_Dallas:  	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
(4:31:01 p.m.) Bot103_Dallas:  	at games.strategy.engine.message.unifiedmessenger.EndPoint.invokeMultiple(EndPoint.java:121)

[Cernelius]

In v1, you cannot redeploy anywhere fighters you just placed into land zones. Redeploying them on carriers is typical of basic v2 (excluding LHTR).

[Cernelius]

For consistency with how you place sea units, I would say that the process should always be to place the fighter directly on the carrier, instead of placing it on the land territory first. However, this raise the general issue that the program doesn't allow you to define what territory is placing the unit, when placing units on sea zones next to two or more land territories that would allow such action.

[Cernelius]

For consistency with how you place sea units, I would say that the process should always be to place the fighter directly on the carrier, instead of placing it on the land territory first. However, this raise the general issue that the program doesn't allow you to define what territory is placing the unit, when placing units on sea zones next to two or more land territories that would allow such action.

This means disabling newly placed fighters from being possibly redeployed (only those already on the map before the phase would be able to do this). This should fix the problem.

[DanVanAtta]

@Cernelius that's a bigger change, we still need the existing behavior (perhaps it is technically v2 rules, but I think we have determined the rule definitions are actually meaningless, so, Pre-v3 is probably the way to describe it). For example, if you have 2 existing fighters on land, under this rule set when you build a carrier you can move those fighters to the new carrier.

[DanVanAtta]

I updated the OP to specify V2 rules instead of V1. The map is World At War.

[Cernelius]

I updated the OP to specify V2 rules instead of V1. The map is World At War.

Ok, but, if we want to be precise, as said, this is not actually generally "v2" but only non-LHTR v2 (LHTR is v2 too, otherwise v3 would be v4). In the documentation we have, the first "v2" (we could call it "v2.0" and LHTR "v2.1") is called "v2 Basic", so I guess better calling it that way, instead of just "v2". This is because it would be incorrect to think that this matter is not anymore relevant from "v3" onwards, as it is, instead, not anymore relevant from "v2 LHTR" onwards (I'm almost sure that v2 LHTR and v3 are exactly the same set of rules for anything related to this matter).

https://github.com/triplea-game/triplea/blob/master/docs/game-rule-sets.md

---

As said, if we would say that LHTR is not v2 (which we could), we would, then, need to say that v3 is v4, v4 is v5, and so on, since LHTR would become v3. So, I believe, due to the way things has been called, we must take that LHTR is v2 too. Therefore, everytime that something does not apply to both v2, I believe it is necessary to specify it somehow (sadly, I believe there is not an historical name for v2 before LHTR, as it was called just v2, but one can be officially decided, like calling it "v2.0" or "v2 Basic" or something (maybe opening a topic in forum for deciding how to call non-LHTR v2?)).

---

For example, in the game you referenced (World At War) there is an off-as-default option called "LHTR Carrier production rules" that, if enabled, should make this problem inexistent (while the game is still fully v2 rules, though now in a mix of LHTR and non).

[Cernelius]

All the above is merely a suggestion to change, in the OP

(with v2 rules)

with something like

(with non-LHTR v2 rules) (with basic v2 rules) (with v2.0 rules)

[DanVanAtta]

@Cernelius , I don't think the code that dictates the fighters moving to carrier is actually dependent on a rule version, it is instead more likely a specific rule (that would ideally be captured as part of a meaningful and well defined ruleset). The only thing important about mentioning the rule is for better identifying and reproducing the problem, that is the important element here, not terminology

Next steps in this issue:

• confirm repro
• simplify repro steps
• attach a save game
• identify a fix and fix

The error is related to a change object that is looking for a unit that is no longer in that territory. It seems like we need to have a relationship or a dependency between change objects so we can also undo dependent moves or notify the user that a dependent move must be undone first (which would be consistent with other undo, even if not optimal, for example undoing transports that have unloaded gives an error message instead of undoing the dependent moves)

[stale[bot]]

I noticed there has not been much activity on this issue. Please check this issue and close it if it no longer applies. Otherwise to help move this issue move forward, please add a comment summarizing any further actions that need to be taken in order to resolve this issue.

[asvitkine]

I haven't followed the discussion here, but has anyone tested if this is still a problem in 2.6?