From ca362664b7cdc91c14ab6fb06b1e976ae4b68817 Mon Sep 17 00:00:00 2001 From: Hans Mackowiak Date: Wed, 27 Mar 2024 16:03:11 +0100 Subject: [PATCH] MagicStack: fix fizzle removing too much Targets (#4873) * MagicStack: fix fizzle removing too much Targets * Fix Dead Ringers * Clean up useless check --- .../forge/ai/simulation/GameSimulator.java | 15 ++- .../spellability/SpellAbilityCondition.java | 19 +--- .../spellability/SpellAbilityVariables.java | 27 +----- .../main/java/forge/game/zone/MagicStack.java | 97 ++++++++----------- .../ai/simulation/GameSimulationTest.java | 30 ++++++ forge-gui/res/cardsfolder/d/dead_ringers.txt | 3 +- forge-gui/res/cardsfolder/g/goblin_welder.txt | 4 +- .../res/cardsfolder/t/the_fall_of_kroog.txt | 7 +- .../cardsfolder/y/yosei_the_morning_star.txt | 4 +- 9 files changed, 97 insertions(+), 109 deletions(-) diff --git a/forge-ai/src/main/java/forge/ai/simulation/GameSimulator.java b/forge-ai/src/main/java/forge/ai/simulation/GameSimulator.java index 2b6b0f120b9..f58db43146b 100644 --- a/forge-ai/src/main/java/forge/ai/simulation/GameSimulator.java +++ b/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. - Player opponent = aiPlayer.getWeakestOpponent(); - resolveStack(simGame, opponent); + if (resolve) { + // TODO: Support multiple opponents. + 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 diff --git a/forge-game/src/main/java/forge/game/spellability/SpellAbilityCondition.java b/forge-game/src/main/java/forge/game/spellability/SpellAbilityCondition.java index 2eade2a642d..d26b63c03f0 100644 --- a/forge-game/src/main/java/forge/game/spellability/SpellAbilityCondition.java +++ b/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")) { - this.setShareAllColors(params.get("ConditionShareAllColors")); + if (params.containsKey("ConditionNoDifferentColors")) { + this.setNoDifferentColors(params.get("ConditionNoDifferentColors")); } if (params.containsKey("ConditionManaSpent")) { @@ -305,16 +302,8 @@ public class SpellAbilityCondition extends SpellAbilityVariables { } } - if (this.isAllTargetsLegal()) { - for (Card c : sa.getTargets().getTargetCards()) { - if (!sa.canTarget(c)) { - return false; - } - } - } - - if (this.getShareAllColors() != null) { - List tgts = AbilityUtils.getDefinedCards(host, this.getShareAllColors(), sa); + if (this.getNoDifferentColors() != null) { + List tgts = AbilityUtils.getDefinedCards(host, this.getNoDifferentColors(), sa); Card first = Iterables.getFirst(tgts, null); if (first == null) { return false; diff --git a/forge-game/src/main/java/forge/game/spellability/SpellAbilityVariables.java b/forge-game/src/main/java/forge/game/spellability/SpellAbilityVariables.java index 6309180f15b..ac036d55fae 100644 --- a/forge-game/src/main/java/forge/game/spellability/SpellAbilityVariables.java +++ b/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() { - return shareAllColors; + public String getNoDifferentColors() { + return noDifferentColors; } - - public void setShareAllColors(String shareAllColors) { - this.shareAllColors = shareAllColors; + public void setNoDifferentColors(String noDifferentColors) { + this.noDifferentColors = noDifferentColors; } /** diff --git a/forge-game/src/main/java/forge/game/zone/MagicStack.java b/forge-game/src/main/java/forge/game/zone/MagicStack.java index d84808659ca..81cbc993630 100644 --- a/forge-game/src/main/java/forge/game/zone/MagicStack.java +++ b/forge-game/src/main/java/forge/game/zone/MagicStack.java @@ -643,71 +643,54 @@ public class MagicStack /* extends MyObservable */ implements Iterable toRemove = Lists.newArrayList(); + if (sa.usesTargeting() && !sa.isZeroTargets()) { + if (fizzle == null) { + // don't overwrite previous result + fizzle = true; + } + // Some targets were chosen, fizzling for this subability is now possible + // With multi-targets, as long as one target is still legal, + // we'll try to go through as much as possible + for (final GameObject o : sa.getTargets()) { + boolean invalidTarget = false; + 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); } - 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) { - fizzle = !sa.canTarget(sa.getTargetCard()); - } else { - // Set fizzle to the same as the parent if there's no target info - fizzle = parentFizzled; + if (sa.getSubAbility() != null) { + fizzle = hasFizzled(sa.getSubAbility(), source, fizzle); } - if (sa.getSubAbility() == null) { - if (fizzle != null && fizzle && rememberTgt) { - source.clearRemembered(); - } - return fizzle != null && fizzle.booleanValue(); + // Remove targets + if (sa.usesTargeting() && !sa.isZeroTargets()) { + sa.getTargets().removeAll(toRemove); } - return hasFizzled(sa.getSubAbility(), source, fizzle) && (fizzle == null || fizzle.booleanValue()); + return fizzle != null && fizzle; } public final SpellAbilityStackInstance peek() { diff --git a/forge-gui-desktop/src/test/java/forge/ai/simulation/GameSimulationTest.java b/forge-gui-desktop/src/test/java/forge/ai/simulation/GameSimulationTest.java index 8b298b3dc19..4ce1382bf28 100644 --- a/forge-gui-desktop/src/test/java/forge/ai/simulation/GameSimulationTest.java +++ b/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()); + } } diff --git a/forge-gui/res/cardsfolder/d/dead_ringers.txt b/forge-gui/res/cardsfolder/d/dead_ringers.txt index 0ddc39fdedb..b2b8b9d1e3f 100644 --- a/forge-gui/res/cardsfolder/d/dead_ringers.txt +++ b/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. -SVar:DBCleanup:DB$ Cleanup | ClearRemembered$ True +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. AI:RemoveDeck:All Oracle:Destroy two target nonblack creatures unless either one is a color the other isn't. They can't be regenerated. diff --git a/forge-gui/res/cardsfolder/g/goblin_welder.txt b/forge-gui/res/cardsfolder/g/goblin_welder.txt index 6ccf8bd2f07..04954a4e0f1 100644 --- a/forge-gui/res/cardsfolder/g/goblin_welder.txt +++ b/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. -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 +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 | 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 diff --git a/forge-gui/res/cardsfolder/t/the_fall_of_kroog.txt b/forge-gui/res/cardsfolder/t/the_fall_of_kroog.txt index 43ab1be2ad2..de71cb3e5fc 100644 --- a/forge-gui/res/cardsfolder/t/the_fall_of_kroog.txt +++ b/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. -SVar:DBDestroy:DB$ Destroy | ValidTgts$ Land.ControlledBy ParentTarget,Land.ControlledBy Remembered | TgtPrompt$ Select target land that player controls | SubAbility$ DBDealDamage +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 | 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:DBCleanup:DB$ Cleanup | ClearRemembered$ True +SVar:DBDamageResolve:DB$ DamageResolve 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. diff --git a/forge-gui/res/cardsfolder/y/yosei_the_morning_star.txt b/forge-gui/res/cardsfolder/y/yosei_the_morning_star.txt index 8c99cb3b5db..47d0cfa8830 100644 --- a/forge-gui/res/cardsfolder/y/yosei_the_morning_star.txt +++ b/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:TrigTap:DB$ Tap | TargetMin$ 0 | TargetMax$ 5 | ValidTgts$ Permanent.ControlledBy ParentTarget,Permanent.ControlledBy Remembered +SVar:TrigSkipPhase:DB$ SkipPhase | ValidTgts$ Player | Step$ Untap | IsCurse$ True | SubAbility$ TrigTap +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.