Fix “once” and rename stage to activePlayers

[delucis]

ANY_ONCE and OTHERS_ONCE no longer end the phase as before. I needed an endIf function to achieve that:

phase: {
  turn: {
    setStage: Stage.ANY_ONCE
  },
  endIf: (G, ctx) => ctx.numMoves && ctx.stage === null,
}

First, a bug:

Once stage is empty, it is set to null. For example, with ANY_ONCE as moves are made:

stage: { 0: '', 1: '' } // no moves made
stage: { 0: '' }        // player 1 moved
stage: null             // player 0 moved

Because the once setting doesn’t end the phase as before, the current player can still make a move (control of moves returns from the stage to the phase/turn). A move triggers processMove, but processMove currently tries to fetch keys from the ctx.stage object when ctx._stageOnce is true, so when it encounters null a TypeError is thrown.

https://github.com/nicolodavis/boardgame.io/blob/1f6b1a24dba9a03c13d9509b80b8aec0968bea51/src/core/flow.js#L682-L684

I’ve added a step to the ANY_ONCE and OTHERS_ONCE turn order tests demonstrating this error.

The TypeError could be avoided easily by checking that stage isn’t null before using it, but the real error is that a move is even possible at this point.

There seem to be two options:

1. Instead of null, set stage to a constant when all players have played. So, by default stage: null, which indicates stage has not been set and move control stays with phase/turn, but stage: 'STAGE_DONE' to indicate all players have played, and keeps move control with the stage.

   A user could then do endIf: (G, ctx) => ctx.stage === STAGE_DONE in a turn or phase as desired.

2. (I think I prefer this one.) Ensure the once setting actually ends a turn or phase or… progresses to a different stage setting… The problem I guess is that before these were all properties of phases, so once would always end the phase. Now, any of the following could be desirable once all players have played during the stage:

   • end the turn
   • end the phase
   • set the stage to a new configuration

Option 1 leaves the developer in charge of deciding what happens once everyone has played and just makes sure no more moves are possible at that point.

Option 2 is more opinionated and might also need some new API to allow a developer to say what they want once to end/set.

Any thoughts?

[nicolodavis]

I think a common pattern in games is to play a card that requires everyone else to do something (discard a card, for example). So the current player yields the turn until everyone else plays before the turn comes back to the current player again.

These sort of moves are typically implemented by doing something like:

move: (G, ctx) => {
  ctx.events.setStage(...);
}

In these cases I think it's nice to revert back to the state before setStage was called (i.e. whatever was in turn.setStage).

To override this default, we can provide options to end the turn or end the phase.

Additionally, we can also include an additional property that is set when the cycle is done.

What do you think?

[delucis]

That makes sense to me.

I’m not sure it’s safe to assume a reset to turn.setStage, because another move might have altered stage in between. For example, a turn might have an initial stage where the current player does some tasks, then a play stage where a “discard” move like you describe might be played. turn.setStage would reset to the initial stage, but after the “discard” move, you’d probably want to return to the play stage (or maybe to some other third state).

What about an API like:

setStage: {
  others: 'discard',
  once: true,
  onEnd: (G, ctx) => {}  // or onEmpty, onDone or something?
}

That would avoid adding an extra field to ctx, keep the current default behaviour, and maybe avoid adding API for ending turns/phases. The onEnd hook could be included in stage defaults too potentially.

[delucis]

The main thing I don’t like here is that onEnd is only relevant when once is true, which kind of makes once redundant, it could be inferred from onEnd’s presence, but then the hook would need a clearer name.

For greater consistency with the rest of the API, it might be nice to have a moveLimit option instead of once, so you could do:

setStage: {
  others: 'discard',
  moveLimit: 2,
  onEnd: (G, ctx) => {}
}

Edit: I realised once is consistent with turn orders, and that moveLimit would need stats for all players, which you already said wasn’t performant.

Edit Edit: Just realised hooks in turn.setStage are a bad idea because it should be compatible with events.setStage and then the onEnd function would have to be stored in ctx, which isn’t possible. Took me a while to get there, but I think your proposals will work best!

[delucis]

OK, so I’ve pushed a fix that sets _stageOnce to false once all players have played, which would allow the current player to keep playing and avoid the bug where a null ctx.stage object was being checked for keys. The tests I modified now pass.

[delucis]

I’ve now also added a ctx.stageCompleted property that is null when once is not
active, false when it is, and true once all stage players have played. I think for now this is probably sufficient, options to automatically end the phase or turn would be sugar.

Remaining questions:

• is there a better shorter name for stageCompleted?

• Right now, stageCompleted is initialised in SetStage and toggled to true in processMove. This means that if future turns don’t have a setStage property, stageCompleted will remain true rather than reverting to null. This isn’t a huge problem, but maybe it should be set to null during StartTurn for turns without a setStage property.

[delucis]

Made the changes suggested in #441

Quick summary

Game context now looks like:

ctx: {
  // ...
  activePlayers: { 0: '', 1: '' },
  activePlayersDone: null,
  _activePlayersOnce: false,
  // ...
}

---

Setting active players can be done via the game event:

events.setActivePlayers({ currentPlayer: '', once: true })

---

Turns can also configure active players:

