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

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);
+
+    }
+
+}

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);
+    }
+
+}

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;
     }
 }

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;
     }
 }