Merge pull request #2157 from asvitkine/sim_imprv

Simulated AI: Fix land pruning logic and skip invalid targets.

forge-ai/src/main/java/forge/ai/simulation/GameSimulator.java

@@ -198,13 +198,10 @@ public class GameSimulator {
             final SpellAbility playingSa = sa;
 
             simGame.copyLastState();
-            boolean success = ComputerUtil.handlePlayingSpellAbility(aiPlayer, sa, simGame, new Runnable() {
+            boolean success = ComputerUtil.handlePlayingSpellAbility(aiPlayer, sa, simGame, () -> {
-                @Override
+                if (interceptor != null) {
-                public void run() {
+                    interceptor.announceX(playingSa);
-                    if (interceptor != null) {
+                    interceptor.chooseTargets(playingSa, GameSimulator.this);
-                        interceptor.announceX(playingSa);
-                        interceptor.chooseTargets(playingSa, GameSimulator.this);
-                    }
                 }
             });
             if (!success) {

forge-ai/src/main/java/forge/ai/simulation/SpellAbilityChoicesIterator.java

@@ -1,5 +1,6 @@
 package forge.ai.simulation;
 
+import forge.ai.ComputerUtilAbility;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -16,7 +17,7 @@ import forge.game.spellability.AbilitySub;
 import forge.game.spellability.SpellAbility;
 
 public class SpellAbilityChoicesIterator {
-    private SimulationController controller;
+    private final SimulationController controller;
 
     private Iterator<int[]> modeIterator;
     private int[] selectedModes;
@@ -34,11 +35,13 @@ public class SpellAbilityChoicesIterator {
         Card selectedChoice;
         Score bestScoreForChoice = new Score(Integer.MIN_VALUE);
     }
-    private ArrayList<ChoicePoint> choicePoints = new ArrayList<>();
+    private final ArrayList<ChoicePoint> choicePoints = new ArrayList<>();
     private int incrementedCpIndex = 0;
     private int cpIndex = -1;
 
     private int evalDepth;
+    // Maps from filtered mode indexes to original ones.
+    private List<Integer> modesMap;
 
     public SpellAbilityChoicesIterator(SimulationController controller) {
         this.controller = controller;
@@ -46,17 +49,28 @@ public class SpellAbilityChoicesIterator {
 
     public List<AbilitySub> chooseModesForAbility(List<AbilitySub> choices, int min, int num, boolean allowRepeat) {
         if (modeIterator == null) {
-            // TODO: Need to skip modes that are invalid (e.g. targets don't exist)!
+            // Skip modes that don't have legal targets.
+            modesMap = new ArrayList<>();
+            int origIndex = -1;
+            for (AbilitySub sub : choices) {
+                origIndex++;
+                if (!ComputerUtilAbility.isFullyTargetable(sub)) {
+                    continue;
+                }
+                modesMap.add(origIndex);
+            }
             // TODO: Do we need to do something special to support cards that have extra costs
             // when choosing more modes, like Blessed Alliance?
-            if (!allowRepeat) {
+            if (modesMap.isEmpty()) {
-                modeIterator = CombinatoricsUtils.combinationsIterator(choices.size(), num);
+                return null;
+            } else if (!allowRepeat) {
+                modeIterator = CombinatoricsUtils.combinationsIterator(modesMap.size(), num);
             } else {
                 // Note: When allowRepeat is true, it does result in many possibilities being tried.
                 // We should ideally prune some of those at a higher level.
-                modeIterator = new AllowRepeatModesIterator(choices.size(), min, num);
+                modeIterator = new AllowRepeatModesIterator(modesMap.size(), min, num);
             }
-            selectedModes = modeIterator.next();
+            selectedModes = remapModes(modeIterator.next());
             advancedToNextMode = true;
         }
         // Note: If modeIterator already existed, selectedModes would have been updated in advance().
@@ -78,6 +92,13 @@ public class SpellAbilityChoicesIterator {
         return result;
     }
 
+    private int[] remapModes(int[] modes) {
+        for (int i = 0; i < modes.length; i++) {
+            modes[i] = modesMap.get(modes[i]);
+        }
+        return modes;
+    }
+
     public Card chooseCard(CardCollection fetchList) {
         cpIndex++;
         if (cpIndex >= choicePoints.size()) {
@@ -86,8 +107,7 @@ public class SpellAbilityChoicesIterator {
         ChoicePoint cp = choicePoints.get(cpIndex);
         // Prune duplicates.
         HashSet<String> uniqueCards = new HashSet<>();
-        for (int i = 0; i < fetchList.size(); i++) {
+        for (Card card : fetchList) {
-            Card card = fetchList.get(i);
             if (uniqueCards.add(card.getName()) && uniqueCards.size() == cp.nextChoice + 1) {
                 cp.selectedChoice = card;
             }
@@ -137,14 +157,9 @@ public class SpellAbilityChoicesIterator {
                 evalDepth++;
                 pushTarget = false;
             }
-            return;
         }
     }
 
-    public int[] getSelectModes() {
-        return selectedModes;
-    }
-
     public boolean advance(Score lastScore) {
         cpIndex = -1;
         for (ChoicePoint cp : choicePoints) {
@@ -195,7 +210,7 @@ public class SpellAbilityChoicesIterator {
             doneEvaluating(bestScoreForMode);
             bestScoreForMode = new Score(Integer.MIN_VALUE);
             if (modeIterator.hasNext()) {
-                selectedModes = modeIterator.next();
+                selectedModes = remapModes(modeIterator.next());
                 advancedToNextMode = true;
                 return true;
             }
@@ -232,8 +247,8 @@ public class SpellAbilityChoicesIterator {
     }
 
     private static class AllowRepeatModesIterator implements Iterator<int[]> {
-        private int numChoices;
+        private final int numChoices;
-        private int max;
+        private final int max;
         private int[] indexes;
 
         public AllowRepeatModesIterator(int numChoices, int min, int max) {

forge-ai/src/main/java/forge/ai/simulation/SpellAbilityPicker.java

@@ -39,6 +39,7 @@ public class SpellAbilityPicker {
     private SpellAbilityChoicesIterator interceptor;
 
     private Plan plan;
+    private int numSimulations;
 
     public SpellAbilityPicker(Game game, Player player) {
         this.game = game;
@@ -66,19 +67,7 @@ public class SpellAbilityPicker {
     private List<SpellAbility> getCandidateSpellsAndAbilities() {
         CardCollection cards = ComputerUtilAbility.getAvailableCards(game, player);
         List<SpellAbility> all = ComputerUtilAbility.getSpellAbilities(cards, player);
-        CardCollection landsToPlay = ComputerUtilAbility.getAvailableLandsToPlay(game, player);
+        HashMap<String, Card> landsDeDupe = new HashMap<>();
-        if (landsToPlay != null) {
-            HashMap<String, Card> landsDeDupe = new HashMap<>();
-            for (Card land : landsToPlay) {
-                Card previousLand = landsDeDupe.get(land.getName());
-                // Skip identical lands.
-                if (previousLand != null && previousLand.getZone() == land.getZone() && previousLand.getOwner() == land.getOwner()) {
-                    continue;
-                }
-                landsDeDupe.put(land.getName(), land);
-                all.add(new LandAbility(land));
-            }
-        }
         List<SpellAbility> candidateSAs = ComputerUtilAbility.getOriginalAndAltCostAbilities(all, player);
         int writeIndex = 0;
         for (int i = 0; i < candidateSAs.size(); i++) {
@@ -86,6 +75,16 @@ public class SpellAbilityPicker {
             if (sa.isManaAbility()) {
                 continue;
             }
+            // Skip identical lands.
+            if (sa instanceof LandAbility) {
+                Card land = sa.getHostCard();
+                Card previousLand = landsDeDupe.get(sa.getHostCard().getName());
+                if (previousLand != null && previousLand.getZone() == land.getZone() &&
+                    previousLand.getOwner() == land.getOwner()) {
+                    continue;
+                }
+                landsDeDupe.put(land.getName(), land);
+            }
             sa.setActivatingPlayer(player, true);
 
             AiPlayDecision opinion = canPlayAndPayForSim(sa);
@@ -95,7 +94,7 @@ public class SpellAbilityPicker {
 
             if (opinion != AiPlayDecision.WillPlay)
                 continue;
-            candidateSAs.set(writeIndex,  sa);
+            candidateSAs.set(writeIndex, sa);
             writeIndex++;
         }
         candidateSAs.subList(writeIndex, candidateSAs.size()).clear();
@@ -353,7 +352,9 @@ public class SpellAbilityPicker {
         if (!ComputerUtilCost.canPayCost(sa, player, sa.isTrigger())) {
             return AiPlayDecision.CantAfford;
         }
-
+        if (!ComputerUtilAbility.isFullyTargetable(sa)) {
+            return AiPlayDecision.TargetingFailed;
+        }
         if (shouldWaitForLater(sa)) {
             return AiPlayDecision.AnotherTime;
         }
@@ -375,10 +376,13 @@ public class SpellAbilityPicker {
         final SpellAbilityChoicesIterator choicesIterator = new SpellAbilityChoicesIterator(controller);
         Score lastScore;
         do {
+            // TODO: MyRandom should be an instance on the game object, so that we could do
+            // simulations in parallel without messing up global state.
             MyRandom.setRandom(new Random(randomSeedToUse));
             GameSimulator simulator = new GameSimulator(controller, game, player, phase);
             simulator.setInterceptor(choicesIterator);
             lastScore = simulator.simulateSpellAbility(sa);
+            numSimulations++;
             if (lastScore.value > bestScore.value) {
                 bestScore = lastScore;
             }
@@ -462,4 +466,8 @@ public class SpellAbilityPicker {
         }
         return ComputerUtil.chooseSacrificeType(player, type, ability, ability.getTargetCard(), effect, amount, exclude);
     }
+
+    public int getNumSimulations() {
+        return numSimulations;
+    }
 }

forge-game/src/main/java/forge/game/spellability/SpellAbility.java

@@ -1254,10 +1254,16 @@ public abstract class SpellAbility extends CardTraitBase implements ISpellAbilit
             return false;
         }
 
-        final TargetRestrictions tr = getTargetRestrictions();
+        final SpellAbility rootAbility = this.getRootAbility();
+        // 115.5. A spell or ability on the stack is an illegal target for itself.
+        // (This covers the spell case.)
+        if (rootAbility.isSpell() && rootAbility.getHostCard() == entity) {
+            return false;
+        }
 
         // Restriction related to this ability
         if (usesTargeting()) {
+            final TargetRestrictions tr = getTargetRestrictions();
             if (tr.isUniqueTargets() && getUniqueTargets().contains(entity))
                 return false;
 

forge-gui-desktop/src/test/java/forge/ai/simulation/SpellAbilityPickerSimulationTest.java

@@ -483,4 +483,126 @@ public class SpellAbilityPickerSimulationTest extends SimulationTest {
         AssertJUnit.assertEquals("Chaos Warp", sa.getHostCard().getName());
         AssertJUnit.assertEquals(expectedTarget, sa.getTargetCard());
     }
+
+    @Test
+    public void testNoSimulationsWhenNoTargets() {
+        Game game = initAndCreateGame();
+        Player p = game.getPlayers().get(1);
+
+        addCard("Island", p);
+        addCard("Island", p);
+        addCardToZone("Counterspell", p, ZoneType.Hand);
+        addCardToZone("Unsummon", p, ZoneType.Hand);
+
+        game.getPhaseHandler().devModeSet(PhaseType.MAIN2, p);
+        game.getAction().checkStateEffects(true);
+
+        SpellAbilityPicker picker = new SpellAbilityPicker(game, p);
+        SpellAbility sa = picker.chooseSpellAbilityToPlay(null);
+        AssertJUnit.assertNull(sa);
+        AssertJUnit.assertEquals(0, picker.getNumSimulations());
+    }
+
+    @Test
+    public void testLandDropPruning() {
+        Game game = initAndCreateGame();
+        Player p = game.getPlayers().get(1);
+
+        addCardToZone("Island", p, ZoneType.Hand);
+        addCardToZone("Island", p, ZoneType.Hand);
+        addCardToZone("Island", p, ZoneType.Hand);
+
+        game.getPhaseHandler().devModeSet(PhaseType.MAIN2, p);
+        game.getAction().checkStateEffects(true);
+
+        SpellAbilityPicker picker = new SpellAbilityPicker(game, p);
+        SpellAbility sa = picker.chooseSpellAbilityToPlay(null);
+        AssertJUnit.assertNotNull(sa);
+        // Only one land drop should be simulated, since the cards are identical.
+        AssertJUnit.assertEquals(1, picker.getNumSimulations());
+    }
+
+    @Test
+    public void testSpellCantTargetSelf() {
+        Game game = initAndCreateGame();
+        Player p = game.getPlayers().get(1);
+        Player opponent = game.getPlayers().get(0);
+
+        addCardToZone("Unsubstantiate", p, ZoneType.Hand);
+        addCard("Forest", p);
+        addCard("Island", p);
+        Card expectedTarget = addCard("Flying Men", opponent);
+
+        game.getPhaseHandler().devModeSet(PhaseType.MAIN2, p);
+        game.getAction().checkStateEffects(true);
+
+        SpellAbilityPicker picker = new SpellAbilityPicker(game, p);
+        SpellAbility sa = picker.chooseSpellAbilityToPlay(null);
+        AssertJUnit.assertNotNull(sa);
+        AssertJUnit.assertEquals(expectedTarget, sa.getTargetCard());
+        // Only a single simulation expected (no target self).
+        AssertJUnit.assertEquals(1, picker.getNumSimulations());
+    }
+
+    @Test
+    public void testModalSpellCantTargetSelf() {
+        Game game = initAndCreateGame();
+        Player p = game.getPlayers().get(1);
+        Player opponent = game.getPlayers().get(0);
+
+        addCardToZone("Decisive Denial", p, ZoneType.Hand);
+        addCard("Forest", p);
+        addCard("Island", p);
+        addCard("Runeclaw Bear", p);
+        addCard("Flying Men", opponent);
+
+        game.getPhaseHandler().devModeSet(PhaseType.MAIN2, p);
+        game.getAction().checkStateEffects(true);
+
+        SpellAbilityPicker picker = new SpellAbilityPicker(game, p);
+        SpellAbility sa = picker.chooseSpellAbilityToPlay(null);
+        AssertJUnit.assertNotNull(sa);
+        // Expected: Runeclaw Bear fights Flying Men
+        // Only a single simulation expected (no target self).
+        AssertJUnit.assertEquals(1, picker.getNumSimulations());
+    }
+
+    @Test
+    public void testModalSpellNoTargetsForModeWithSubAbility() {
+        Game game = initAndCreateGame();
+        Player p = game.getPlayers().get(1);
+
+        addCardToZone("Temur Charm", p, ZoneType.Hand);
+        addCard("Forest", p);
+        addCard("Island", p);
+        addCard("Mountain", p);
+
+        game.getPhaseHandler().devModeSet(PhaseType.MAIN2, p);
+        game.getAction().checkStateEffects(true);
+
+        SpellAbilityPicker picker = new SpellAbilityPicker(game, p);
+         picker.chooseSpellAbilityToPlay(null);
+        // Only mode "Creatures with power 3 or less can’t block this turn" should be simulated.
+        AssertJUnit.assertEquals(1, picker.getNumSimulations());
+    }
+
+    @Test
+    public void testModalSpellNoTargetsForAnyModes() {
+        Game game = initAndCreateGame();
+        Player p = game.getPlayers().get(1);
+
+        addCardToZone("Drown in the Loch", p, ZoneType.Hand);
+        addCard("Swamp", p);
+        addCard("Island", p);
+
+        game.getPhaseHandler().devModeSet(PhaseType.MAIN2, p);
+        game.getAction().checkStateEffects(true);
+
+        SpellAbilityPicker picker = new SpellAbilityPicker(game, p);
+        picker.chooseSpellAbilityToPlay(null);
+        // TODO: Ideally, this would be 0 simulations, but we currently only determine there are no
+        // valid modes in SpellAbilityChoicesIterator, which runs already when we're simulating.
+        // Still, this test case exercises the code path and ensures we don't crash in this case.
+        AssertJUnit.assertEquals(1, picker.getNumSimulations());
+    }
 }