removing players from activePlayers

[nicolodavis]

Spawning off a thread from a previous discussion at #442, which is talking about (among other things) the behavior of how players are removed from activePlayers.

Background

activePlayers is a map that keeps track of all the players that can make a move on the current turn. This is an evolution of the current concept actionPlayers. It also keeps track of what "stage" each active player is in. A "stage" is analogous to a "phase", and basically restricts what moves the player in that stage can make. It can be manipulated using something like this:

setActivePlayers({ others: 'discard', once: true });

which does the following things:

• put every player (other than the current player) in activePlayers.
• sets the stages of those players to discard.
• allows them to make one move before removing them from activePlayers.

This allows implementing cards that allow other players to take an action before reverting back to the current player (the Militia card in Dominion, for example).

Problem

once: true is too specific.

• What if the player must make 2 moves before they are removed?
• What if some other condition has to be satisfied before the player is removed?

Proposals

1. Add moveLimit to the stages config. Now each stage can declare how many moves ought to be made before a player is removed from activePlayers. This has the benefit that each stage can implement a different moveLimit. The downside is that it cannot be customized at the call-site. It also has the downside that we have to track the number of moves made by each player, which is additional state that can slow things down.

2. Create a removeFromActivePlayers event that can be called from a move. This can be used to remove players after a complex condition is met (rather than merely checking how many moves they have made).

[nicolodavis]

For Proposal 2, we can also use a declarative approach if we want to remove a player from activePlayers the moment they make a particular move.

moves: {
  discard: {
    impl: (G, ctx) => { ... },
    removeFromActivePlayers: true,
  }
}

This would be analogous to having a moveLimit: 1 that is applied only to this specific move.

The imperative way of doing this would be:

moves: {
  discard: (G, ctx) => {
    ...
    ctx.events.removeFromActivePlayers();
  }
}

We'll still need the imperative approach to support conditional removal, but the declarative approach is nicer for the simple case (so I'm proposing having both).

We already support the long-form move syntax, so it's just a matter of adding an additional boolean option to it.

[delucis]

I like both moveLimit and removeFromActivePlayers. (Although I’m not yet really familiar with the long-form move API.)

Do you know what profiling it was that suggested a multiplayer numMoves was a performance hit? I’ve been thinking about it and can’t figure out why that might be.

[nicolodavis]

The long-form move syntax just provides the moves as an object instead of a function:

// Short Form
moveA: (G, ctx) => { ... }

// Long Form
moveA: {
  impl: (G, ctx) => { ... }  // the move function

  // options that apply to this move
  optionA: true,
  optionB: true,
  ...
}

[nicolodavis]

I used https://clinicjs.org/flame/ to run a profile and produce a flame graph. I also subsequently added npm run benchmark that I run once in a while to check if we've introduced any performance regressions.

It's possible that my implementation was just naive (and caused some deep object nesting that needed to be copied many times). However, if we can implement a solution that doesn't require these counters (which would be the case if we just implement removeFromActivePlayers and not moveLimit), that's a good thing because then we're not adding state to ctx.

[delucis]

I feel like moveLimit is a common enough requirement to make it worth implementing. And there must be a way to do that that's performant. I'll take look and also see if I can figure out the benchmarking.

removeFromActivePlayers doesn't solve the same problems, but I do think it's a useful API to have (see #447).