multiple player/opponent choose - fixed that game ask players in random order instead APNAP (closes #12532);

game: fixed wrong player restore for TestPlayer and SimulatedPlayer2;

Mage.Common/src/main/java/mage/view/CardView.java

@@ -993,7 +993,6 @@ public class CardView extends SimpleCardView {
                 && a.getManaCostStr().equals(b.getManaCostStr())
                 && a.getRules().equals(b.getRules())
                 && Objects.equals(a.getRarity(), b.getRarity())
-
                 && a.getFrameStyle() == b.getFrameStyle()
                 && Objects.equals(a.getCounters(), b.getCounters())
                 && a.isFaceDown() == b.isFaceDown())) {
@@ -1004,6 +1003,7 @@ public class CardView extends SimpleCardView {
                 && Objects.equals(a.getCardNumber(), b.getCardNumber())
                 && Objects.equals(a.getImageNumber(), b.getImageNumber())
                 && Objects.equals(a.getImageFileName(), b.getImageFileName())
+                && Objects.equals(a.getUsesVariousArt(), b.getUsesVariousArt())
         )) {
             return false;
         }

Mage.Server.Plugins/Mage.Player.AI.MA/src/mage/player/ai/SimulatedPlayer2.java

@@ -35,6 +35,7 @@ public final class SimulatedPlayer2 extends ComputerPlayer {
 
     private static final boolean AI_SIMULATE_ALL_BAD_AND_GOOD_TARGETS = false; // TODO: enable and do performance test (it's increase calculations by x2, but is it useful?)
 
+    // warning, simulated player do not restore own data by game rollback
     private final boolean isSimulatedPlayer;
     private transient ConcurrentLinkedQueue<Ability> allActions; // all possible abilities to play (copies with already selected targets)
     private final Player originalPlayer; // copy of the original player, source of choices/results in tests
@@ -54,6 +55,17 @@ public final class SimulatedPlayer2 extends ComputerPlayer {
         this.originalPlayer = player.originalPlayer.copy();
     }
 
+    @Override
+    public void restore(Player player) {
+        // simulated player can be created from any player type
+        if (!originalPlayer.getClass().equals(player.getClass())) {
+            throw new IllegalArgumentException("Wrong code usage: simulated player must use same player class all the time. Need "
+                    + originalPlayer.getClass().getSimpleName() + ", but try to restore " + player.getClass().getSimpleName());
+        }
+
+        super.restore(player.getRealPlayer());
+    }
+
     @Override
     public SimulatedPlayer2 copy() {
         return new SimulatedPlayer2(this);

Mage.Tests/src/test/java/org/mage/test/multiplayer/BloodchiefAscensionTest.java

@@ -1,4 +1,3 @@
-
 package org.mage.test.multiplayer;
 
 import mage.constants.PhaseStep;
@@ -9,13 +8,12 @@ import org.junit.Test;
 import org.mage.test.serverside.base.CardTestMultiPlayerBase;
 
 /**
- *
+ * @author LevelX2, JayDi85
- * @author LevelX2
  */
 public class BloodchiefAscensionTest extends CardTestMultiPlayerBase {
 
     @Test
-    public void testBloodchiefAscensionAllPlayers() {
+    public void test_BloodchiefAscension_AllAlive() {
         // Enchantment
         // At the beginning of each end step, if an opponent lost 2 or more life this turn, you may put a quest counter on Bloodchief Ascension. (Damage causes loss of life.)
         // Whenever a card is put into an opponent's graveyard from anywhere, if Bloodchief Ascension has three or more quest counters on it, you may have that player lose 2 life. If you do, you gain 2 life.
@@ -30,19 +28,41 @@ public class BloodchiefAscensionTest extends CardTestMultiPlayerBase {
         addCard(Zone.BATTLEFIELD, playerB, "Mountain", 3);
         addCard(Zone.HAND, playerB, "Fireball");
 
+        // Player order: A -> D -> C -> B
+        runCode("before", 1, PhaseStep.PRECOMBAT_MAIN, playerA, (info, player, game) -> {
+            // make sure range used
+            Assert.assertTrue(playerA.hasPlayerInRange(playerA.getId()));
+            Assert.assertTrue(playerA.hasPlayerInRange(playerD.getId()));
+            Assert.assertFalse(playerA.hasPlayerInRange(playerC.getId()));
+            Assert.assertTrue(playerA.hasPlayerInRange(playerB.getId()));
+        });
+
+        // turn 1
+        // damage itself, no quest counters gain
         castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Fireball", playerA);
         setChoice(playerA, "X=2");
+        setChoice(playerA, true); // put quest counter on end of turn 2
 
+        // turn 2
+        checkPermanentCounters("after turn 1", 2, PhaseStep.UPKEEP, playerA, "Bloodchief Ascension", CounterType.QUEST, 0);
+        // damage opponent, +1 quest counter
         castSpell(2, PhaseStep.PRECOMBAT_MAIN, playerD, "Fireball", playerD);
         setChoice(playerD, "X=2");
 
+        // turn 3
+        checkPermanentCounters("after turn 2", 3, PhaseStep.UPKEEP, playerA, "Bloodchief Ascension", CounterType.QUEST, 0 + 1);
+        // damage out of range player, no quest counter
         castSpell(3, PhaseStep.PRECOMBAT_MAIN, playerC, "Fireball", playerC);
         setChoice(playerC, "X=2");
 
+        // turn 4
+        checkPermanentCounters("after turn 3", 4, PhaseStep.UPKEEP, playerA, "Bloodchief Ascension", CounterType.QUEST, 0 + 1 + 0);
+        // damage opponent, +1 quest counter
         castSpell(4, PhaseStep.PRECOMBAT_MAIN, playerB, "Fireball", playerB);
         setChoice(playerB, "X=2");
+        setChoice(playerA, true); // put quest counter on end of turn 4
 
-        // Player order: A -> D -> C -> B
+        setStrictChooseMode(true);
         setStopAt(4, PhaseStep.END_TURN);
         execute();
 
@@ -65,7 +85,7 @@ public class BloodchiefAscensionTest extends CardTestMultiPlayerBase {
      * play. I took lethal damage on my turn, but they didn't get a counter on
      * Bloodchief Ascension at my end step. I think they should, even though I
      * had left the game from dying, because of:
-     *
+     * <p>
      * 800.4g. If a player leaves the game during their turn, that turn
      * continues to its completion without an active player. If the active
      * player would receive priority, instead the next player in turn order
@@ -73,7 +93,7 @@ public class BloodchiefAscensionTest extends CardTestMultiPlayerBase {
      * or step ends, whichever is appropriate.
      */
     @Test
-    public void testBloodchiefAscension() {
+    public void test_BloodchiefAscension_DieBeforeEndTurn() {
         // Enchantment
         // At the beginning of each end step, if an opponent lost 2 or more life this turn, you may put a quest counter on Bloodchief Ascension. (Damage causes loss of life.)
         // Whenever a card is put into an opponent's graveyard from anywhere, if Bloodchief Ascension has three or more quest counters on it, you may have that player lose 2 life. If you do, you gain 2 life.
@@ -87,19 +107,42 @@ public class BloodchiefAscensionTest extends CardTestMultiPlayerBase {
         addCard(Zone.BATTLEFIELD, playerB, "Mountain", 21);
         addCard(Zone.HAND, playerB, "Fireball");
 
+        // Player order: A -> D -> C -> B
+        runCode("before", 1, PhaseStep.PRECOMBAT_MAIN, playerA, (info, player, game) -> {
+            // make sure range used
+            Assert.assertTrue(playerA.hasPlayerInRange(playerA.getId()));
+            Assert.assertTrue(playerA.hasPlayerInRange(playerD.getId()));
+            Assert.assertFalse(playerA.hasPlayerInRange(playerC.getId()));
+            Assert.assertTrue(playerA.hasPlayerInRange(playerB.getId()));
+
+        });
+
+        // turn 1
+        // damage itself, no quest counters gain
         castSpell(1, PhaseStep.PRECOMBAT_MAIN, playerA, "Fireball", playerA);
         setChoice(playerA, "X=2");
+        setChoice(playerA, true); // put quest counter on end of turn 2
 
+        // turn 2
+        checkPermanentCounters("after turn 1", 2, PhaseStep.UPKEEP, playerA, "Bloodchief Ascension", CounterType.QUEST, 0);
+        // damage opponent, +1 quest counter
         castSpell(2, PhaseStep.PRECOMBAT_MAIN, playerD, "Fireball", playerD);
         setChoice(playerD, "X=2");
 
+        // turn 3
+        checkPermanentCounters("after turn 2", 3, PhaseStep.UPKEEP, playerA, "Bloodchief Ascension", CounterType.QUEST, 0 + 1);
+        // damage out of range player, no quest counter
         castSpell(3, PhaseStep.PRECOMBAT_MAIN, playerC, "Fireball", playerC);
         setChoice(playerC, "X=2");
 
+        // turn 4
+        checkPermanentCounters("after turn 3", 4, PhaseStep.UPKEEP, playerA, "Bloodchief Ascension", CounterType.QUEST, 0 + 1 + 0);
+        // opponent kills itself, turn continue to the end and must give +1 counters
         castSpell(4, PhaseStep.PRECOMBAT_MAIN, playerB, "Fireball", playerB);
         setChoice(playerB, "X=20");
+        setChoice(playerA, true); // put quest counter on end of turn 4
 
-        // Player order: A -> D -> C -> B
+        setStrictChooseMode(true);
         setStopAt(4, PhaseStep.END_TURN);
         execute();
 

Mage.Tests/src/test/java/org/mage/test/multiplayer/PlayersListAndOrderTest.java

@@ -0,0 +1,209 @@
+package org.mage.test.multiplayer;
+
+import mage.constants.MultiplayerAttackOption;
+import mage.constants.PhaseStep;
+import mage.constants.RangeOfInfluence;
+import mage.game.FreeForAll;
+import mage.game.Game;
+import mage.game.GameException;
+import mage.game.mulligan.MulliganType;
+import mage.players.Player;
+import mage.players.PlayerList;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mage.test.serverside.base.CardTestMultiPlayerBase;
+
+import java.io.FileNotFoundException;
+import java.util.*;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+/**
+ * @author JayDi85
+ */
+public class PlayersListAndOrderTest extends CardTestMultiPlayerBase {
+
+    // problem: some player's list can return random/unstable order (HashSet or HashMap instead linked list)
+    // so make sure all relates players list return stable/linked collection, not random
+    // also make sure some lists return in turn order
+    // TODO: add reverse turn order support from Aeon Engine
+    private List<UUID> needByAddedAll = new ArrayList<>();
+    private List<UUID> needByApnapAllFromA = new ArrayList<>();
+    private List<UUID> needByApnapAllFromB = new ArrayList<>();
+    private List<UUID> needByApnapOpponentsFromA = new ArrayList<>();
+    private List<UUID> needByApnapOpponentsFromB = new ArrayList<>();
+
+    @Override
+    protected Game createNewGameAndPlayers() throws GameException, FileNotFoundException {
+        // Start Life = 2
+        Game game = new FreeForAll(MultiplayerAttackOption.MULTIPLE, RangeOfInfluence.ALL, MulliganType.GAME_DEFAULT.getMulligan(0), 2, 7);
+        // Player order: A -> D -> C -> B
+        playerA = createPlayer(game, "PlayerA");
+        playerB = createPlayer(game, "PlayerB");
+        playerC = createPlayer(game, "PlayerC");
+        playerD = createPlayer(game, "PlayerD");
+
+        needByAddedAll.addAll(Arrays.asList(
+                playerA.getId(),
+                playerB.getId(),
+                playerC.getId(),
+                playerD.getId()
+        ));
+        needByApnapAllFromA.addAll(Arrays.asList(
+                playerA.getId(),
+                playerD.getId(),
+                playerC.getId(),
+                playerB.getId()
+        ));
+        needByApnapAllFromB.addAll(Arrays.asList(
+                playerB.getId(),
+                playerA.getId(),
+                playerD.getId(),
+                playerC.getId()
+        ));
+        needByApnapOpponentsFromA.addAll(Arrays.asList(
+                playerD.getId(),
+                playerC.getId(),
+                playerB.getId()
+        ));
+        needByApnapOpponentsFromB.addAll(Arrays.asList(
+                playerA.getId(),
+                playerD.getId(),
+                playerC.getId()
+        ));
+
+        return game;
+    }
+
+    private void assertPlayersListFromCollection(String info, List<UUID> need, Collection<UUID> source) {
+        List<UUID> byArray = new ArrayList<>(source);
+        assertPlayersList("toArray - " + info, need, byArray);
+
+        // warning, do not optimize code here by IDE - it must use stream/iterator, not toArray (toArray called in addAll or constructor);
+        List<UUID> byIterator = new ArrayList<>(source).stream().collect(Collectors.toList());
+        assertPlayersList("iterator - " + info, need, byIterator);
+    }
+
+    private void assertPlayersList(String info, List<UUID> need, List<UUID> current) {
+        int maxCheckCount = 100;
+        IntStream.rangeClosed(1, maxCheckCount).forEach(i -> {
+            if (!need.equals(current)) {
+                List<Player> needPlayers = need.stream().map(id -> currentGame.getPlayer(id)).collect(Collectors.toList());
+                List<Player> currentPlayers = current.stream().map(id -> currentGame.getPlayer(id)).collect(Collectors.toList());
+                Assert.fail(info + "\n" + "need: " + needPlayers + "\n" + "find: " + currentPlayers);
+            }
+        });
+    }
+
+    @Test
+    public void test_Game_GetPlayers() {
+        // game.getPlayers() - added order, for inner game engine only
+
+        runCode("check", 1, PhaseStep.PRECOMBAT_MAIN, playerA, (info, player, game) -> {
+            List<UUID> byValues = currentGame.getPlayers().values()
+                    .stream()
+                    .map(Player::getId)
+                    .collect(Collectors.toList());
+            assertPlayersList("game.getPlayers().values() - must return in added order", needByAddedAll, byValues);
+
+            List<UUID> byKeys = new ArrayList<>(currentGame.getPlayers().keySet());
+            assertPlayersList("game.getPlayers().keySet() - must return in added order", needByAddedAll, byKeys);
+        });
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.END_TURN);
+        execute();
+    }
+
+    @Test
+    public void test_Game_GetPlayerList() {
+        // game.getPlayerList() - APNAP order, for cards usage
+
+        PlayerList list = new PlayerList();
+        list.add(playerA.getId());
+        list.add(playerB.getId());
+        list.add(playerC.getId());
+        list.add(playerD.getId());
+        list.setCurrent(playerA.getId());
+        List<UUID> current = new ArrayList<>(list);
+        assertPlayersList("for fast debug", needByApnapAllFromA, current);
+
+        runCode("check", 1, PhaseStep.PRECOMBAT_MAIN, playerA, (info, player, game) -> {
+            assertPlayersListFromCollection(
+                    "game.getPlayerList() - must return in APNAP order",
+                    needByApnapAllFromA,
+                    currentGame.getPlayerList()
+            );
+        });
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.END_TURN);
+        execute();
+    }
+
+    @Test
+    public void test_Game_GetOpponents() {
+        // game.getOpponents() - APNAP order, for cards usage
+
+        runCode("check", 1, PhaseStep.PRECOMBAT_MAIN, playerA, (info, player, game) -> {
+            assertPlayersListFromCollection(
+                    "game.getOpponents(from player A) - must return in APNAP order",
+                    needByApnapOpponentsFromA,
+                    currentGame.getOpponents(playerA.getId())
+            );
+            assertPlayersListFromCollection(
+                    "game.getOpponents(from player B) - must return in APNAP order",
+                    needByApnapOpponentsFromB,
+                    currentGame.getOpponents(playerB.getId())
+            );
+        });
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.END_TURN);
+        execute();
+    }
+
+    @Test
+    public void test_GameState_GetPlayerList() {
+        // game.getState().getPlayerList() - APNAP order, for cards usage
+
+        runCode("check", 1, PhaseStep.PRECOMBAT_MAIN, playerA, (info, player, game) -> {
+            assertPlayersListFromCollection(
+                    "game.getState().getPlayerList(from current player) - must return in APNAP order",
+                    needByApnapAllFromA,
+                    currentGame.getState().getPlayerList()
+            );
+            assertPlayersListFromCollection(
+                    "game.getState().getPlayerList(from player B) - must return in APNAP order",
+                    needByApnapAllFromB,
+                    currentGame.getState().getPlayerList(playerB.getId())
+            );
+        });
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.END_TURN);
+        execute();
+    }
+
+    @Test
+    public void test_GameState_GetPlayersInRange() {
+        // game.getState().getPlayersInRange(player, game) - APNAP order, for cards usage
+
+        runCode("check", 1, PhaseStep.PRECOMBAT_MAIN, playerA, (info, player, game) -> {
+            assertPlayersListFromCollection(
+                    "game.getState().getPlayersInRange(from player A) - must return in APNAP order",
+                    needByApnapAllFromA,
+                    currentGame.getState().getPlayersInRange(playerA.getId(), currentGame)
+            );
+            assertPlayersListFromCollection(
+                    "game.getState().getPlayersInRange(from player B) - must return in APNAP order",
+                    needByApnapAllFromB,
+                    currentGame.getState().getPlayersInRange(playerB.getId(), currentGame)
+            );
+        });
+
+        setStrictChooseMode(true);
+        setStopAt(1, PhaseStep.END_TURN);
+        execute();
+    }
+}

Mage.Tests/src/test/java/org/mage/test/player/TestPlayer.java

@@ -54,7 +54,6 @@ import mage.util.MultiAmountMessage;
 import mage.util.RandomUtil;
 import org.apache.log4j.Logger;
 import org.junit.Assert;
-import org.junit.Ignore;
 
 import java.io.Serializable;
 import java.util.*;
@@ -69,7 +68,6 @@ import static org.mage.test.serverside.base.impl.CardTestPlayerAPIImpl.*;
  *
  * @author BetaSteward_at_googlemail.com, Simown, JayDi85
  */
-@Ignore
 public class TestPlayer implements Player {
 
     private static final Logger LOGGER = Logger.getLogger(TestPlayer.class);
@@ -90,6 +88,8 @@ public class TestPlayer implements Player {
     private int maxCallsWithoutAction = 400;
     private int foundNoAction = 0;
 
+    // warning, test player do not restore own data by game rollback
+
     // full playable AI, TODO: can be deleted?
     private boolean AIPlayer;
     // AI simulates a real game, e.g. ignores strict mode and play command/priority, see aiXXX commands
@@ -2600,7 +2600,7 @@ public class TestPlayer implements Player {
                 targetCardZonesChecked.add(Zone.GRAVEYARD);
                 TargetCard targetFull = (TargetCard) target.getOriginalTarget();
 
-                List<UUID> needPlayers = game.getState().getPlayersInRange(this.getId(), game).toList();
+                List<UUID> needPlayers = new ArrayList<>(game.getState().getPlayersInRange(this.getId(), game));
                 // fix for opponent graveyard
                 if (target.getOriginalTarget() instanceof TargetCardInOpponentsGraveyard) {
                     // current player remove
@@ -2947,8 +2947,12 @@ public class TestPlayer implements Player {
 
     @Override
     public void restore(Player player) {
-        // no rollback for test player meta data (modesSet, actions, choices, targets, aliases)
+        if (!(player instanceof TestPlayer)) {
-        computerPlayer.restore(player);
+            throw new IllegalArgumentException("Wrong code usage: can't restore from player class " + player.getClass().getName());
+        }
+
+        // no rollback for test player metadata (modesSet, actions, choices, targets, aliases, etc)
+        computerPlayer.restore(player.getRealPlayer());
     }
 
     @Override
@@ -2984,8 +2988,8 @@ public class TestPlayer implements Player {
     }
 
     @Override
-    public Set<UUID> getInRange() {
+    public boolean hasPlayerInRange(UUID checkingPlayerId) {
-        return computerPlayer.getInRange();
+        return computerPlayer.hasPlayerInRange(checkingPlayerId);
     }
 
     @Override
@@ -4619,4 +4623,9 @@ public class TestPlayer implements Player {
         //Assert.fail("Wrong choice command: " + choice);
         LOGGER.warn("Wrong choice command: " + choice);
     }
+
+    @Override
+    public Player getRealPlayer() {
+        return this.computerPlayer;
+    }
 }

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

@@ -158,31 +158,42 @@ public interface Game extends MageItem, Serializable, Copyable<Game> {
 
     Player getPlayerOrPlaneswalkerController(UUID playerId);
 
+    /**
+     * Static players list from start of the game. Use it to find player by ID.
+     */
     Players getPlayers();
 
+    /**
+     * Static players list from start of the game. Use it to interate by starting turn order.
+     * WARNING, it's ignore range and leaved players, so use it game engine only
+     */
     PlayerList getPlayerList();
 
+    /**
+     *  Returns opponents list in range for the given playerId. Use it to interate by starting turn order.
+     *  Warning, it will return dead players until end of turn.
+     */
     default Set<UUID> getOpponents(UUID playerId) {
         return getOpponents(playerId, false);
     }
 
     /**
-     * Returns a Set of opponents in range for the given playerId
+     *  Returns opponents list in range for the given playerId. Use it to interate by starting turn order.
+     *  Warning, it will return dead players until end of turn.
      *
-     * @param playerId
+     * @param excludeDeadPlayers exclude dead player immediately without waiting range update on next turn
-     * @param excludeDeadPlayers Determines if players who have lost are excluded from the list
-     * @return
      */
     default Set<UUID> getOpponents(UUID playerId, boolean excludeDeadPlayers) {
         Player player = getPlayer(playerId);
         if (player == null) {
-            return new HashSet<>();
+            return new LinkedHashSet<>();
         }
 
-        return player.getInRange().stream()
+        return this.getPlayerList().stream()
                 .filter(opponentId -> !opponentId.equals(playerId))
+                .filter(player::hasPlayerInRange)
                 .filter(opponentId -> !excludeDeadPlayers || !getPlayer(opponentId).hasLost())
-                .collect(Collectors.toSet());
+                .collect(Collectors.toCollection(LinkedHashSet::new));
     }
 
     default boolean isActivePlayer(UUID playerId) {

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

@@ -3183,8 +3183,6 @@ public abstract class GameImpl implements Game {
 
     /**
      * Return a list of all players ignoring the range of visible players
-     *
-     * @return
      */
     @Override
     public PlayerList getPlayerList() {

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

@@ -62,8 +62,8 @@ public class GameState implements Serializable, Copyable<GameState> {
     // warning, do not use another keys with same starting text cause copy code search and clean all related values
     public static final String COPIED_CARD_KEY = "CopiedCard";
 
-    private final Players players;
+    private final Players players; // full players by ID (static list, table added order)
-    private final PlayerList playerList;
+    private final PlayerList playerList; // full players (static list, turn order e.g. apnap)
     private UUID choosingPlayerId; // player that makes a choice at game start
 
     // revealed cards <Name, <Cards>>, will be reset if all players pass priority
@@ -719,8 +719,6 @@ public class GameState implements Serializable, Copyable<GameState> {
     /**
      * Returns a list of all players of the game ignoring range or if a player
      * has lost or left the game.
-     *
-     * @return playerList
      */
     public PlayerList getPlayerList() {
         return playerList;
@@ -761,8 +759,9 @@ public class GameState implements Serializable, Copyable<GameState> {
         PlayerList newPlayerList = new PlayerList();
         Player currentPlayer = game.getPlayer(playerId);
         if (currentPlayer != null) {
+            // must fill PlayerList by table added order (same as main game)
             for (Player player : players.values()) {
-                if (player.isInGame() && currentPlayer.getInRange().contains(player.getId())) {
+                if (player.isInGame() && currentPlayer.hasPlayerInRange(player.getId())) {
                     newPlayerList.add(player.getId());
                 }
             }

Mage/src/main/java/mage/players/Player.java

@@ -296,7 +296,11 @@ public interface Player extends MageItem, Copyable<Player> {
 
     ManaPool getManaPool();
 
-    Set<UUID> getInRange();
+    /**
+     * Is checking player in range of current player.
+     * Warning, range list updates on start of the turn due rules.
+     */
+    boolean hasPlayerInRange(UUID checkingPlayerId);
 
     boolean isTopCardRevealed();
 
@@ -1250,4 +1254,10 @@ public interface Player extends MageItem, Copyable<Player> {
     }
 
     public UserData getControllingPlayersUserData(Game game);
+
+    /**
+     * Some player implementations (TestPlayer, Simulated) uses facade structure with hidden player object,
+     * so that's method helps to find real player that used by a game (in most use cases it's a PlayerImpl)
+     */
+    Player getRealPlayer();
 }

Mage/src/main/java/mage/players/PlayerImpl.java

@@ -146,7 +146,7 @@ public abstract class PlayerImpl implements Player, Serializable {
     protected boolean idleTimeout;
 
     protected RangeOfInfluence range;
-    protected Set<UUID> inRange = new HashSet<>(); // players list in current range of influence (updates each turn)
+    protected Set<UUID> inRange = new HashSet<>(); // players list in current range of influence (updates each turn due rules)
 
     protected boolean isTestMode = false;
     protected boolean canGainLife = true;
@@ -309,6 +309,10 @@ public abstract class PlayerImpl implements Player, Serializable {
      */
     @Override
     public void restore(Player player) {
+        if (!(player instanceof PlayerImpl)) {
+            throw new IllegalArgumentException("Wrong code usage: can't restore from player class " + player.getClass().getName());
+        }
+
         this.name = player.getName();
         this.human = player.isHuman();
         this.life = player.getLife();
@@ -357,7 +361,7 @@ public abstract class PlayerImpl implements Player, Serializable {
         this.attachments.addAll(player.getAttachments());
 
         this.inRange.clear();
-        this.inRange.addAll(player.getInRange());
+        this.inRange.addAll(((PlayerImpl) player).inRange);
         this.payLifeCostLevel = player.getPayLifeCostLevel();
         this.sacrificeCostFilter = player.getSacrificeCostFilter() != null
                 ? player.getSacrificeCostFilter().copy() : null;
@@ -579,16 +583,16 @@ public abstract class PlayerImpl implements Player, Serializable {
     }
 
     @Override
-    public Set<UUID> getInRange() {
+    public boolean hasPlayerInRange(UUID checkingPlayerId) {
         if (inRange.isEmpty()) {
             // runtime check: inRange filled on beginTurn, but unit tests adds cards by cheat engine before game starting,
             // so inRange will be empty and some ETB effects can be broken (example: Spark Double puts direct to battlefield).
             // Cheat engine already have a workaround, so that error must not be visible in normal situation.
             // TODO: that's error possible on GameView call before real game start (too laggy players on starting???)
-            throw new IllegalStateException("Wrong code usage (game is not started, but you call getInRange in some effects).");
+            throw new IllegalStateException("Wrong code usage (game is not started, but you call hasPlayerInRange in some effects).");
         }
 
-        return inRange;
+        return inRange.contains(checkingPlayerId);
     }
 
     @Override
@@ -5130,7 +5134,7 @@ public abstract class PlayerImpl implements Player, Serializable {
     public boolean hasOpponent(UUID playerToCheckId, Game game) {
         return !this.getId().equals(playerToCheckId)
                 && game.isOpponent(this, playerToCheckId)
-                && getInRange().contains(playerToCheckId);
+                && hasPlayerInRange(playerToCheckId);
     }
 
     @Override
@@ -5470,4 +5474,9 @@ public abstract class PlayerImpl implements Player, Serializable {
     public String toString() {
         return getName() + " (" + super.getClass().getSimpleName() + ")";
     }
+
+    @Override
+    public Player getRealPlayer() {
+        return this;
+    }
 }

Mage/src/main/java/mage/players/PlayerList.java

@@ -27,7 +27,7 @@ public class PlayerList extends CircularList<UUID> {
         UUID currentPlayerBefore = this.get();
         UUID nextPlayerId = game.isTurnOrderReversed() ? super.getPrevious() : super.getNext();
         do {
-            if (basePlayer.getInRange().contains(nextPlayerId)) {
+            if (basePlayer.hasPlayerInRange(nextPlayerId)) {
                 return game.getPlayer(nextPlayerId);
             }
             nextPlayerId = game.isTurnOrderReversed() ? super.getPrevious() : super.getNext();

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

@@ -1,20 +1,13 @@
-
 package mage.util;
 
 import java.io.Serializable;
-import java.util.ArrayList;
+import java.util.*;
-import java.util.Collection;
-import java.util.ConcurrentModificationException;
-import java.util.Iterator;
-import java.util.List;
-import java.util.ListIterator;
 import java.util.concurrent.locks.ReentrantLock;
 
 /**
- * a thread-safe circular list
+ * Component: a thread-safe circular list, used for players list
  *
- * @author BetaSteward_at_googlemail.com
+ * @author BetaSteward_at_googlemail.com, JayDi85
- * @param <E>
  */
 public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
     //TODO: might have to make E extend Copyable
@@ -42,17 +35,17 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
     }
 
     /**
-     * Inserts an element into the current position
+     * Insert new element at the current position (make new element as current)
-     *
-     * @param e
-     * @return
      */
     @Override
-    public boolean add(E e) {
+    public boolean add(E element) {
-        list.add(this.index, e);
+        add(this.index, element);
         return true;
     }
 
+    /**
+     * Insert new element at selected position (keep old element as current)
+     */
     @Override
     public void add(int index, E element) {
         lock.lock();
@@ -65,35 +58,36 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
     }
 
     /**
+     * Set current position to an element
      *
-     * @param e the element to set as current
+     * @return false on unknown element
-     * @return true if element e exists and index was set
      */
-    public boolean setCurrent(E e) {
+    public boolean setCurrent(E element) {
-        if (list.contains(e)) {
+        if (list.contains(element)) {
-            this.index = list.indexOf(e);
+            this.index = list.indexOf(element);
             return true;
         }
         return false;
     }
 
     /**
-     * Retrieves the element at the current position
+     * Find current element
-     *
-     * @return
      */
     public E get() {
+        return get(this.index);
+    }
+
+    /**
+     * Find element at searching position
+     */
+    @Override
+    public E get(int index) {
         if (list.size() > this.index) {
             return list.get(this.index);
         }
         return null;
     }
 
-    @Override
-    public E get(int index) {
-        return list.get(index);
-    }
-
     /**
      * Returns the next element in the list. Will loop around to the beginning
      * of the list if the current element is the last.
@@ -117,18 +111,23 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
     /**
      * Removes the current element from the list
      *
-     * @return <tt>true</tt> is the item was successfully removed
+     * @return true on successfully removed
      */
     public boolean remove() {
         return this.remove(get());
     }
 
+    /**
+     * Removes element from searching position
+     *
+     * @return true on successfully removed
+     */
     @Override
     public E remove(int index) {
         lock.lock();
         try {
             E ret = list.remove(index);
-            checkPointer();
+            fixInvalidPointer();
             modCount++;
             return ret;
         } finally {
@@ -141,7 +140,7 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
         lock.lock();
         try {
             boolean ret = list.remove(o);
-            checkPointer();
+            fixInvalidPointer();
             modCount++;
             return ret;
         } finally {
@@ -152,50 +151,49 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
     private int incrementPointer() {
         lock.lock();
         try {
-            index = incrementListPointer(index);
+            index = findNextListPointer(index);
             return index;
         } finally {
             lock.unlock();
         }
     }
 
-    private int incrementListPointer(int index) {
+    private int findNextListPointer(int currentIndex) {
-        index++;
+        currentIndex++;
-        if (index >= list.size()) {
+        if (currentIndex >= list.size()) {
-            index = 0;
+            currentIndex = 0;
         }
-        return index;
+        return currentIndex;
     }
 
     private int decrementPointer() {
         lock.lock();
         try {
-            index = decrementListPointer(index);
+            index = findPrevListPointer(index);
             return index;
         } finally {
             lock.unlock();
         }
     }
 
-    private int decrementListPointer(int index) {
+    private int findPrevListPointer(int currentIndex) {
-        index--;
+        currentIndex--;
-        if (index < 0) {
+        if (currentIndex < 0) {
-            index = list.size() - 1;
+            currentIndex = list.size() - 1;
         }
-        return index;
+        return currentIndex;
     }
 
     /**
      * This method should only be called from a locked method thus it is not
      * necessary to lock from this method
      */
-    private int checkPointer() {
+    private void fixInvalidPointer() {
         if (index > list.size()) {
             index = list.size() - 1;
         } else if (index < 0) {
             index = 0;
         }
-        return index;
     }
 
     @Override
@@ -215,16 +213,22 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
 
     @Override
     public Object[] toArray() {
-        return list.toArray();
+        return toArray(new UUID[0]);
     }
 
     @Override
     public <T> T[] toArray(T[] a) {
-        return list.toArray(a);
+        T[] res = list.toArray(a);
+
+        // sort due default order
+        Iterator<E> iter = this.iterator();
+        int insertIndex = 0;
+        while (iter.hasNext()) {
+            res[insertIndex] = (T) iter.next();
+            insertIndex++;
         }
 
-    public List<E> toList() {
+        return res;
-        return list;
     }
 
     @Override
@@ -254,7 +258,7 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
         try {
             boolean ret = list.removeAll(c);
             modCount++;
-            checkPointer();
+            fixInvalidPointer();
             return ret;
         } finally {
             lock.unlock();
@@ -267,7 +271,7 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
         try {
             boolean ret = list.retainAll(c);
             modCount++;
-            checkPointer();
+            fixInvalidPointer();
             return ret;
         } finally {
             lock.unlock();
@@ -286,6 +290,10 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
         }
     }
 
+    public E set(E element) {
+        return this.set(this.index, element);
+    }
+
     @Override
     public E set(int index, E element) {
         lock.lock();
@@ -297,10 +305,6 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
         }
     }
 
-    public E set(E element) {
-        return this.set(this.index, element);
-    }
-
     @Override
     public int indexOf(Object o) {
         return list.indexOf(o);
@@ -361,7 +365,7 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
                 throw new ConcurrentModificationException();
             }
             E data = (E) list.get(cursor);
-            cursor = incrementListPointer(cursor);
+            cursor = findNextListPointer(cursor);
             hasMoved = true;
             return data;
         }
@@ -409,7 +413,7 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
                 throw new ConcurrentModificationException();
             }
             E data = (E) list.get(cursor);
-            cursor = incrementListPointer(cursor);
+            cursor = findNextListPointer(cursor);
             hasMoved = true;
             return data;
         }
@@ -430,7 +434,7 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
             if (curModCount != modCount) {
                 throw new ConcurrentModificationException();
             }
-            cursor = decrementListPointer(cursor);
+            cursor = findPrevListPointer(cursor);
             hasMoved = true;
             return (E) list.get(cursor);
         }
@@ -438,7 +442,7 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
         @Override
         public int nextIndex() {
             if (this.hasNext()) {
-                return incrementListPointer(cursor);
+                return findNextListPointer(cursor);
             }
             return list.size();
         }
@@ -446,7 +450,7 @@ public class CircularList<E> implements List<E>, Iterable<E>, Serializable {
         @Override
         public int previousIndex() {
             if (this.hasPrevious()) {
-                return decrementListPointer(cursor);
+                return findPrevListPointer(cursor);
             }
             return -1;
         }