in progress attempt at fixing Nadu in edge scenarios

Mage.Tests/src/test/java/org/mage/test/cards/single/mh3/NaduWingedWisdomTest.java

@@ -99,4 +99,171 @@ public class NaduWingedWisdomTest extends CardTestPlayerBase {
         assertHandCount(playerA, "Grizzly Bears", 2);
         assertPermanentCount(playerA, 4);
     }
+
+    @Test
+    public void test_Ephemerate_SeparateCount() {
+        setStrictChooseMode(true);
+        skipInitShuffling();
+
+        addCard(Zone.BATTLEFIELD, playerA, nadu);
+        addCard(Zone.BATTLEFIELD, playerA, "Shuko"); // Equip {0}
+        addCard(Zone.BATTLEFIELD, playerA, "Plains");
+        addCard(Zone.HAND, playerA, "Ephemerate");
+        addCard(Zone.BATTLEFIELD, playerA, "Elite Vanguard", 1);
+        addCard(Zone.LIBRARY, playerA, "Grizzly Bears", 10);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +1 bears in hand
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +1 bears in hand
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // No trigger third time
+
+        checkHandCardCount("2 triggers before ephemerate", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 2);
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Ephemerate", nadu, true);
+        // +1 bears in hand
+
+        checkHandCardCount("1 trigger on casting ephemerate", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 3);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +1 bears in hand
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +1 bears in hand
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // No trigger third time
+
+        setStopAt(1, PhaseStep.BEGIN_COMBAT);
+        execute();
+
+        assertHandCount(playerA, "Grizzly Bears", 5);
+    }
+
+    @Test
+    public void test_Sakashima_SeparateCount() {
+        setStrictChooseMode(true);
+        skipInitShuffling();
+
+        addCard(Zone.BATTLEFIELD, playerA, nadu);
+        addCard(Zone.BATTLEFIELD, playerA, "Shuko"); // Equip {0}
+        addCard(Zone.BATTLEFIELD, playerA, "Island", 4);
+        addCard(Zone.HAND, playerA, "Sakashima the Impostor");
+        addCard(Zone.LIBRARY, playerA, "Grizzly Bears", 10);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", nadu);
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +1 bears in hand
+        checkHandCardCount("1: 1 triggers before casting Sakashima", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 1);
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Sakashima the Impostor", true);
+        setChoice(playerA, true); // yes to "you may have"
+        setChoice(playerA, nadu); // choose to copy Nadu
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", nadu);
+        setChoice(playerA, "Whenever this creature becomes the target of a spell or ability"); // 2 triggers
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +2 bears in hand
+        checkHandCardCount("2: 2 triggers first reequip after casting Sakashima", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 3);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", nadu);
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +1 bears in hand
+        checkHandCardCount("3: 1 trigger second reequip after casting Sakashima", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 4);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", nadu);
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        checkHandCardCount("4: 0 trigger third reequip after casting Sakashima", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 4);
+        // No additional trigger
+
+        setStopAt(1, PhaseStep.BEGIN_COMBAT);
+        execute();
+
+        assertHandCount(playerA, "Grizzly Bears", 4);
+    }
+
+    @Test
+    public void test_DoubleNadu_MirrorGallery_SeparateCount() {
+        setStrictChooseMode(true);
+        skipInitShuffling();
+
+        addCard(Zone.BATTLEFIELD, playerA, "Mirror Gallery");
+        addCard(Zone.BATTLEFIELD, playerA, nadu, 2);
+        addCard(Zone.BATTLEFIELD, playerA, "Shuko"); // Equip {0}
+        addCard(Zone.BATTLEFIELD, playerA, "Elite Vanguard", 1);
+        addCard(Zone.LIBRARY, playerA, "Grizzly Bears", 10);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        setChoice(playerA, "Whenever this creature becomes the target of a spell or ability"); // 2 triggers
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +2 bears in hand
+        checkHandCardCount("1: after first equip", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 2);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        setChoice(playerA, "Whenever this creature becomes the target of a spell or ability"); // 2 triggers
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +2 bears in hand
+        checkHandCardCount("2: after second equip", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 4);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // No trigger third time
+        checkHandCardCount("3: after third equip", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 4);
+
+        setStopAt(1, PhaseStep.BEGIN_COMBAT);
+        execute();
+
+        assertHandCount(playerA, "Grizzly Bears", 4);
+    }
+
+    @Test
+    public void test_DoubleNadu_MirrorGallery_2_SeparateCount() {
+        setStrictChooseMode(true);
+        skipInitShuffling();
+
+        addCard(Zone.BATTLEFIELD, playerA, "Mirror Gallery");
+        addCard(Zone.BATTLEFIELD, playerA, nadu);
+        addCard(Zone.BATTLEFIELD, playerA, "Tropical Island", 3);
+        addCard(Zone.HAND, playerA, nadu);
+        addCard(Zone.BATTLEFIELD, playerA, "Shuko"); // Equip {0}
+        addCard(Zone.BATTLEFIELD, playerA, "Elite Vanguard", 1);
+        addCard(Zone.LIBRARY, playerA, "Grizzly Bears", 10);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +1 bears in hand
+        checkHandCardCount("1: before casting second Nadu", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 1);
+
+        castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, nadu, true);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        setChoice(playerA, "Whenever this creature becomes the target of a spell or ability"); // 2 triggers
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +2 bears in hand
+        checkHandCardCount("2: first trigger after casting second Nadu", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 3);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // +1 bears in hand
+        checkHandCardCount("3: second trigger after casting second Nadu", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 4);
+
+        activateAbility(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Equip {0}", "Elite Vanguard");
+        waitStackResolved(1, PhaseStep.PRECOMBAT_MAIN);
+        // no trigger fourth time
+        checkHandCardCount("4: third trigger after casting second Nadu", 1, PhaseStep.PRECOMBAT_MAIN, playerA, "Grizzly Bears", 4);
+
+        setStopAt(1, PhaseStep.BEGIN_COMBAT);
+        execute();
+
+        assertHandCount(playerA, "Grizzly Bears", 4);
+    }
 }

