currentPlayer not advancing using TurnOrder.DEFAULT at phase end.

[nearwood]

I think I've found a new issue in v0.33 and newer. The current player is retained during the end of one phase and beginning of another one. I'm using TurnOrder.DEFAULT.

Here's a CodeSandbox of the issue reproduced with Jest, with the relevant code reproduced here:

import { TurnOrder } from "boardgame.io/core";

const Client = require("boardgame.io").Client;

const game = {
  setup: () => ({
    dice: 3
  }),
  turn: { moveLimit: 1, order: TurnOrder.DEFAULT },
  phases: {
    A: {
      start: true,
      next: "A",
      onBegin: (G, ctx) => {
        console.log(
          `Phase A start. Dice: ${G.dice}, player: ${ctx.currentPlayer}`
        );
      },
      endIf: (G, ctx) => G.dice === 0,
      onEnd: (G, ctx) => {
        console.log(
          `Phase A end. Dice: ${G.dice}, player: ${ctx.currentPlayer}`
        );
        G.dice = 3;
      }
    }
  },
  moves: {
    noop: (G, ctx) => {
      G.dice -= 1;
      console.log(`Player ${ctx.currentPlayer} moves. Dice: ${G.dice}`);
    }
  }
};

const client = Client({ game, numPlayers: 3 });

test("turn test", async () => {
  expect(client.getState().ctx.currentPlayer).toBe("0");
  client.moves.noop(); //then goes to player 1
  client.moves.noop(); //then goes to player 2
  client.moves.noop(); //**should** now go to player 3
  // phase ends
  expect(client.getState().ctx.currentPlayer).toBe("0");
});

With 3 players, 1 move per player, and the phase ends after 3 moves, at the start of the next phase the player should be "0". The console output is probably more helpful:

Player 2 moves. Dice: 0 
Phase A end. Dice: 0, player: 2 
Phase A start. Dice: 3, player: 2 
Player 2 moves. Dice: 2

The phase section of the docs:

Whenever a phase ends, the current player's turn is first ended automatically.

This was the behaviour pre 0.33 or 0.34. Also, I cannot seem to end this persons turn by using events.endTurn() either (during phase.onEnd), or with moveLimit specified or not. But ignore that for now.

[delucis]

I implemented this behaviour in #444. The default maintains the current player at the start of a new phase partly because before v0.33 phases didn’t end turns and they were effectively unrelated to turns (so an unchanged currentPlayer was consistent with previous behaviour).

If you want the current player to be incremented in a new phase, you can set a suitable first option in that phase’s turn configuration, for example:

phases: {
  phaseThatIncrementsCurrentPlayer: {
    turn: {
      order: {
        first: (G, ctx) => (ctx.playOrderPos + 1) % ctx.playOrder.length,
      },
    },
  },
}

Given that phases now automatically end turns, there may be an argument for currentPlayer being incremented in TurnOrder.DEFAULT, which is currently the following:

https://github.com/nicolodavis/boardgame.io/blob/ca64543a4966557f8da9bed586c390913c276024/src/core/turn-order.js#L316-L319

[nicolodavis]

Given that phases now end turns, I think we should rethink this default.

My intuition tells me that these are the most common turn orders in games:

1. Simple round robin (which also advances when the phase ends).
2. Round robin that always starts from 0.
3. Round robin that starts from a game specific "starting player".

[nearwood]

Ok, I think I may have gotten the heart of issue slightly incorrect, from what @delucis said. So now turns are ended but the currentPlayer is not incremented?

Prior to 0.33 my turns ended regardless of when the phase ended, which is the desired behavior.

Now the end of a phase also ends a turn. I think the confusion is that since I was testing in my local game with 2 players it was actually increment twice, and returning back to 0. When I am off work I can double-check.

I had assumed the docs had not been updated with this change.

In any case, my game is round-robin but the phases should be separate from whomever's turn it is. All moves end a turn (moveLimit: 1), and certain moves may end a phase, but the phase logic should not interrupt the turn logic.

[delucis]

Yes, now the endPhase event automatically ends the current turn first — allowing use of turn.onEnd — before ending the phase. The currentPlayer in the first turn of the new phase is derived from the turn.order.first function (as shown in my example above). Because TurnOrder.DEFAULT is what it is currently, using that explicitly (or implicitly by not setting a turn order), will not increment currentPlayer.

I agree with @nicolodavis that incrementing currentPlayer (i.e. default support for option 1 in that list) would probably be a reasonable default for the future although it is a breaking change for anyone using phases and TurnOrder.DEFAULT.

[nearwood]

Could the newer turn ending phase behavior be behind a flag? autoEndTurn: {bool} or something like that, defaulting to false to retain pre-0.33 behavior?

That way TurnOrder.DEFAULT won't need to be changed, and the prior behavior for phases also works, everyone's happy?

[nearwood]

If you want the current player to be incremented in a new phase, you can set a suitable first option in that phase’s turn configuration, for example:

phases: {
  phaseThatIncrementsCurrentPlayer: {
    turn: {
      order: {
        first: (G, ctx) => (ctx.playOrderPos + 1) % ctx.playOrder.length,
      },
    },
  },
}

This doesn't work quite right for me, I only have one phase that starts at the beginning of the game and repeats until game end. I need just the turn that was ended by the phase to be incremented to the next player. Overriding order.first causes off-by-one issues at the start of my game. I could code them out but, I think putting this behavior behind a phase prop is still the best option.

I thought I could do a simple workaround by just call ctx.events.endTurn({next: '[desiredPlayer]') in the phase's onEnd event, but it doesn't seem to do anything. :(

[nearwood]

Reading over the changes in #444 and the comments, I may still be off mark.

Perhaps another angle makes more sense: What if the moveLimit is reached at the same time the phase ends? What's the order of precedence?

Eg. By setting moveLimit: 1 I expect the current player to increment after each move regardless of whatever phase transition.

[delucis]

Oh right, my suggested fix doesn’t account for the very first phase.

This should work for you:

first: (G, ctx) =>
  ctx.turn === 0
    ? ctx.playOrderPos
    : (ctx.playOrderPos + 1) % ctx.playOrder.length

Events in hooks are also currently broken (#482) which is why endTurn in phase.onEnd isn't working, but it also seems a little hacky to have to do that in this case.

[nearwood]

Yes, that works but I was trying to say the new behavior (the phase ending a turn) should behind a feature flag, to avoid having to change TurnOrder.DEFAULT, etc. Or at least, updating the migration guide to be more explicit that this is new behavior.

RE: Events in hooks: Thanks, that explains it.

[delucis]

Right — the confusing thing is that the turns are currently ended, it’s just that then the first player in the new phase is the same as the last player in the previous phase. So technically two turns for the same player split by the phase change. Given how confusing and uncommon that behaviour probably is, it doesn’t seem like a good default!

[nearwood]

Dope, that's right. Forgot about that. Yeah it is confusing. I guess the issue is I was expecting the turn.order.next to be called at that last turn in a phase.