MagicStack: fix fizzle removing too much Targets (#4873)

* MagicStack: fix fizzle removing too much Targets

* Fix Dead Ringers

* Clean up useless check

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

@@ -151,9 +151,12 @@ public class GameSimulator {
     }
 
     public Score simulateSpellAbility(SpellAbility origSa) {
-        return simulateSpellAbility(origSa, this.eval);
+        return simulateSpellAbility(origSa, this.eval, true);
     }
-    public Score simulateSpellAbility(SpellAbility origSa, GameStateEvaluator eval) {
+    public Score simulateSpellAbility(SpellAbility origSa, boolean resolve) {
+        return simulateSpellAbility(origSa, this.eval, resolve);
+    }
+    public Score simulateSpellAbility(SpellAbility origSa, GameStateEvaluator eval, boolean resolve) {
         SpellAbility sa;
         if (origSa instanceof LandAbility) {
             Card hostCard = (Card) copier.find(origSa.getHostCard());
@@ -209,9 +212,11 @@ public class GameSimulator {
             }
         }
 
-        // TODO: Support multiple opponents.
+        if (resolve) {
-        Player opponent = aiPlayer.getWeakestOpponent();
+            // TODO: Support multiple opponents.
-        resolveStack(simGame, opponent);
+            Player opponent = aiPlayer.getWeakestOpponent();
+            resolveStack(simGame, opponent);
+        }
 
         // TODO: If this is during combat, before blockers are declared,
         // we should simulate how combat will resolve and evaluate that

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

@@ -114,9 +114,6 @@ public class SpellAbilityCondition extends SpellAbilityVariables {
             if (value.equals("Bargain")) {
                 this.bargain = true;
             }
-            if (value.equals("AllTargetsLegal")) {
-                this.setAllTargetsLegal(true);
-            }
             if (value.equals("AltCost"))
                 this.altCostPaid = true;
 
@@ -215,8 +212,8 @@ public class SpellAbilityCondition extends SpellAbilityVariables {
             }
         }
 
-        if (params.containsKey("ConditionShareAllColors")) {
+        if (params.containsKey("ConditionNoDifferentColors")) {
-            this.setShareAllColors(params.get("ConditionShareAllColors"));
+            this.setNoDifferentColors(params.get("ConditionNoDifferentColors"));
         }
 
         if (params.containsKey("ConditionManaSpent")) {
@@ -305,16 +302,8 @@ public class SpellAbilityCondition extends SpellAbilityVariables {
             }
         }
 
-        if (this.isAllTargetsLegal()) {
+        if (this.getNoDifferentColors() != null) {
-            for (Card c : sa.getTargets().getTargetCards()) {
+            List<Card> tgts = AbilityUtils.getDefinedCards(host, this.getNoDifferentColors(), sa);
-                if (!sa.canTarget(c)) {
-                    return false;
-                }
-            }
-        }
-
-        if (this.getShareAllColors() != null) {
-            List<Card> tgts = AbilityUtils.getDefinedCards(host, this.getShareAllColors(), sa);
             Card first = Iterables.getFirst(tgts, null);
             if (first == null) {
                 return false;

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

@@ -95,8 +95,6 @@ public class SpellAbilityVariables implements Cloneable {
     private boolean blessing = false;
     private boolean solved = false;
 
-    private boolean allTargetsLegal = false;
-
     /** The s is present. */
     private String isPresent = null;
     private String isPresent2 = null;
@@ -137,7 +135,7 @@ public class SpellAbilityVariables implements Cloneable {
     private String lifeAmount = "GE1";
 
     /** The shareAllColors. */
-    private String shareAllColors = null;
+    private String noDifferentColors = null;
 
     /** The mana spent. */
     private String manaSpent = "";
@@ -360,20 +358,6 @@ public class SpellAbilityVariables implements Cloneable {
     protected boolean bargain = false;
     protected boolean foretold = false;
 
-    /**
-     * @return the allTargetsLegal
-     */
-    public boolean isAllTargetsLegal() {
-        return allTargetsLegal;
-    }
-
-    /**
-     * @param allTargets the allTargetsLegal to set
-     */
-    public void setAllTargetsLegal(boolean allTargets) {
-        this.allTargetsLegal = allTargets;
-    }
-
     // IsPresent for Valid battlefield stuff
 
     /**
@@ -554,12 +538,11 @@ public class SpellAbilityVariables implements Cloneable {
 
     public final boolean isSolved() {     return this.solved;  }
 
-    public String getShareAllColors() {
+    public String getNoDifferentColors() {
-        return shareAllColors;
+        return noDifferentColors;
     }
-
+    public void setNoDifferentColors(String noDifferentColors) {
-    public void setShareAllColors(String shareAllColors) {
+        this.noDifferentColors = noDifferentColors;
-        this.shareAllColors = shareAllColors;
     }
 
     /**

forge-game/src/main/java/forge/game/zone/MagicStack.java

@@ -643,71 +643,54 @@ public class MagicStack /* extends MyObservable */ implements Iterable<SpellAbil
         return hasLegalTargeting(sa.getSubAbility());
     }
 
-    private final boolean hasFizzled(final SpellAbility sa, final Card source, final Boolean parentFizzled) {
+    private final boolean hasFizzled(final SpellAbility sa, final Card source, Boolean fizzle) {
-        // Can't fizzle unless there are some targets
+        List<GameObject> toRemove = Lists.newArrayList();
-        Boolean fizzle = null;
+        if (sa.usesTargeting() && !sa.isZeroTargets()) {
-        boolean rememberTgt = sa.getRootAbility().hasParam("RememberOriginalTargets");
+            if (fizzle == null) {
-
+                // don't overwrite previous result
-        if (sa.usesTargeting()) {
+                fizzle = true;
-            if (sa.isZeroTargets()) {
+            }
-                // Nothing targeted, and nothing needs to be targeted.
+            // Some targets were chosen, fizzling for this subability is now possible
-            } else {
+            // With multi-targets, as long as one target is still legal,
-                // Some targets were chosen, fizzling for this subability is now possible
+            // we'll try to go through as much as possible
-                //fizzle = true;
+            for (final GameObject o : sa.getTargets()) {
-                // With multi-targets, as long as one target is still legal,
+                boolean invalidTarget = false;
-                // we'll try to go through as much as possible
+                if (o instanceof Card) {
-                final TargetChoices choices = sa.getTargets();
+                    final Card card = (Card) o;
-                for (final GameObject o : Lists.newArrayList(sa.getTargets())) {
+                    Card current = game.getCardState(card);
-                    boolean invalidTarget = false;
+                    if (current != null) {
-                    if (rememberTgt) {
+                        invalidTarget = !current.equalsWithGameTimestamp(card);
-                        source.addRemembered(o);
-                    }
-                    if (o instanceof Card) {
-                        final Card card = (Card) o;
-                        Card current = game.getCardState(card);
-                        if (current != null) {
-                            invalidTarget = !current.equalsWithGameTimestamp(card);
-                        }
-                        invalidTarget = invalidTarget || !sa.canTarget(card);
-                    } else if (o instanceof SpellAbility) {
-                        SpellAbilityStackInstance si = getInstanceMatchingSpellAbilityID((SpellAbility)o);
-                        invalidTarget = si == null ? true : !sa.canTarget(si.getSpellAbility());
-                    } else {
-                        invalidTarget = !sa.canTarget(o);
-                    }
-                    // TODO remove targets only after the Loop
-                    // Remove targets
-                    if (invalidTarget) {
-                        choices.remove(o);
-                    } else {
-                        fizzle = false;
-                    }
-
-                    if (sa.hasParam("CantFizzle")) {
-                        // Gilded Drake cannot be countered by rules if the
-                        // targeted card is not valid
-                        fizzle = false;
                     }
+                    invalidTarget = invalidTarget || !sa.canTarget(card);
+                } else if (o instanceof SpellAbility) {
+                    SpellAbilityStackInstance si = getInstanceMatchingSpellAbilityID((SpellAbility)o);
+                    invalidTarget = si == null ? true : !sa.canTarget(si.getSpellAbility());
+                } else {
+                    invalidTarget = !sa.canTarget(o);
                 }
-                if (fizzle == null) {
+
-                    fizzle = true;
+                if (invalidTarget) {
+                    toRemove.add(o);
+                } else {
+                    fizzle = false;
+                }
+
+                if (sa.hasParam("CantFizzle")) {
+                    // Gilded Drake cannot be countered by rules if the
+                    // targeted card is not valid
+                    fizzle = false;
                 }
             }
         }
-        else if (sa.getTargetCard() != null) {
+        if (sa.getSubAbility() != null) {
-            fizzle = !sa.canTarget(sa.getTargetCard());
+            fizzle = hasFizzled(sa.getSubAbility(), source, fizzle);
-        } else {
-            // Set fizzle to the same as the parent if there's no target info
-            fizzle = parentFizzled;
         }
 
-        if (sa.getSubAbility() == null) {
+        // Remove targets
-            if (fizzle != null && fizzle && rememberTgt) {
+        if (sa.usesTargeting() && !sa.isZeroTargets()) {
-                source.clearRemembered();
+            sa.getTargets().removeAll(toRemove);
-            }
-            return fizzle != null && fizzle.booleanValue();
         }
-        return hasFizzled(sa.getSubAbility(), source, fizzle) && (fizzle == null || fizzle.booleanValue());
+        return fizzle != null && fizzle;
     }
 
     public final SpellAbilityStackInstance peek() {

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

@@ -2506,4 +2506,34 @@ public class GameSimulationTest extends SimulationTest {
         AssertJUnit.assertTrue(transformedHeliodToken.isTransformed());
         AssertJUnit.assertTrue(transformedHeliodToken.isBackSide());
     }
+
+    @Test
+    public void testBasicSpellFizzling() {
+        Game game = initAndCreateGame();
+        Player p = game.getPlayers().get(0);
+        game.getPhaseHandler().devModeSet(PhaseType.MAIN2, p);
+
+        addCardToZone("Swamp", p, ZoneType.Library);
+        Card bear = addCard("Bear Cub", p);
+
+        addCards("Swamp", 5, p);
+        Card destroy = addCardToZone("Annihilate", p, ZoneType.Hand);
+        SpellAbility destroySA = destroy.getFirstSpellAbility();
+        destroySA.getTargets().add(bear);
+
+        addCards("Island", 2, p);
+        Card fizzle = addCardToZone("Mage's Guile", p, ZoneType.Hand);
+        SpellAbility fizzleSA = fizzle.getFirstSpellAbility();
+        fizzleSA.getTargets().add(bear);
+
+        GameSimulator sim = createSimulator(game, p);
+        game = sim.getSimulatedGameState();
+
+        sim.simulateSpellAbility(destroySA, false);
+        AssertJUnit.assertEquals(1, game.getStackZone().size());
+        sim.simulateSpellAbility(fizzleSA);
+
+        // spell should fizzle so no card was drawn
+        AssertJUnit.assertEquals(0, game.getPlayers().get(0).getCardsIn(ZoneType.Hand).size());
+    }
 }

forge-gui/res/cardsfolder/d/dead_ringers.txt

@@ -1,7 +1,6 @@
 Name:Dead Ringers
 ManaCost:4 B
 Types:Sorcery
-A:SP$ Destroy | Cost$ 4 B | TargetMin$ 2 | TargetMax$ 2 | NoRegen$ True | ValidTgts$ Creature.nonBlack | TgtPrompt$ Select target nonblack creatures | RememberOriginalTargets$ True | SubAbility$ DBCleanup | ConditionShareAllColors$ DirectRemembered | SpellDescription$ Destroy two target nonblack creatures unless either one is a color the other isn't. They can't be regenerated.
+A:SP$ Destroy | TargetMin$ 2 | TargetMax$ 2 | NoRegen$ True | ValidTgts$ Creature.nonBlack | TgtPrompt$ Select target nonblack creatures | ConditionNoDifferentColors$ Targeted | ConditionDefined$ Targeted | ConditionPresent$ Card | ConditionPresentCompare$ EQ2 | SpellDescription$ Destroy two target nonblack creatures unless either one is a color the other isn't. They can't be regenerated.
-SVar:DBCleanup:DB$ Cleanup | ClearRemembered$ True
 AI:RemoveDeck:All
 Oracle:Destroy two target nonblack creatures unless either one is a color the other isn't. They can't be regenerated.

forge-gui/res/cardsfolder/g/goblin_welder.txt

@@ -2,8 +2,8 @@ Name:Goblin Welder
 ManaCost:R
 Types:Creature Goblin Artificer
 PT:1/1
-A:AB$ Pump | Cost$ T | ValidTgts$ Artifact | TgtPrompt$ Select target artifact a player controls | RememberObjects$ ThisTargetedCard | Condition$ AllTargetsLegal | SubAbility$ DBTargetYard | StackDescription$ If both targets are still legal as this ability resolves, {p:TargetedController} simultaneously sacrifices {c:ThisTargetedCard} | SpellDescription$ Choose target artifact a player controls and target artifact card in that player's graveyard. If both targets are still legal as this ability resolves, that player simultaneously sacrifices the artifact and returns the artifact card to the battlefield.
+A:AB$ Pump | Cost$ T | ValidTgts$ Artifact | TgtPrompt$ Select target artifact a player controls | RememberObjects$ ThisTargetedCard | SubAbility$ DBTargetYard | StackDescription$ If both targets are still legal as this ability resolves, {p:TargetedController} simultaneously sacrifices {c:ThisTargetedCard} | SpellDescription$ Choose target artifact a player controls and target artifact card in that player's graveyard. If both targets are still legal as this ability resolves, that player simultaneously sacrifices the artifact and returns the artifact card to the battlefield.
-SVar:DBTargetYard:DB$ Pump | ValidTgts$ Artifact | TargetsWithDefinedController$ ParentTargetedController | TgtPrompt$ Select target artifact card in that player's graveyard | TgtZone$ Graveyard | PumpZone$ Graveyard | ImprintCards$ ThisTargetedCard | Condition$ AllTargetsLegal | StackDescription$ and returns {c:ThisTargetedCard} to the battlefield. | SubAbility$ DBBranch
+SVar:DBTargetYard:DB$ Pump | ValidTgts$ Artifact | TargetsWithDefinedController$ ParentTargetedController | TgtPrompt$ Select target artifact card in that player's graveyard | TgtZone$ Graveyard | PumpZone$ Graveyard | ImprintCards$ ThisTargetedCard | StackDescription$ and returns {c:ThisTargetedCard} to the battlefield. | SubAbility$ DBBranch
 SVar:DBBranch:DB$ Branch | BranchConditionSVar$ TargetCheck | BranchConditionSVarCompare$ GE2 | TrueSubAbility$ DBSacrifice | FalseSubAbility$ DBCleanup
 SVar:DBSacrifice:DB$ SacrificeAll | ValidCards$ Card.IsRemembered | SubAbility$ DBReturn
 SVar:DBReturn:DB$ ChangeZone | Defined$ Imprinted | Origin$ Graveyard | Destination$ Battlefield | SubAbility$ DBCleanup

forge-gui/res/cardsfolder/t/the_fall_of_kroog.txt

@@ -1,10 +1,9 @@
 Name:The Fall of Kroog
 ManaCost:4 R R
 Types:Sorcery
-A:SP$ Pump | ValidTgts$ Opponent | SubAbility$ DBDestroy | StackDescription$ SpellDescription | RememberOriginalTargets$ True | SpellDescription$ Choose target opponent. Destroy target land that player controls. CARDNAME deals 3 damage to that player and 1 damage to each creature they control.
+A:SP$ Pump | ValidTgts$ Opponent | SubAbility$ DBDestroy | StackDescription$ SpellDescription | SpellDescription$ Choose target opponent. Destroy target land that player controls. CARDNAME deals 3 damage to that player and 1 damage to each creature they control.
-SVar:DBDestroy:DB$ Destroy | ValidTgts$ Land.ControlledBy ParentTarget,Land.ControlledBy Remembered | TgtPrompt$ Select target land that player controls | SubAbility$ DBDealDamage
+SVar:DBDestroy:DB$ Destroy | ValidTgts$ Land.ControlledBy ParentTarget | TgtPrompt$ Select target land that player controls | SubAbility$ DBDealDamage
 SVar:DBDealDamage:DB$ DealDamage | Defined$ TargetedPlayer | NumDmg$ 3 | SubAbility$ DBDamageAll | DamageMap$ True | StackDescription$ None
 SVar:DBDamageAll:DB$ DamageAll | ValidCards$ Creature.ControlledBy TargetedPlayer | NumDmg$ 1 | SubAbility$ DBDamageResolve
-SVar:DBDamageResolve:DB$ DamageResolve | SubAbility$ DBCleanup
+SVar:DBDamageResolve:DB$ DamageResolve
-SVar:DBCleanup:DB$ Cleanup | ClearRemembered$ True
 Oracle:Choose target opponent. Destroy target land that player controls. The Fall of Kroog deals 3 damage to that player and 1 damage to each creature they control.

forge-gui/res/cardsfolder/y/yosei_the_morning_star.txt

@@ -4,6 +4,6 @@ Types:Legendary Creature Dragon Spirit
 PT:5/5
 K:Flying
 T:Mode$ ChangesZone | Origin$ Battlefield | Destination$ Graveyard | ValidCard$ Card.Self | Execute$ TrigSkipPhase | TriggerDescription$ When CARDNAME dies, target player skips their next untap step. Tap up to five target permanents that player controls.
-SVar:TrigSkipPhase:DB$ SkipPhase | ValidTgts$ Player | Step$ Untap | RememberOriginalTargets$ True | IsCurse$ True | SubAbility$ TrigTap
+SVar:TrigSkipPhase:DB$ SkipPhase | ValidTgts$ Player | Step$ Untap | IsCurse$ True | SubAbility$ TrigTap
-SVar:TrigTap:DB$ Tap | TargetMin$ 0 | TargetMax$ 5 | ValidTgts$ Permanent.ControlledBy ParentTarget,Permanent.ControlledBy Remembered
+SVar:TrigTap:DB$ Tap | TargetMin$ 0 | TargetMax$ 5 | ValidTgts$ Permanent.ControlledBy ParentTarget
 Oracle:Flying\nWhen Yosei, the Morning Star dies, target player skips their next untap step. Tap up to five target permanents that player controls.