Mage/src/main/java/mage/abilities/Ability.java

@@ -46,6 +46,12 @@ public interface Ability extends Controllable, Serializable {
      */
     void newOriginalId(); // TODO: delete newOriginalId???
 
+
+    /**
+     * Assigns a specific originalId (helpful when adding an ability with a continuous effect)
+     */
+    void setOriginalId(UUID originalId);
+
     /**
      * Gets the {@link AbilityType} of this ability.
      *

Mage/src/main/java/mage/abilities/AbilityImpl.java

@@ -152,9 +152,16 @@ public abstract class AbilityImpl implements Ability {
 
     @Override
     public void newOriginalId() {
-        this.id = UUID.randomUUID();
+        setOriginalId(UUID.randomUUID());
-        this.originalId = id;
+    }
-        getEffects().newId();
+
+    @Override
+    public void setOriginalId(UUID newOriginalId) {
+        boolean hasChanged = !newOriginalId.equals(originalId);
+        this.originalId = newOriginalId;
+        if (hasChanged) {
+            getEffects().newId();
+        }
     }
 
     @Override

Mage/src/main/java/mage/abilities/effects/common/continuous/GainAbilityControlledEffect.java

@@ -1,5 +1,6 @@
 package mage.abilities.effects.common.continuous;
 
+import mage.MageObject;
 import mage.MageObjectReference;
 import mage.abilities.Ability;
 import mage.abilities.CompoundAbility;
@@ -14,18 +15,21 @@ import mage.game.Game;
 import mage.game.permanent.Permanent;
 import mage.util.CardUtil;
 
-import java.util.Iterator;
+import java.util.*;
 
 /**
  * @author BetaSteward_at_googlemail.com
  */
 public class GainAbilityControlledEffect extends ContinuousEffectImpl {
 
-    protected CompoundAbility ability;
+    protected CompoundAbility abilities;
     protected boolean excludeSource;
     protected FilterPermanent filter;
     protected boolean forceQuotes = false;
     protected boolean durationRuleAtStart = false; // put duration rule to the start of the rules instead end
+    protected Map<MageObjectReference, List<UUID>> originalIds = new HashMap<>(); // keep consistent individual originalId of gained ability for each affected permanent.
+    protected UUID lastSourceOriginalId; // remember the original id for the source giving the ability. If it changes, originalIds need to be fresh.
+    protected int lastSourceZcc; // remember the source zcc giving the ability. If it changes, originalIds need to be fresh.
 
     public GainAbilityControlledEffect(Ability ability, Duration duration) {
         this(ability, duration, StaticFilters.FILTER_PERMANENTS);
@@ -35,31 +39,38 @@ public class GainAbilityControlledEffect extends ContinuousEffectImpl {
         this(ability, duration, filter, false);
     }
 
-    public GainAbilityControlledEffect(CompoundAbility ability, Duration duration, FilterPermanent filter) {
+    public GainAbilityControlledEffect(CompoundAbility abilities, Duration duration, FilterPermanent filter) {
-        this(ability, duration, filter, false);
+        this(abilities, duration, filter, false);
     }
 
     public GainAbilityControlledEffect(Ability ability, Duration duration, FilterPermanent filter, boolean excludeSource) {
         this(new CompoundAbility(ability), duration, filter, excludeSource);
     }
 
-    public GainAbilityControlledEffect(CompoundAbility ability, Duration duration, FilterPermanent filter, boolean excludeSource) {
+    public GainAbilityControlledEffect(CompoundAbility abilities, Duration duration, FilterPermanent filter, boolean excludeSource) {
         super(duration, Layer.AbilityAddingRemovingEffects_6, SubLayer.NA, Outcome.AddAbility);
-        this.ability = ability;
+        this.abilities = abilities;
         this.filter = filter;
         this.excludeSource = excludeSource;
         setText();
 
-        this.generateGainAbilityDependencies(ability, filter);
+        this.generateGainAbilityDependencies(abilities, filter);
     }
 
     protected GainAbilityControlledEffect(final GainAbilityControlledEffect effect) {
         super(effect);
-        this.ability = effect.ability.copy();
+        this.abilities = effect.abilities.copy();
         this.filter = effect.filter.copy();
         this.excludeSource = effect.excludeSource;
         this.forceQuotes = effect.forceQuotes;
         this.durationRuleAtStart = effect.durationRuleAtStart;
+        for (MageObjectReference mor : effect.originalIds.keySet()) {
+            List<UUID> array = new ArrayList<>();
+            array.addAll(effect.originalIds.get(mor));
+            this.originalIds.put(mor, array);
+        }
+        this.lastSourceOriginalId = effect.lastSourceOriginalId;
+        this.lastSourceZcc = effect.lastSourceZcc;
     }
 
     @Override
@@ -80,14 +91,44 @@ public class GainAbilityControlledEffect extends ContinuousEffectImpl {
         return new GainAbilityControlledEffect(this);
     }
 
+    /**
+     * OriginalIds for the copied abilities for a given permanent need to stay consistent each time the effect apply.
+     * This method attempts to retrieved stored originalIds, and if not found, create new ones.
+     */
+    private List<UUID> getOriginalIds(MageObjectReference permMOR, Ability source, Game game) {
+        UUID sourceOriginalId = source.getOriginalId();
+        MageObject sourceObject = source.getSourceObject(game);
+        int sourceZcc = sourceObject == null ? -1 : sourceObject.getZoneChangeCounter(game);
+        //System.out.println(sourceOriginalId + " " + sourceZcc);
+        if (!sourceOriginalId.equals(lastSourceOriginalId) || sourceZcc != lastSourceZcc) {
+            // The source of the ability has changed, discarding outdated originalIds
+            originalIds.clear();
+            lastSourceOriginalId = sourceOriginalId;
+            lastSourceZcc = sourceZcc;
+        }
+        if (originalIds.containsKey(permMOR)) {
+            return originalIds.get(permMOR);
+        }
+        List<UUID> newOriginalIds = new ArrayList<>();
+        for (int i = 0; i < abilities.size(); ++i) {
+            newOriginalIds.add(UUID.randomUUID());
+        }
+        originalIds.put(permMOR, newOriginalIds);
+        return newOriginalIds;
+    }
+
     @Override
     public boolean apply(Game game, Ability source) {
         if (getAffectedObjectsSet()) {
             for (Iterator<MageObjectReference> it = affectedObjectList.iterator(); it.hasNext(); ) { // filter may not be used again, because object can have changed filter relevant attributes but still geets boost
-                Permanent perm = it.next().getPermanentOrLKIBattlefield(game); //LKI is neccessary for "dies triggered abilities" to work given to permanets  (e.g. Showstopper)
+                MageObjectReference mor = it.next();
+                Permanent perm = mor.getPermanentOrLKIBattlefield(game); //LKI is necessary for "dies triggered abilities" to work given to permanets  (e.g. Showstopper)
                 if (perm != null) {
-                    for (Ability abilityToAdd : ability) {
+                    List<UUID> originalIds = getOriginalIds(mor, source, game);
-                        perm.addAbility(abilityToAdd, source.getSourceId(), game);
+                    for (int i = 0; i < abilities.size(); ++i) {
+                        Ability abilityToAdd = abilities.get(i);
+                        UUID originalId = originalIds.get(i);
+                        perm.addAbility(abilityToAdd, originalId, source.getSourceId(), game);
                     }
                 } else {
                     it.remove();
@@ -100,8 +141,11 @@ public class GainAbilityControlledEffect extends ContinuousEffectImpl {
             for (Permanent perm : game.getBattlefield().getActivePermanents(filter, source.getControllerId(), source, game)) {
                 if (perm.isControlledBy(source.getControllerId())
                         && !(excludeSource && perm.getId().equals(source.getSourceId()))) {
-                    for (Ability abilityToAdd : ability) {
+                    List<UUID> originalIds = getOriginalIds(new MageObjectReference(perm, game), source, game);
-                        perm.addAbility(abilityToAdd, source.getSourceId(), game);
+                    for (int i = 0; i < abilities.size(); ++i) {
+                        Ability abilityToAdd = abilities.get(i);
+                        UUID originalId = originalIds.get(i);
+                        perm.addAbility(abilityToAdd, originalId, source.getSourceId(), game);
                     }
                 }
             }
@@ -109,14 +153,6 @@ public class GainAbilityControlledEffect extends ContinuousEffectImpl {
         return true;
     }
 
-    public void setAbility(Ability ability) {
-        this.ability = new CompoundAbility(ability);
-    }
-
-    public Ability getFirstAbility() {
-        return ability.get(0);
-    }
-
     private void setText() {
         StringBuilder sb = new StringBuilder();
         if (durationRuleAtStart && !duration.toString().isEmpty() && duration != Duration.EndOfGame) {
@@ -125,7 +161,7 @@ public class GainAbilityControlledEffect extends ContinuousEffectImpl {
         if (excludeSource) {
             sb.append("other ");
         }
-        String gainedAbility = CardUtil.stripReminderText(ability.getRule());
+        String gainedAbility = CardUtil.stripReminderText(abilities.getRule());
         sb.append(filter.getMessage());
         if (!filter.getMessage().contains("you control")) {
             sb.append(" you control");

Mage/src/main/java/mage/game/permanent/Permanent.java

@@ -221,17 +221,24 @@ public interface Permanent extends Card, Controllable {
 
     String getValue(GameState state);
 
+    // TODO: remove all in favor of the originalId ones?
+    Ability addAbility(Ability ability, UUID sourceId, Game game);
+
+    // TODO: remove all in favor of the originalId ones?
+    Ability addAbility(Ability ability, UUID sourceId, Game game, boolean fromExistingObject);
+
     /**
      * Add abilities to the permanent, can be used in effects
      *
      * @param ability
-     * @param sourceId can be null
+     * @param originalId set the originalId for the ability's copy.
+     * @param sourceId   can be null
      * @param game
      * @return can be null for exists abilities
      */
-    Ability addAbility(Ability ability, UUID sourceId, Game game);
+    Ability addAbility(Ability ability, UUID originalId, UUID sourceId, Game game);
 
-    Ability addAbility(Ability ability, UUID sourceId, Game game, boolean fromExistingObject);
+    Ability addAbility(Ability ability, UUID originalId, UUID sourceId, Game game, boolean fromExistingObject);
 
     void removeAllAbilities(UUID sourceId, Game game);
 

Mage/src/main/java/mage/game/permanent/PermanentImpl.java

@@ -413,22 +413,36 @@ public abstract class PermanentImpl extends CardImpl implements Permanent {
         return super.getAbilities(game);
     }
 
+    // TODO: remove. temporary
+    @Override
+    public Ability addAbility(Ability ability, UUID sourceId, Game game) {
+        return addAbility(ability, null, sourceId, game);
+    }
+
+    // TODO: remove. temporary
+    @Override
+    public Ability addAbility(Ability ability, UUID sourceId, Game game, boolean fromExistingObject) {
+        return addAbility(ability, null, sourceId, game, fromExistingObject);
+    }
+
     /**
      * Add an ability to the permanent. When copying from an existing source
      * you should use the fromExistingObject variant of this function to prevent double-copying subabilities
      *
-     * @param ability  The ability to be added
+     * @param ability    The ability to be added
-     * @param sourceId id of the source doing the added (for the effect created to add it)
+     * @param originalId original id for the ability once added.
+     * @param sourceId   id of the source doing the added (for the effect created to add it)
      * @param game
      * @return The newly added ability copy
      */
     @Override
-    public Ability addAbility(Ability ability, UUID sourceId, Game game) {
+    public Ability addAbility(Ability ability, UUID originalId, UUID sourceId, Game game) {
-        return addAbility(ability, sourceId, game, false);
+        return addAbility(ability, originalId, sourceId, game, false);
     }
 
     /**
      * @param ability            The ability to be added
+     * @param originalId         original id for the ability once added.
      * @param sourceId           id of the source doing the added (for the effect created to add it)
      * @param game
      * @param fromExistingObject if copying abilities from an existing source then must ignore sub-abilities because they're already on the source object
@@ -436,12 +450,15 @@ public abstract class PermanentImpl extends CardImpl implements Permanent {
      * @return The newly added ability copy
      */
     @Override
-    public Ability addAbility(Ability ability, UUID sourceId, Game game, boolean fromExistingObject) {
+    public Ability addAbility(Ability ability, UUID originalId, UUID sourceId, Game game, boolean fromExistingObject) {
         // singleton abilities -- only one instance
         // other abilities -- any amount of instances
         if (!abilities.containsKey(ability.getId())) {
             Ability copyAbility = ability.copy();
             copyAbility.newId(); // needed so that source can get an ability multiple times (e.g. Raging Ravine)
+            if (originalId != null) { // TODO: should we enforce not null originalId?
+                copyAbility.setOriginalId(originalId);
+            }
             copyAbility.setControllerId(controllerId);
             copyAbility.setSourceId(objectId);
             // triggered abilities must be added to the state().triggers

Mage/src/main/java/mage/game/stack/StackAbility.java

@@ -492,6 +492,10 @@ public class StackAbility extends StackObjectImpl implements Ability {
     public void newOriginalId() {
     }
 
+    @Override
+    public void setOriginalId(UUID newOriginalId) {
+    }
+
     @Override
     public Ability getStackAbility() {
         return ability;