diff --git a/Mage.Tests/src/test/java/org/mage/test/cards/single/inv/ManaMazeTest.java b/Mage.Tests/src/test/java/org/mage/test/cards/single/inv/ManaMazeTest.java index 5b96860a953..f137c6db9e9 100644 --- a/Mage.Tests/src/test/java/org/mage/test/cards/single/inv/ManaMazeTest.java +++ b/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 sourceList = new ArrayList<>(Arrays.asList("val1", "val2", "val3")); @@ -33,13 +32,7 @@ public class ManaMazeTest extends CardTestPlayerBase { List> sourceObjectList = new ArrayList<>(); sourceObjectList.add(new ArrayList<>(Arrays.asList("val1", "val2", "val3"))); sourceObjectList.add(new ArrayList<>(Arrays.asList(sourceObjectList))); - List> copyObjectList = 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()); + CardUtil.deepCopyObject(sourceObjectList); } @Test diff --git a/Mage/src/main/java/mage/abilities/Ability.java b/Mage/src/main/java/mage/abilities/Ability.java index 4eb1d0e6f54..d978ad7996e 100644 --- a/Mage/src/main/java/mage/abilities/Ability.java +++ b/Mage/src/main/java/mage/abilities/Ability.java @@ -366,6 +366,11 @@ public interface Ability extends Controllable, Serializable { List getWatchers(); + /** + * Add watcher blueprint (real watcher will be created on card/ability init) + * + * @param watcher + */ void addWatcher(Watcher watcher); /** diff --git a/Mage/src/main/java/mage/game/GameImpl.java b/Mage/src/main/java/mage/game/GameImpl.java index 21e76cef684..2f9e48cf098 100644 --- a/Mage/src/main/java/mage/game/GameImpl.java +++ b/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(); diff --git a/Mage/src/main/java/mage/game/GameState.java b/Mage/src/main/java/mage/game/GameState.java index 6bf80111caa..c4c3b17a6cf 100644 --- a/Mage/src/main/java/mage/game/GameState.java +++ b/Mage/src/main/java/mage/game/GameState.java @@ -1065,8 +1065,7 @@ public class GameState implements Serializable, Copyable { addTrigger((TriggeredAbility) ability, sourceId, attachedTo); } - List watcherList = new ArrayList<>(ability.getWatchers()); // Workaround to prevent ConcurrentModificationException, not clear to me why this is happening now - for (Watcher watcher : watcherList) { + for (Watcher watcher : ability.getWatchers()) { // 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 { } else if (attachedTo instanceof Controllable) { controllerId = ((Controllable) attachedTo).getControllerId(); } - watcher.setControllerId(controllerId); - watcher.setSourceId(attachedTo == null ? ability.getSourceId() : attachedTo.getId()); - watchers.add(watcher); + + Watcher newWatcher = watcher.copy(); + 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 { List 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.setSourceId(ability.getSourceId()); - this.watchers.add(watcher); + Watcher newWatcher = watcher.copy(); + newWatcher.setControllerId(ability.getControllerId()); + newWatcher.setSourceId(ability.getSourceId()); + this.watchers.add(newWatcher); } } @@ -1378,8 +1380,11 @@ public class GameState implements Serializable, Copyable { ); // 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() { diff --git a/Mage/src/main/java/mage/util/CardUtil.java b/Mage/src/main/java/mage/util/CardUtil.java index 41db4d93109..9cc6767be7a 100644 --- a/Mage/src/main/java/mage/util/CardUtil.java +++ b/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) + *

+ * Warning, don't use self reference objects because it will raise StackOverflowError + * + * @param value + * @return + * @param + */ public static 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"); } } diff --git a/Mage/src/main/java/mage/watchers/Watcher.java b/Mage/src/main/java/mage/watchers/Watcher.java index f13f37b5dad..490e7b3bdc6 100644 --- a/Mage/src/main/java/mage/watchers/Watcher.java +++ b/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); } } diff --git a/Mage/src/main/java/mage/watchers/Watchers.java b/Mage/src/main/java/mage/watchers/Watchers.java index 79058d08171..0e842186486 100644 --- a/Mage/src/main/java/mage/watchers/Watchers.java +++ b/Mage/src/main/java/mage/watchers/Watchers.java @@ -25,8 +25,11 @@ public class Watchers extends HashMap { 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) { diff --git a/Mage/src/main/java/mage/watchers/common/CommanderInfoWatcher.java b/Mage/src/main/java/mage/watchers/common/CommanderInfoWatcher.java index 01deb52095b..cd5d1681808 100644 --- a/Mage/src/main/java/mage/watchers/common/CommanderInfoWatcher.java +++ b/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 entry : damageToPlayer.entrySet()) { Player damagedPlayer = game.getPlayer(entry.getKey()); sb.append("").append(commanderTypeName).append(" 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(), "" + commanderTypeName + " 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);