refactor: simplified extra turn code, fixed NPE error on usage (Alchemist's Gambit)

Mage.Server.Plugins/Mage.Player.Human/src/mage/player/human/HumanPlayer.java

@@ -1003,6 +1003,8 @@ public class HumanPlayer extends PlayerImpl {
                 }
             }
 
+            // TODO: check that all skips and stops used from real controlling player
+            //  like holdingPriority (is it a bug here?)
             if (getJustActivatedType() != null && !holdingPriority) {
                 if (controllingPlayer.getUserData().isPassPriorityCast()
                         && getJustActivatedType() == AbilityType.SPELL) {

Mage.Server/src/main/java/mage/server/game/GameController.java

@@ -1212,8 +1212,8 @@ public class GameController implements GameCallback {
         sb.append(state.getStepNum());
         sb.append("<br>getTurn: ");
         sb.append(state.getTurn());
-        sb.append("<br>getTurnId: ");
+        sb.append("<br>getExtraTurnId: ");
-        sb.append(state.getTurnId());
+        sb.append(state.getExtraTurnId());
         sb.append("<br>getTurnMods: ");
         sb.append(state.getTurnMods());
         sb.append("<br>getTurnNum: ");

Mage.Sets/src/mage/cards/a/AlchemistsGambit.java

@@ -93,6 +93,6 @@ class AlchemistsGambitEffect extends ReplacementEffectImpl {
 
     @Override
     public boolean applies(GameEvent event, Ability source, Game game) {
-        return game.getState().getTurnId().equals(turnId);
+        return this.turnId != null && turnId.equals(game.getState().getExtraTurnId());
     }
 }

Mage.Tests/src/test/java/org/mage/test/turnmod/ExtraTurnsTest.java

@@ -2,104 +2,92 @@ package org.mage.test.turnmod;
 
 import mage.constants.PhaseStep;
 import mage.constants.Zone;
+import mage.players.Player;
 import org.junit.Assert;
 import org.junit.Test;
+import org.mage.test.player.TestPlayer;
 import org.mage.test.serverside.base.CardTestPlayerBase;
 
 /**
- *
+ * @author LevelX2, JayDi85
- * @author LevelX2
  */
 public class ExtraTurnsTest extends CardTestPlayerBase {
 
-    /**
+    private void checkTurnControl(int turn, TestPlayer needTurnController, boolean isExtraTurn) {
-     * Emrakul, the Promised End not giving an extra turn when cast in the
+        runCode("checking turn " + turn, turn, PhaseStep.POSTCOMBAT_MAIN, playerA, (info, player, game) -> {
-     * opponent's turn
+            Player defaultTurnController = game.getPlayer(game.getActivePlayerId());
-     */
+            Player realTurnController = defaultTurnController.getTurnControlledBy() == null ? defaultTurnController : game.getPlayer(defaultTurnController.getTurnControlledBy());
-    @Test
+            Assert.assertEquals(String.format("turn %d must be controlled by %s", turn, needTurnController.getName()),
-    public void testEmrakulCastOnOpponentsTurnCheckTurn3() {
+                    needTurnController.getName(), realTurnController.getName());
-        addCard(Zone.BATTLEFIELD, playerA, "Island", 12);
+            Assert.assertEquals(String.format("turn %d must be %s", turn, (isExtraTurn ? "extra turn" : "normal turn")),
-        addCard(Zone.GRAVEYARD, playerA, "Island", 1);
+                    isExtraTurn, game.getState().isExtraTurn());
-        // Emrakul, the Promised End costs {1} less to cast for each card type among cards in your graveyard.
+        });
-        // When you cast Emrakul, you gain control of target opponent during that player's next turn. After that turn, that player takes an extra turn.
-        // Flying
-        // Trample
-        // Protection from instants
-        addCard(Zone.HAND, playerA, "Emrakul, the Promised End", 1); // {13}
-        // Flash (You may cast this spell any time you could cast an instant.)
-        // Creature cards you own that aren't on the battlefield have flash.
-        // Each opponent can cast spells only any time they could cast a sorcery.
-        addCard(Zone.BATTLEFIELD, playerA, "Teferi, Mage of Zhalfir", 1);
-
-        castSpell(2, PhaseStep.PRECOMBAT_MAIN, playerA, "Emrakul, the Promised End");
-        // Turn 4 is the next turn of opponent (player B)  that player A controls
-        // So Turn 5 is the extra turn for player B after Turn 4
-        setStopAt(3, PhaseStep.DRAW);
-        execute();
-
-        assertPermanentCount(playerA, "Emrakul, the Promised End", 1);
-        Assert.assertTrue("Turn 3 is no extra turn ", !currentGame.getState().isExtraTurn());
-        Assert.assertEquals("For turn " + currentGame.getTurnNum() + ", playerA has to be the active player but active player is: "
-                + currentGame.getPlayer(currentGame.getActivePlayerId()).getName(), currentGame.getActivePlayerId(), playerA.getId());
     }
 
     @Test
-    public void testEmrakulCastOnOpponentsTurnCheckTurn4() {
+    public void test_EmrakulMustGiveExtraTurn_OnOwnTurn() {
-        addCard(Zone.BATTLEFIELD, playerA, "Island", 12);
-        addCard(Zone.GRAVEYARD, playerA, "Island", 1);
         // Emrakul, the Promised End costs {1} less to cast for each card type among cards in your graveyard.
         // When you cast Emrakul, you gain control of target opponent during that player's next turn. After that turn, that player takes an extra turn.
-        // Flying
-        // Trample
-        // Protection from instants
         addCard(Zone.HAND, playerA, "Emrakul, the Promised End", 1); // {13}
-        // Flash (You may cast this spell any time you could cast an instant.)
+        addCard(Zone.BATTLEFIELD, playerA, "Island", 12);
-        // Creature cards you own that aren't on the battlefield have flash.
+        addCard(Zone.GRAVEYARD, playerA, "Island", 1);
-        // Each opponent can cast spells only any time they could cast a sorcery.
-        addCard(Zone.BATTLEFIELD, playerA, "Teferi, Mage of Zhalfir", 1);
 
-        castSpell(2, PhaseStep.PRECOMBAT_MAIN, playerA, "Emrakul, the Promised End");
+        // cast emrakul on own turn and take extra turn
+        castSpell(3, PhaseStep.PRECOMBAT_MAIN, playerA, "Emrakul, the Promised End");
+        addTarget(playerA, playerB);
+
+        // checks for all turns
         // Turn 4 is the next turn of opponent (player B)  that player A controls
         // So Turn 5 is the extra turn for player B after Turn 4
-        setStopAt(4, PhaseStep.DRAW);
+        checkTurnControl(1, playerA, false);
-        execute();
+        checkTurnControl(2, playerB, false);
+        checkTurnControl(3, playerA, false);
+        checkTurnControl(4, playerA, false); // A take control of B's turn
+        checkTurnControl(5, playerB, true); // B take extra turn
+        checkTurnControl(6, playerA, false);
+        checkTurnControl(7, playerB, false);
 
-        assertPermanentCount(playerA, "Emrakul, the Promised End", 1);
+        setStrictChooseMode(true);
-        Assert.assertTrue("Turn 4 is a controlled turn ", !playerB.isGameUnderControl());
+        setStopAt(8, PhaseStep.END_TURN);
-        Assert.assertEquals("For turn " + currentGame.getTurnNum() + ", playerB has to be the active player but active player is: "
+        execute();
-                + currentGame.getPlayer(currentGame.getActivePlayerId()).getName(), currentGame.getActivePlayerId(), playerB.getId());
     }
 
     @Test
-    public void testEmrakulCastOnOpponentsTurnCheckTurn5() {
+    public void test_EmrakulMustGiveExtraTurn_OnOpponentTurn() {
-        addCard(Zone.BATTLEFIELD, playerA, "Island", 12);
-        addCard(Zone.GRAVEYARD, playerA, "Island", 1);
         // Emrakul, the Promised End costs {1} less to cast for each card type among cards in your graveyard.
         // When you cast Emrakul, you gain control of target opponent during that player's next turn. After that turn, that player takes an extra turn.
-        // Flying
-        // Trample
-        // Protection from instants
         addCard(Zone.HAND, playerA, "Emrakul, the Promised End", 1); // {13}
+        addCard(Zone.BATTLEFIELD, playerA, "Island", 12);
+        addCard(Zone.GRAVEYARD, playerA, "Island", 1);
+        //
         // Flash (You may cast this spell any time you could cast an instant.)
         // Creature cards you own that aren't on the battlefield have flash.
         // Each opponent can cast spells only any time they could cast a sorcery.
         addCard(Zone.BATTLEFIELD, playerA, "Teferi, Mage of Zhalfir", 1);
 
+        // cast emrakul on opponent's turn and take extra turn
         castSpell(2, PhaseStep.PRECOMBAT_MAIN, playerA, "Emrakul, the Promised End");
+        addTarget(playerA, playerB);
+
+        // checks for all turns
         // Turn 4 is the next turn of opponent (player B)  that player A controls
         // So Turn 5 is the extra turn for player B after Turn 4
-        setStopOnTurn(5);
+        checkTurnControl(1, playerA, false);
-        execute();
+        checkTurnControl(2, playerB, false);
+        checkTurnControl(3, playerA, false);
+        checkTurnControl(4, playerA, false); // A take control of B's turn
+        checkTurnControl(5, playerB, true); // B take extra turn
+        checkTurnControl(6, playerA, false);
+        checkTurnControl(7, playerB, false);
 
-        assertPermanentCount(playerA, "Emrakul, the Promised End", 1);
+        setStrictChooseMode(true);
-        Assert.assertTrue("Turn 5 is an extra turn ", currentGame.getState().isExtraTurn());
+        setStopAt(8, PhaseStep.END_TURN);
-        Assert.assertEquals("For turn " + currentGame.getTurnNum() + ", playerB has to be the active player but active player is: "
+        execute();
-                + currentGame.getPlayer(currentGame.getActivePlayerId()).getName(), currentGame.getActivePlayerId(), playerB.getId());
     }
 
     /**
      * https://github.com/magefree/mage/issues/6824
-     *
+     * <p>
      * When you cast miracled Temporal Mastery with God-Eternal Kefnet on the
      * battlefield and copy it with it's ability you get only 1 extra turn. It
      * should be 2, since you cast Temporal Mastery with it's miracle ability +
@@ -107,9 +95,7 @@ public class ExtraTurnsTest extends CardTestPlayerBase {
      * proceeds to next player.
      */
     @Test
-    public void testCopyMiracledTemporalMastery4TwoExtraTurns() {
+    public void test_MiracledTemporalMastery_MustGives2ExtraTurnsWithCopy() {
-        setStrictChooseMode(true);
-
         addCard(Zone.BATTLEFIELD, playerB, "Island", 7);
         // Flying
         // You may reveal the first card you draw each turn as you draw it. Whenever you reveal an instant or sorcery card this way,
@@ -129,15 +115,18 @@ public class ExtraTurnsTest extends CardTestPlayerBase {
         setChoice(playerB, false); // Would you like to reveal first drawn card? (Turn 3)
         setChoice(playerB, false); // Would you like to reveal first drawn card? (Turn 4)
 
+        // turn checks
         // Turn 3 + 4 are extra turns
-        setStopAt(4, PhaseStep.PRECOMBAT_MAIN);
+        checkTurnControl(1, playerA, false);
+        checkTurnControl(2, playerB, false);
+        checkTurnControl(3, playerB, true); // extra turn for B
+        checkTurnControl(4, playerB, true); // extra turn for B
+        checkTurnControl(5, playerA, false);
+
+        setStrictChooseMode(true);
+        setStopAt(5, PhaseStep.END_TURN);
         execute();
 
         assertExileCount(playerB, "Temporal Mastery", 1);
-
-        Assert.assertTrue("Turn 4 is an extra turn ", currentGame.getState().isExtraTurn());
-        Assert.assertEquals("For turn " + currentGame.getTurnNum() + ", playerB has to be the active player but active player is: "
-                + currentGame.getPlayer(currentGame.getActivePlayerId()).getName(), currentGame.getActivePlayerId(), playerB.getId());
     }
-
 }

Mage/src/main/java/mage/abilities/effects/common/turn/AddExtraTurnControllerEffect.java

@@ -82,16 +82,16 @@ public class AddExtraTurnControllerEffect extends OneShotEffect {
 
 class LoseGameDelayedTriggeredAbility extends DelayedTriggeredAbility {
 
-    private final UUID connectedTurnMod;
+    private final UUID turnId;
 
-    public LoseGameDelayedTriggeredAbility(UUID connectedTurnMod) {
+    public LoseGameDelayedTriggeredAbility(UUID turnId) {
         super(new LoseGameSourceControllerEffect(), Duration.EndOfGame);
-        this.connectedTurnMod = connectedTurnMod;
+        this.turnId = turnId;
     }
 
     public LoseGameDelayedTriggeredAbility(final LoseGameDelayedTriggeredAbility ability) {
         super(ability);
-        this.connectedTurnMod = ability.connectedTurnMod;
+        this.turnId = ability.turnId;
     }
 
     @Override
@@ -106,7 +106,7 @@ class LoseGameDelayedTriggeredAbility extends DelayedTriggeredAbility {
 
     @Override
     public boolean checkTrigger(GameEvent event, Game game) {
-        return connectedTurnMod != null && connectedTurnMod.equals(game.getState().getTurnId());
+        return this.turnId != null && this.turnId.equals(game.getState().getExtraTurnId());
     }
 
     @Override

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

@@ -206,6 +206,8 @@ public interface Game extends MageItem, Serializable, Copyable<Game> {
     /**
      * Id of the player the current turn it is.
      *
+     * Player can be under control of another player, so search a real GUI's controller by Player->getTurnControlledBy
+     *
      * @return
      */
     UUID getActivePlayerId();

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

@@ -1033,25 +1033,26 @@ public abstract class GameImpl implements Game {
     private boolean playExtraTurns() {
         //20091005 - 500.7
         TurnMod extraTurn = getNextExtraTurn();
-        while (extraTurn != null) {
+        try {
-            GameEvent event = new GameEvent(GameEvent.EventType.PLAY_TURN, null, null, extraTurn.getPlayerId());
+            while (extraTurn != null) {
-            if (!replaceEvent(event)) {
+                GameEvent event = new GameEvent(GameEvent.EventType.PLAY_TURN, null, null, extraTurn.getPlayerId());
-                Player extraPlayer = this.getPlayer(extraTurn.getPlayerId());
+                if (!replaceEvent(event)) {
-                if (extraPlayer != null && extraPlayer.canRespond()) {
+                    Player extraPlayer = this.getPlayer(extraTurn.getPlayerId());
-                    state.setExtraTurn(true);
+                    if (extraPlayer != null && extraPlayer.canRespond()) {
-                    state.setTurnId(extraTurn.getId());
+                        state.setExtraTurnId(extraTurn.getId());
-                    if (!this.isSimulation()) {
+                        if (!this.isSimulation()) {
-                        informPlayers(extraPlayer.getLogName() + " takes an extra turn");
+                            informPlayers(extraPlayer.getLogName() + " takes an extra turn");
-                    }
+                        }
-                    if (!playTurn(extraPlayer)) {
+                        if (!playTurn(extraPlayer)) {
-                        return false;
+                            return false;
+                        }
                     }
                 }
+                extraTurn = getNextExtraTurn();
             }
-            extraTurn = getNextExtraTurn();
+        } finally {
+            state.setExtraTurnId(null);
         }
-        state.setTurnId(null);
-        state.setExtraTurn(false);
         return true;
     }
 

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

@@ -91,8 +91,7 @@ public class GameState implements Serializable, Copyable<GameState> {
     private Battlefield battlefield;
     private int turnNum = 1;
     private int stepNum = 0;
-    private UUID turnId = null;
+    private UUID extraTurnId = null; // id of the current extra turn (null on normal turn or after game stopped)
-    private boolean extraTurn = false;
     private boolean gameOver;
     private boolean paused;
     private ContinuousEffects effects;
@@ -162,7 +161,7 @@ public class GameState implements Serializable, Copyable<GameState> {
         this.battlefield = state.battlefield.copy();
         this.turnNum = state.turnNum;
         this.stepNum = state.stepNum;
-        this.extraTurn = state.extraTurn;
+        this.extraTurnId = state.extraTurnId;
         this.effects = state.effects.copy();
         for (TriggeredAbility trigger : state.triggered) {
             this.triggered.add(trigger.copy());
@@ -229,7 +228,7 @@ public class GameState implements Serializable, Copyable<GameState> {
         companion.clear();
         turnNum = 1;
         stepNum = 0;
-        extraTurn = false;
+        extraTurnId = null;
         gameOver = false;
         specialActions.clear();
         cardState.clear();
@@ -265,7 +264,7 @@ public class GameState implements Serializable, Copyable<GameState> {
         this.battlefield = state.battlefield;
         this.turnNum = state.turnNum;
         this.stepNum = state.stepNum;
-        this.extraTurn = state.extraTurn;
+        this.extraTurnId = state.extraTurnId;
         this.effects = state.effects;
         this.triggered = state.triggered;
         this.triggers = state.triggers;
@@ -616,20 +615,16 @@ public class GameState implements Serializable, Copyable<GameState> {
         this.turnNum = turnNum;
     }
 
-    public UUID getTurnId() {
+    public UUID getExtraTurnId() {
-        return this.turnId;
+        return this.extraTurnId;
     }
 
-    public void setTurnId(UUID turnId) {
+    public void setExtraTurnId(UUID extraTurnId) {
-        this.turnId = turnId;
+        this.extraTurnId = extraTurnId;
     }
 
     public boolean isExtraTurn() {
-        return extraTurn;
+        return this.extraTurnId != null;
-    }
-
-    public void setExtraTurn(boolean extraTurn) {
-        this.extraTurn = extraTurn;
     }
 
     public boolean isGameOver() {

Mage/src/main/java/mage/game/turn/Step.java

@@ -9,6 +9,10 @@ import mage.game.events.GameEvent;
 import mage.game.events.GameEvent.EventType;
 
 /**
+ * Game's step
+ *
+ * Warning, don't use a changeable data in step's implementations
+ * TODO: implement copyable<> interface and copy usage in GameState
  *
  * @author BetaSteward_at_googlemail.com
  */