From 35a5cc48a73f5dfd3787896843432c0c94cdcd95 Mon Sep 17 00:00:00 2001 From: xenohedron Date: Sun, 6 Aug 2023 23:04:53 -0400 Subject: [PATCH] Improve DealsCombatDamageEquippedTriggeredAbility (#10767) * improve variable name * add test for Jitte trample damage * fix duplicated triggers when trampling over to player * bring corresponding ability into alignment * adjust authorship, comment --- .../cards/single/bok/UmezawasJitteTest.java | 51 ++++++++++++++ .../damage/DealsCombatDamageTriggerTest.java | 70 +++++++++++++++++++ ...sCombatDamageEquippedTriggeredAbility.java | 20 +++++- .../DealsCombatDamageTriggeredAbility.java | 44 +++++++----- 4 files changed, 166 insertions(+), 19 deletions(-) create mode 100644 Mage.Tests/src/test/java/org/mage/test/cards/single/bok/UmezawasJitteTest.java create mode 100644 Mage.Tests/src/test/java/org/mage/test/cards/triggers/damage/DealsCombatDamageTriggerTest.java diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/single/bok/UmezawasJitteTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/single/bok/UmezawasJitteTest.java new file mode 100644 index 00000000000..ba3d214b340 --- /dev/null +++ b/Mage.Tests/src/test/java/org/mage/test/cards/single/bok/UmezawasJitteTest.java @@ -0,0 +1,51 @@ +package org.mage.test.cards.single.bok; + +import mage.constants.PhaseStep; +import mage.constants.Zone; +import mage.counters.CounterType; +import org.junit.Test; +import org.mage.test.serverside.base.CardTestPlayerBase; + +/** + * @author xenohedron + */ +public class UmezawasJitteTest extends CardTestPlayerBase { + + private static final String jitte = "Umezawa's Jitte"; + // Whenever equipped creature deals combat damage, put two charge counters on Umezawa’s Jitte. + // Equip 2 + private static final String attacker = "Spiked Baloth"; // 4/2 trample + private static final String defender1 = "Memnite"; // 1/1 vanilla + private static final String defender2 = "Darksteel Myr"; // 0/1 indestructible + + @Test + public void testTrampleSingleDamageTrigger() { + // Reported bug: #10763 + + addCard(Zone.BATTLEFIELD, playerA, jitte); + addCard(Zone.BATTLEFIELD, playerA, attacker); + addCard(Zone.BATTLEFIELD, playerB, defender1); + addCard(Zone.BATTLEFIELD, playerB, defender2); + addCard(Zone.BATTLEFIELD, playerA, "Wastes", 2); + + activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip", attacker); + + attack(1, playerA, attacker, playerB); + block(1, playerB, defender1, attacker); + block(1, playerB, defender2, attacker); + // Blockers are ordered in order added by the test framework + setChoice(playerA, "X=1"); // 1 damage to first defender + setChoice(playerA, "X=1"); // 1 damage to second defender + + setStrictChooseMode(true); + setStopAt(1, PhaseStep.POSTCOMBAT_MAIN); + execute(); + + assertLife(playerB, 20 - 2); + assertGraveyardCount(playerB, defender1, 1); + assertPermanentCount(playerB, defender2, 1); + assertCounterCount(playerA, jitte, CounterType.CHARGE, 2); + + } + +} diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/triggers/damage/DealsCombatDamageTriggerTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/damage/DealsCombatDamageTriggerTest.java new file mode 100644 index 00000000000..ec876e631e0 --- /dev/null +++ b/Mage.Tests/src/test/java/org/mage/test/cards/triggers/damage/DealsCombatDamageTriggerTest.java @@ -0,0 +1,70 @@ +package org.mage.test.cards.triggers.damage; + +import mage.constants.PhaseStep; +import mage.constants.Zone; +import org.junit.Test; +import org.mage.test.serverside.base.CardTestPlayerBase; + +/** + * @author xenohedron + */ +public class DealsCombatDamageTriggerTest extends CardTestPlayerBase { + + private static final String drinker = "Drinker of Sorrow"; // 5/3 + // Whenever Drinker of Sorrow deals combat damage, sacrifice a permanent. + private static final String memnite = "Memnite"; // 1/1 + + @Test + public void triggerSourceDealsDamage() { + addCard(Zone.BATTLEFIELD, playerA, drinker); + addCard(Zone.BATTLEFIELD, playerA, memnite); + + attack(1, playerA, drinker, playerB); + + addTarget(playerA, memnite); // to sacrifice + + setStrictChooseMode(true); + setStopAt(1, PhaseStep.POSTCOMBAT_MAIN); + execute(); + + assertLife(playerB, 20 - 5); + assertPermanentCount(playerA, drinker, 1); + assertGraveyardCount(playerA, memnite, 1); + } + + @Test + public void noTriggerOtherDealsDamage() { + addCard(Zone.BATTLEFIELD, playerA, drinker); + addCard(Zone.BATTLEFIELD, playerA, memnite); + + attack(1, playerA, memnite, playerB); + + setStrictChooseMode(true); + setStopAt(1, PhaseStep.POSTCOMBAT_MAIN); + execute(); + + assertLife(playerB, 20 - 1); + assertPermanentCount(playerA, drinker, 1); + assertPermanentCount(playerA, memnite, 1); + } + + @Test + public void triggerTwoSourcesDealDamage() { + addCard(Zone.BATTLEFIELD, playerA, drinker, 2); + + attack(1, playerA, drinker, playerB); + attack(1, playerA, drinker, playerB); + + setChoice(playerA, "Whenever"); // order identical triggers + addTarget(playerA, drinker); // to sacrifice + addTarget(playerA, drinker); // to sacrifice + + setStrictChooseMode(true); + setStopAt(1, PhaseStep.POSTCOMBAT_MAIN); + execute(); + + assertLife(playerB, 20 - 10); + assertGraveyardCount(playerA, drinker, 2); + } + +} diff --git a/Mage/src/main/java/mage/abilities/common/DealsCombatDamageEquippedTriggeredAbility.java b/Mage/src/main/java/mage/abilities/common/DealsCombatDamageEquippedTriggeredAbility.java index 7a25695a74c..82877928a76 100644 --- a/Mage/src/main/java/mage/abilities/common/DealsCombatDamageEquippedTriggeredAbility.java +++ b/Mage/src/main/java/mage/abilities/common/DealsCombatDamageEquippedTriggeredAbility.java @@ -10,21 +10,25 @@ import mage.game.events.GameEvent; import mage.game.permanent.Permanent; /** - * @author TheElk801 + * @author TheElk801, xenohedron */ public class DealsCombatDamageEquippedTriggeredAbility extends TriggeredAbilityImpl { + private boolean usedThisStep; + public DealsCombatDamageEquippedTriggeredAbility(Effect effect) { this(effect, false); } public DealsCombatDamageEquippedTriggeredAbility(Effect effect, boolean optional) { super(Zone.BATTLEFIELD, effect, optional); + this.usedThisStep = false; setTriggerPhrase("Whenever equipped creature deals combat damage, "); } protected DealsCombatDamageEquippedTriggeredAbility(final DealsCombatDamageEquippedTriggeredAbility ability) { super(ability); + this.usedThisStep = ability.usedThisStep; } @Override @@ -34,12 +38,18 @@ public class DealsCombatDamageEquippedTriggeredAbility extends TriggeredAbilityI @Override public boolean checkEventType(GameEvent event, Game game) { - return event.getType() == GameEvent.EventType.DAMAGED_PLAYER_BATCH - || event.getType() == GameEvent.EventType.DAMAGED_PERMANENT_BATCH; + return event instanceof DamagedBatchEvent || event.getType() == GameEvent.EventType.COMBAT_DAMAGE_STEP_PRE; } @Override public boolean checkTrigger(GameEvent event, Game game) { + if (event.getType() == GameEvent.EventType.COMBAT_DAMAGE_STEP_PRE) { + usedThisStep = false; // clear before damage + return false; + } + if (usedThisStep || !(event instanceof DamagedBatchEvent)) { + return false; // trigger only on DamagedEvent and if not yet triggered this step + } Permanent sourcePermanent = getSourcePermanentOrLKI(game); if (sourcePermanent == null || sourcePermanent.getAttachedTo() == null) { return false; @@ -54,7 +64,11 @@ public class DealsCombatDamageEquippedTriggeredAbility extends TriggeredAbilityI if (amount < 1) { return false; } + usedThisStep = true; this.getEffects().setValue("damage", amount); + // TODO: this value saved will not be correct if both permanent and player damaged by a single creature + // Need to rework engine logic to fire a single DamagedBatchEvent including both permanents and players + // Only Sword of Hours is currently affected. return true; } } diff --git a/Mage/src/main/java/mage/abilities/common/DealsCombatDamageTriggeredAbility.java b/Mage/src/main/java/mage/abilities/common/DealsCombatDamageTriggeredAbility.java index 748b54dd2be..f8f0b90fc69 100644 --- a/Mage/src/main/java/mage/abilities/common/DealsCombatDamageTriggeredAbility.java +++ b/Mage/src/main/java/mage/abilities/common/DealsCombatDamageTriggeredAbility.java @@ -4,29 +4,30 @@ import mage.abilities.TriggeredAbilityImpl; import mage.abilities.effects.Effect; import mage.constants.Zone; import mage.game.Game; +import mage.game.events.DamagedBatchEvent; import mage.game.events.DamagedEvent; import mage.game.events.GameEvent; /** - * This triggers only once for each phase the source creature deals damage. + * This triggers only once for each combat damage step the source creature deals damage. * So a creature blocked by two creatures and dealing damage to both blockers in the same * combat damage step triggers only once. * - * @author LevelX + * @author LevelX, xenohedron */ public class DealsCombatDamageTriggeredAbility extends TriggeredAbilityImpl { - private boolean usedInPhase; + private boolean usedThisStep; public DealsCombatDamageTriggeredAbility(Effect effect, boolean optional) { super(Zone.BATTLEFIELD, effect, optional); - this.usedInPhase = false; + this.usedThisStep = false; setTriggerPhrase("Whenever {this} deals combat damage, "); } protected DealsCombatDamageTriggeredAbility(final DealsCombatDamageTriggeredAbility ability) { super(ability); - this.usedInPhase = ability.usedInPhase; + this.usedThisStep = ability.usedThisStep; } @Override @@ -36,22 +37,33 @@ public class DealsCombatDamageTriggeredAbility extends TriggeredAbilityImpl { @Override public boolean checkEventType(GameEvent event, Game game) { - return event instanceof DamagedEvent || event.getType() == GameEvent.EventType.COMBAT_DAMAGE_STEP_PRE; + return event instanceof DamagedBatchEvent || event.getType() == GameEvent.EventType.COMBAT_DAMAGE_STEP_PRE; } @Override public boolean checkTrigger(GameEvent event, Game game) { - if (event instanceof DamagedEvent - && !usedInPhase - && event.getSourceId().equals(this.sourceId) - && ((DamagedEvent) event).isCombatDamage()) { - usedInPhase = true; - getEffects().setValue("damage", event.getAmount()); - return true; - } if (event.getType() == GameEvent.EventType.COMBAT_DAMAGE_STEP_PRE) { - usedInPhase = false; + usedThisStep = false; // clear before damage + return false; } - return false; + if (usedThisStep || !(event instanceof DamagedBatchEvent)) { + return false; // trigger only on DamagedEvent and if not yet triggered this step + } + int amount = ((DamagedBatchEvent) event) + .getEvents() + .stream() + .filter(DamagedEvent::isCombatDamage) + .filter(e -> e.getAttackerId().equals(this.sourceId)) + .mapToInt(GameEvent::getAmount) + .sum(); + if (amount < 1) { + return false; + } + usedThisStep = true; + this.getEffects().setValue("damage", amount); + // TODO: this value saved will not be correct if both permanent and player damaged by a single creature + // Need to rework engine logic to fire a single DamagedBatchEvent including both permanents and players + // Only Aisha of Sparks and Smoke is currently affected. + return true; } }