Mana Maze - fixed game error on usage (closes #11572, closes #11575);

Mage.Tests/src/test/java/org/mage/test/cards/single/inv/ManaMazeTest.java

@@ -4,7 +4,6 @@ import mage.constants.PhaseStep;
 import mage.constants.Zone;
 import mage.util.CardUtil;
 import org.junit.Assert;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.mage.test.serverside.base.CardTestPlayerBase;
 
@@ -15,12 +14,12 @@ import java.util.List;
 /**
  * @author JayDi85
  */
-@Ignore // TODO: enable after deep copy fix
 public class ManaMazeTest extends CardTestPlayerBase {
 
-    @Test
+    @Test(expected = StackOverflowError.class)
     public void test_DeepCopy_WithSelfReference() {
         // stack overflow bug: https://github.com/magefree/mage/issues/11572
+        // proof of self ref reason for stack overflow
 
         // list
         List<String> sourceList = new ArrayList<>(Arrays.asList("val1", "val2", "val3"));
@@ -33,13 +32,7 @@ public class ManaMazeTest extends CardTestPlayerBase {
         List<List<Object>> sourceObjectList = new ArrayList<>();
         sourceObjectList.add(new ArrayList<>(Arrays.asList("val1", "val2", "val3")));
         sourceObjectList.add(new ArrayList<>(Arrays.asList(sourceObjectList)));
-        List<List<Object>> copyObjectList = CardUtil.deepCopyObject(sourceObjectList);
+        CardUtil.deepCopyObject(sourceObjectList);
-        Assert.assertNotSame(sourceObjectList, copyObjectList);
-        Assert.assertEquals(sourceObjectList.size(), copyObjectList.size());
-        Assert.assertEquals(sourceObjectList.get(0).size(), copyObjectList.get(0).size());
-        Assert.assertEquals(sourceObjectList.get(0).toString(), copyObjectList.get(0).toString());
-        Assert.assertEquals(sourceObjectList.get(1).size(), copyObjectList.get(1).size());
-        Assert.assertEquals(sourceObjectList.get(1).toString(), copyObjectList.get(1).toString());
     }
 
     @Test

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

@@ -366,6 +366,11 @@ public interface Ability extends Controllable, Serializable {
 
     List<Watcher> getWatchers();
 
+    /**
+     * Add watcher blueprint (real watcher will be created on card/ability init)
+     *
+     * @param watcher
+     */
     void addWatcher(Watcher watcher);
 
     /**

Mage/src/main/java/mage/game/GameImpl.java

@@ -1378,7 +1378,6 @@ public abstract class GameImpl implements Game {
     public void initPlayerDefaultWatchers(UUID playerId) {
         PlayerDamagedBySourceWatcher playerDamagedBySourceWatcher = new PlayerDamagedBySourceWatcher();
         playerDamagedBySourceWatcher.setControllerId(playerId);
-
         getState().addWatcher(playerDamagedBySourceWatcher);
 
         BloodthirstWatcher bloodthirstWatcher = new BloodthirstWatcher();

Mage/src/main/java/mage/game/GameState.java

@@ -1065,8 +1065,7 @@ public class GameState implements Serializable, Copyable<GameState> {
             addTrigger((TriggeredAbility) ability, sourceId, attachedTo);
         }
 
-        List<Watcher> watcherList = new ArrayList<>(ability.getWatchers()); // Workaround to prevent ConcurrentModificationException, not clear to me why this is happening now
+        for (Watcher watcher : ability.getWatchers()) {
-        for (Watcher watcher : watcherList) {
             // TODO: Check that watcher for commanderAbility (where attachedTo = null) also work correctly
             UUID controllerId = ability.getControllerId();
             if (attachedTo instanceof Card) {
@@ -1074,9 +1073,11 @@ public class GameState implements Serializable, Copyable<GameState> {
             } else if (attachedTo instanceof Controllable) {
                 controllerId = ((Controllable) attachedTo).getControllerId();
             }
-            watcher.setControllerId(controllerId);
+
-            watcher.setSourceId(attachedTo == null ? ability.getSourceId() : attachedTo.getId());
+            Watcher newWatcher = watcher.copy();
-            watchers.add(watcher);
+            newWatcher.setControllerId(controllerId);
+            newWatcher.setSourceId(attachedTo == null ? ability.getSourceId() : attachedTo.getId());
+            watchers.add(newWatcher);
         }
 
         for (Ability sub : ability.getSubAbilities()) {
@@ -1136,9 +1137,10 @@ public class GameState implements Serializable, Copyable<GameState> {
 
         List<Watcher> watcherList = new ArrayList<>(ability.getWatchers()); // Workaround to prevent ConcurrentModificationException, not clear to me why this is happening now
         for (Watcher watcher : watcherList) {
-            watcher.setControllerId(ability.getControllerId());
+            Watcher newWatcher = watcher.copy();
-            watcher.setSourceId(ability.getSourceId());
+            newWatcher.setControllerId(ability.getControllerId());
-            this.watchers.add(watcher);
+            newWatcher.setSourceId(ability.getSourceId());
+            this.watchers.add(newWatcher);
         }
     }
 
@@ -1378,8 +1380,11 @@ public class GameState implements Serializable, Copyable<GameState> {
         ); // The stored MOR is the stack-moment MOR so need to subtract one from the permanent's ZCC for the check
     }
 
-    public void addWatcher(Watcher watcher) {
+    /**
-        this.watchers.add(watcher);
+     * Must add copy of the original watcher, e.g. from an ability
+     */
+    public void addWatcher(Watcher newWatcher) {
+        this.watchers.add(newWatcher);
     }
 
     public void resetWatchers() {

Mage/src/main/java/mage/util/CardUtil.java

@@ -1758,6 +1758,15 @@ public final class CardUtil {
                 || o instanceof Enum;
     }
 
+    /**
+     * Make deep copy of any object (supported by xmage)
+     * <p>
+     * Warning, don't use self reference objects because it will raise StackOverflowError
+     *
+     * @param value
+     * @return
+     * @param <T>
+     */
     public static <T> T deepCopyObject(T value) {
         if (isImmutableObject(value)) {
             return value;
@@ -1789,6 +1798,7 @@ public final class CardUtil {
             AbstractMap.SimpleImmutableEntry entryValue = (AbstractMap.SimpleImmutableEntry) value;
             return (T) new AbstractMap.SimpleImmutableEntry(deepCopyObject(entryValue.getKey()), deepCopyObject(entryValue.getValue()));
         } else {
+            // warning, do not add unnecessarily new data types and structures to game engine, try to use only standard types (see above)
             throw new IllegalStateException("Unhandled object " + value.getClass().getSimpleName() + " during deep copy, must add explicit handling of all Object types");
         }
     }

Mage/src/main/java/mage/watchers/Watcher.java

@@ -60,7 +60,7 @@ public abstract class Watcher implements Serializable {
             case CARD:
                 return sourceId + getBasicKey();
             default:
-                return getBasicKey();
+                throw new IllegalArgumentException("Unknown watcher scope: " + this.getClass().getSimpleName() + " - " + scope);
         }
     }
 

Mage/src/main/java/mage/watchers/Watchers.java

@@ -25,8 +25,11 @@ public class Watchers extends HashMap<String, Watcher> {
         return new Watchers(this);
     }
 
-    public void add(Watcher watcher) {
+    /**
-        putIfAbsent(watcher.getKey(), watcher);
+     * Must add copy of the original watcher, e.g. from an ability
+     */
+    public void add(Watcher newWatcher) {
+        putIfAbsent(newWatcher.getKey(), newWatcher);
     }
 
     public void watch(GameEvent event, Game game) {

Mage/src/main/java/mage/watchers/common/CommanderInfoWatcher.java

@@ -9,7 +9,6 @@ import mage.constants.WatcherScope;
 import mage.game.Game;
 import mage.game.events.DamagedPlayerEvent;
 import mage.game.events.GameEvent;
-import mage.game.events.GameEvent.EventType;
 import mage.game.permanent.Permanent;
 import mage.players.Player;
 import mage.watchers.Watcher;
@@ -74,20 +73,20 @@ public class CommanderInfoWatcher extends Watcher {
             if (playsCount > 0) {
                 sb.append(' ').append(playsCount).append(playsCount == 1 ? " time" : " times").append(" played from the command zone.");
             }
-            this.addInfo(object, "Commander", sb.toString(), game);
+            this.addInfoToObject(object, "Commander", sb.toString(), game);
 
             if (checkCommanderDamage) {
                 for (Map.Entry<UUID, Integer> entry : damageToPlayer.entrySet()) {
                     Player damagedPlayer = game.getPlayer(entry.getKey());
                     sb.append("<b>").append(commanderTypeName).append("</b> did ").append(entry.getValue()).append(" combat damage to player ").append(damagedPlayer.getLogName()).append('.');
-                    this.addInfo(object, "Commander" + entry.getKey(),
+                    this.addInfoToObject(object, "Commander" + entry.getKey(),
                             "<b>" + commanderTypeName + "</b> did " + entry.getValue() + " combat damage to player " + damagedPlayer.getLogName() + '.', game);
                 }
             }
         }
     }
 
-    private void addInfo(MageObject object, String key, String value, Game game) {
+    private void addInfoToObject(MageObject object, String key, String value, Game game) {
         ((Card) object).addInfo(key, value, game);
         if (object instanceof Permanent) {
             ((Permanent) object).addInfo(key, value, game);