turn: {
  activePlayers: { currentPlayer: 'A', others: 'B' },
  stages: { A: {}, B: {} }
}

or with a default configuration:

import { ActivePlayers } from 'boardgame.io/core'

const turn = {
  activePlayers: ActivePlayers.ALL // renamed from ANY
}

---

Internally, SetStage and SetStageEvent have also been renamed to SetActivePlayers and SetActivePlayersEvent.

[nicolodavis]

Great work @delucis!

Found a minor bug that was not resetting activePlayersDone properly and fixed it (in addition to adding a test).

[delucis]

Awesome! 🎉 Yeah, wasn’t sure where would be appropriate to reset it — this looks good.

Once this is merged, I can fix the examples and maybe documentation if needed? What else is left to do in phases-overhaul?

[nicolodavis]

Fixing the examples sounds good. I think the main thing to fix would be the Turn Order Simulator, which doesn't know anything about stages at the moment.

Other minor things that come to mind:

Reset behavior of activePlayers

I'm not sure what the consensus is on the default behavior of resetting activePlayers after activePlayersDone is set to true. The current behavior is that we set it back to null. Other options are:

1. Set it to the value in turn.activePlayers:
   Like you rightly pointed out, this may make sense at the beginning of the turn, but not so much after any moves have been made that modify activePlayers.

2. Set it to the previous value:
   Whenever setActivePlayers is called, we can just save ctx.activePlayers to ctx._prevActivePlayers and then restore it to that value after the "once cycle" is done.

3. Pass the next value in the setActivePlayers call:

setActivePlayers({ others: 'A', once: true, afterDone: { currentPlayer: 'B' } });

This is not particularly elegant, and also has some confusing semantics (for example, can you nest afterDone if you have a once: true inside it?)

moveLimit

Maybe we should support moveLimit inside a stage definition and get rid of the once property?

stages: {
  play: { ... },
  discard: { moveLimit: 1 },
}

ctx.events.setActivePlayers({ others: 'discard' });   // once is no longer needed

The disadvantage of putting it inside the stage definition is that you cannot use different values at different call sites (but I wonder if this matters; you can always create a second nearly identical stage with a different moveLimit).

Documentation

I would hold off on contributing to the documentation at the moment. I want to refactor the docs, so let's wait till I get the general outline there first.

Thanks for your help in getting this out!

[delucis]

Ok, I’ll have a go at the examples. Here are some thoughts on the above:

Reset behavior of activePlayers

I think the current behaviour is actually pretty good: set to turn.activePlayers at the start of each turn.

1. As I already mentioned, I don’t really like option 1 — resetting to turn.activePlayers during the turn. I feel like it might create more problems than it would solve and one would end up having to override this behaviour fairly frequently. If it were what someone wanted, this behaviour is already possible with onMove and ctx.activePlayersDone.

2. Option 2 could be nice, but I wonder if it implements too specific a behaviour: setting once to true would trigger this reset behaviour, but no other setting would and there’s no endActivePlayers event to do the equivalent manually. Also, because the behaviour relies on an older bit of state, it might make flow harder to immediately grasp. Like option 1, this behaviour is technically already possible.

3. Option 3 is interesting, but I am very wary of the nesting there and I think you would need to allow recursion for it to be useable 😬. It could definitely add some new fluidity to the API, but at what cost in terms of nested nextActivePlayers in ctx? (I do think this might be a nice API to use though.)

I think by far the simplest is to say that active player management during a turn is down to moves and hooks. The only problem I can think of is that in hooks there’s no way of knowing which specific activePlayersDone just completed, so for more complex flows, you’d definitely have to rely on stages and moves to manage it. I think that’s OK though? Otherwise, option 3 could be a solution.

moveLimit

I like the idea of having a moveLimit for each stage and of having limits that aren’t only 1. It would require having numMoves stats for all players in ctx again. Getting rid of once would also mean ALL_ONCE and OTHERS_ONCE could no longer be supplied as default objects. In other words, move limits would require using stages, which is not currently the case. I think the benefits of this would probably outweigh the slightly more verbose syntax.

Alternatively, it could be possible to also support an activePlayers.moveLimit to maintain something like once:

ALL_ONCE: { all: '', moveLimit: 1 }

Allowing activePlayers.moveLimit could be more expressive, because you could more easily do { others: 'discard', moveLimit: 2 } and then have a different limit without needing a different stage. But it’s definitely also good for stages to be able to declare their limits, so different players can have different limits and different moves can be locked down to specific limits more simply.

This would require deciding how to handle the case where both a stage and the activePlayers setting contain moveLimit. My instinct would be that stages take precedence, but that could produce bugs where someone has:

turn: {
  activePlayers: { all: 'A', moveLimit: 1 }
  stages: {
    A: { moveLimit: 2 }
  }
}

The actual move limit would be 2. Personally, that seems preferable to having a stage which declares a move limit that is sometimes ignored, but it could definitely be a source of bugs for users.

You previously mentioned wanting to keep multiplayer move statistics in a separate plugin. Could this be part of that? Could a plugin add the possibility to include moveLimit fields in stages and manage ctx accordingly onMove?

[nicolodavis]

Created two issues to continue the discussion on this:
#445 #446