From 6b9532febd3e116ec1908440682cd822ba01c5b9 Mon Sep 17 00:00:00 2001 From: Grath <1895280+Grath@users.noreply.github.com> Date: Wed, 25 Dec 2024 14:18:01 -0500 Subject: [PATCH] [refactor/bugfix] use rule 802.2a where appropriate. (#13179) * [refactor/bugfix] use rule 802.2a where appropriate. Many effects which relied on getDefendingPlayerId would fail if the attacking creature had been removed from combat before they resolved, in which case the defending player ID would be null. This fixes these issues. * Add test for removing attacking creature with Defending Player triggered ability. Change allowFormer to be true by default, reduce falses to only necessary cases. --- .../src/mage/cards/b/BeckoningWillOWisp.java | 2 +- Mage.Sets/src/mage/cards/o/OgreMarauder.java | 1 - Mage.Sets/src/mage/cards/s/SkymarkRoc.java | 1 - .../cards/u/UlamogTheCeaselessHunger.java | 3 +- .../src/mage/cards/w/WardscaleDragon.java | 3 +- .../test/combat/RemoveFromCombatTest.java | 26 +++++++++++++ .../main/java/mage/game/combat/Combat.java | 37 +++++++++++++++++++ .../java/mage/game/combat/CombatGroup.java | 7 ++++ 8 files changed, 73 insertions(+), 7 deletions(-) diff --git a/Mage.Sets/src/mage/cards/b/BeckoningWillOWisp.java b/Mage.Sets/src/mage/cards/b/BeckoningWillOWisp.java index 73fbd1b422b..363b90e2a52 100644 --- a/Mage.Sets/src/mage/cards/b/BeckoningWillOWisp.java +++ b/Mage.Sets/src/mage/cards/b/BeckoningWillOWisp.java @@ -70,7 +70,7 @@ enum BeckoningWillOWispPredicate implements ObjectSourcePlayerPredicate input, Game game) { UUID playerId = (UUID) game.getState().getValue(input.getSourceId() + "_" + game.getState().getZoneChangeCounter(input.getSourceId()) + "_chosenOpponent"); - return playerId != null && playerId.equals(game.getCombat().getDefendingPlayerId(input.getObject().getId(), game)); + return playerId != null && playerId.equals(game.getCombat().getDefendingPlayerId(input.getObject().getId(), game, false)); } } diff --git a/Mage.Sets/src/mage/cards/o/OgreMarauder.java b/Mage.Sets/src/mage/cards/o/OgreMarauder.java index eef1dcaf58a..e30f6d34f2b 100644 --- a/Mage.Sets/src/mage/cards/o/OgreMarauder.java +++ b/Mage.Sets/src/mage/cards/o/OgreMarauder.java @@ -21,7 +21,6 @@ import mage.constants.SubType; import mage.filter.StaticFilters; import mage.game.Game; import mage.players.Player; -import mage.target.common.TargetControlledCreaturePermanent; /** * diff --git a/Mage.Sets/src/mage/cards/s/SkymarkRoc.java b/Mage.Sets/src/mage/cards/s/SkymarkRoc.java index b0c3972d972..b9bc17a294a 100644 --- a/Mage.Sets/src/mage/cards/s/SkymarkRoc.java +++ b/Mage.Sets/src/mage/cards/s/SkymarkRoc.java @@ -18,7 +18,6 @@ import mage.filter.predicate.mageobject.ToughnessPredicate; import mage.filter.predicate.permanent.ControllerIdPredicate; import mage.game.Game; import mage.game.events.GameEvent; -import mage.game.events.GameEvent.EventType; import mage.target.common.TargetCreaturePermanent; /** diff --git a/Mage.Sets/src/mage/cards/u/UlamogTheCeaselessHunger.java b/Mage.Sets/src/mage/cards/u/UlamogTheCeaselessHunger.java index 2fd36db8c4f..0e84fc7d752 100644 --- a/Mage.Sets/src/mage/cards/u/UlamogTheCeaselessHunger.java +++ b/Mage.Sets/src/mage/cards/u/UlamogTheCeaselessHunger.java @@ -19,7 +19,6 @@ import mage.constants.Zone; import mage.filter.FilterPermanent; import mage.game.Game; import mage.game.events.GameEvent; -import mage.game.events.GameEvent.EventType; import mage.game.permanent.Permanent; import mage.game.stack.Spell; import mage.players.Player; @@ -113,7 +112,7 @@ class UlamogAttackTriggeredAbility extends TriggeredAbilityImpl { @Override public boolean checkTrigger(GameEvent event, Game game) { - Permanent sourcePermanent = game.getPermanent(this.getSourceId()); + Permanent sourcePermanent = game.getPermanentOrLKIBattlefield(this.getSourceId()); if (sourcePermanent != null && event.getSourceId() != null && event.getSourceId().equals(this.getSourceId())) { diff --git a/Mage.Sets/src/mage/cards/w/WardscaleDragon.java b/Mage.Sets/src/mage/cards/w/WardscaleDragon.java index 1b0af0849f4..950ff7ffd71 100644 --- a/Mage.Sets/src/mage/cards/w/WardscaleDragon.java +++ b/Mage.Sets/src/mage/cards/w/WardscaleDragon.java @@ -13,7 +13,6 @@ import mage.constants.CardType; import mage.constants.SubType; import mage.constants.Duration; import mage.constants.Outcome; -import mage.constants.Zone; import mage.game.Game; import mage.game.events.GameEvent; import mage.game.permanent.Permanent; @@ -73,7 +72,7 @@ class WardscaleDragonRuleEffect extends ContinuousRuleModifyingEffectImpl { public boolean applies(GameEvent event, Ability source, Game game) { Permanent sourcePermanent = game.getPermanent(source.getSourceId()); if (sourcePermanent != null && sourcePermanent.isAttacking()) { - return event.getPlayerId().equals(game.getCombat().getDefendingPlayerId(sourcePermanent.getId(), game)); + return event.getPlayerId().equals(game.getCombat().getDefendingPlayerId(sourcePermanent.getId(), game, false)); } return false; } diff --git a/Mage.Tests/src/test/java/org/mage/test/combat/RemoveFromCombatTest.java b/Mage.Tests/src/test/java/org/mage/test/combat/RemoveFromCombatTest.java index 503a37ea8fc..c2c0b3ba3c4 100644 --- a/Mage.Tests/src/test/java/org/mage/test/combat/RemoveFromCombatTest.java +++ b/Mage.Tests/src/test/java/org/mage/test/combat/RemoveFromCombatTest.java @@ -106,4 +106,30 @@ public class RemoveFromCombatTest extends CardTestPlayerBase { assertLife(playerB, 20 - 2); assertGraveyardCount(playerB, "Jace, Memory Adept", 1); } + + /** + * Validate rule 806.2a: Abilities which refer to Defending Player still mean that defending player, even if the + * attacking creature is removed from combat. + */ + @Test + public void test_RemoveAttackerWithDefendingPlayerTriggeredAbilityOnStack() { + + addCard(Zone.HAND, playerA, "Swords to Plowshares", 1); + addCard(Zone.BATTLEFIELD, playerA, "Agate-Blade Assassin", 1); // 2/2 + addCard(Zone.BATTLEFIELD, playerA, "Plains", 1); + + // attack player + attack(1, playerA, "Agate-Blade Assassin", playerB); + // remove Agate-Blade Assassin from combat + castSpell(1, PhaseStep.DECLARE_ATTACKERS, playerA, "Swords to Plowshares"); + addTarget(playerA, "Agate-Blade Assassin"); + + setStrictChooseMode(true); + setStopAt(1, PhaseStep.END_TURN); + execute(); + + assertLife(playerB, 20 - 1); + assertLife(playerA, 20 + 1 /* StP */ + 1 /* Agate-Blade Assassin trigger */); + } + } diff --git a/Mage/src/main/java/mage/game/combat/Combat.java b/Mage/src/main/java/mage/game/combat/Combat.java index a5c7829a8f4..7137f686c7b 100644 --- a/Mage/src/main/java/mage/game/combat/Combat.java +++ b/Mage/src/main/java/mage/game/combat/Combat.java @@ -39,6 +39,7 @@ import org.apache.log4j.Logger; import java.io.Serializable; import java.util.*; import java.util.stream.Collectors; +import java.util.stream.Stream; /** * @author BetaSteward_at_googlemail.com @@ -59,6 +60,7 @@ public class Combat implements Serializable, Copyable { private final List useToughnessForDamageFilters = new ArrayList<>(); protected List groups = new ArrayList<>(); + protected List formerGroups = new ArrayList<>(); protected Map blockingGroups = new HashMap<>(); // all possible defenders (players, planeswalkers or battle) protected Set defenders = new HashSet<>(); @@ -83,6 +85,9 @@ public class Combat implements Serializable, Copyable { for (CombatGroup group : combat.groups) { groups.add(group.copy()); } + for (CombatGroup group : combat.formerGroups) { + formerGroups.add(group.copy()); + } defenders.addAll(combat.defenders); for (Map.Entry group : combat.blockingGroups.entrySet()) { blockingGroups.put(group.getKey(), group.getValue()); @@ -181,6 +186,7 @@ public class Combat implements Serializable, Copyable { public void clear() { groups.clear(); + formerGroups.clear(); blockingGroups.clear(); defenders.clear(); attackingPlayerId = null; @@ -1679,6 +1685,36 @@ public class Combat implements Serializable, Copyable { * @return */ public UUID getDefendingPlayerId(UUID attackingCreatureId, Game game) { + return getDefendingPlayerId(attackingCreatureId, game, true); + } + + /** + * Returns the playerId of the player that is attacked by given attacking + * creature or formerly-attacking creature. + * + * @param attackingCreatureId + * @param game + * @return + */ + public UUID getDefendingPlayerId(UUID attackingCreatureId, Game game, boolean allowFormer) { + if (allowFormer) { + /* + * 802.2a. Any rule, object, or effect that refers to a "defending player" refers to one specific defending + * player, not to all of the defending players. If an ability of an attacking creature refers to a + * defending player, or a spell or ability refers to both an attacking creature and a defending player, + * then unless otherwise specified, the defending player it's referring to is the player that creature is + * attacking, the controller of the planeswalker that creature is attacking, or the protector of the battle + * that player is attacking. If that creature is no longer attacking, the defending player it's referring + * to is the player that creature was attacking before it was removed from combat, the controller of the + * planeswalker that creature was attacking before it was removed from combat, or the protector of the + * battle that player was attacking before it was removed from combat. + */ + return Stream.concat(groups.stream(), formerGroups.stream()) + .filter(group -> (group.getAttackers().contains(attackingCreatureId) || group.getFormerAttackers().contains(attackingCreatureId))) + .map(CombatGroup::getDefendingPlayerId) + .findFirst() + .orElse(null); + } return groups .stream() .filter(group -> group.getAttackers().contains(attackingCreatureId)) @@ -1743,6 +1779,7 @@ public class Combat implements Serializable, Copyable { } } if (group.attackers.isEmpty()) { + formerGroups.add(group); groups.remove(group); } return; diff --git a/Mage/src/main/java/mage/game/combat/CombatGroup.java b/Mage/src/main/java/mage/game/combat/CombatGroup.java index 22ad213ada8..9106b2d4abc 100644 --- a/Mage/src/main/java/mage/game/combat/CombatGroup.java +++ b/Mage/src/main/java/mage/game/combat/CombatGroup.java @@ -27,6 +27,7 @@ import java.util.stream.Stream; public class CombatGroup implements Serializable, Copyable { protected List attackers = new ArrayList<>(); + protected List formerAttackers = new ArrayList<>(); protected List blockers = new ArrayList<>(); protected List blockerOrder = new ArrayList<>(); protected List attackerOrder = new ArrayList<>(); @@ -49,6 +50,7 @@ public class CombatGroup implements Serializable, Copyable { protected CombatGroup(final CombatGroup group) { this.attackers.addAll(group.attackers); + this.formerAttackers.addAll(group.formerAttackers); this.blockers.addAll(group.blockers); this.blockerOrder.addAll(group.blockerOrder); this.attackerOrder.addAll(group.attackerOrder); @@ -81,6 +83,10 @@ public class CombatGroup implements Serializable, Copyable { return attackers; } + public List getFormerAttackers() { + return formerAttackers; + } + public List getBlockers() { return blockers; } @@ -737,6 +743,7 @@ public class CombatGroup implements Serializable, Copyable { public boolean remove(UUID creatureId) { boolean result = false; if (attackers.contains(creatureId)) { + formerAttackers.add(creatureId); attackers.remove(creatureId); result = true; attackerOrder.remove(creatureId);