reset behavior of activePlayers

[nicolodavis]

Spawning off a thread from a previous discussion at #442, which is talking about (among other things) what to do to activePlayers after it becomes empty (this is a new concept in the phases-overhaul branch).

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

Players are removed from activePlayers once they make a move. After activePlayers becomes empty, it is set to null (which means that only the current player can play, but they are not in any stage).

However, if the currentPlayer was in a stage to begin with (i.e. activePlayers was something like { currentPlayer: 'play' } before setActivePlayers was called), then this state is lost.

Proposals

1. Reset activePlayers to the previous state before setActivePlayers was called once it becomes empty.

2. Pass in the state to reset to in the setActionPlayers call itself.

Notes

• Both proposals need to clearly define when to trigger the reset behavior. Is it sufficient to just assume that a reset is required when activePlayers becomes empty?

[nicolodavis]

Proposal 1 is implemented in 4a51096.

I'll keep this RFC open for a while in case people come up with better ideas that would warrant reverting the change and implementing something else.

[delucis]

I'll try out this implementation when I'm back at my computer, but I am still concerned about this approach as I wrote in #442:

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.

There would need to be an opportunity to idiomatically intervene on this reset behaviour to at least declare that you want to disable it or pass a function that can return activePlayers from state.

My instinct is still that a next option to setActivePlayers might be the better way to go. If you like, I could try my hand at an implementation so we would have concrete comparison points?

[nicolodavis]

Oh, it's not specific to once at all.

It just happens whenever activePlayers becomes empty, whether players are removed from there using once, or a new mechanism like moveLimit or by calling removeFromActivePlayers.

I also like the idea of having next to override this behavior (we can have both).

What do you think of the following semantics?

1. If next is not specified, then just revert back to the state before setActivePlayers was called. This works nicely for the Militia case and other examples where you just want to have everyone else do something, but you want to proceed with the original state after that.

2. If next is specified, then use that to reset the state. Let's disallow nesting here. So, you can specify all the options that go to setActivrPlayers here except next.

[delucis]

Oh, right, once removeFromActivePlayers etc. is available, it’s not only once that could end up emptying activePlayers. Makes sense.

Quick scenario though, that I think still makes resetting to the previous state (as a default) a challenge:

1. setActivePlayers({ currentPlayer: 'setup' })
   Player makes some setup moves etc. before ending that stage with a move that calls…

2. setActivePlayers({ currentPlayer: 'play' })
   Player plays a “Militia” card or similar, which calls…

3. setActivePlayers({ others: 'discard' })

4. Assuming the discard stage has a moveLimit, all other players discard and activePlayers becomes empty…

5. …and the library automatically resets to { currentPlayer: 'play' }. So far so good!

6. Player plays another move which calls removeFromActivePlayers(currentPlayer) 😬

7. activePlayers has become empty again. What should happen? Have we stored a history of stage states so we can now reset to { currentPlayer: 'setup' }? Or can we only reset one level deep?

This isn’t the best example, but I hope it demonstrates the problem. Maybe a better scenario would be if one of the players in the “discard” stage could play a card that causes all players to take an action before continuing to discard or something.

If resetting is a default, it should be consistent, which would mean storing a full history. You could argue that the above scenario would be programmer error — that they should know not to empty activePlayers in such a situation — but it seems counter intuitive to have a behaviour which only happens some of the time due to internal (library managed) state.

I guess this is to say that whether dealing with a default reset behaviour or a next parameter, nesting/history is kind of essential for ease of use. My personal preference would still be for a next API (and not both options), because the programmer is then in control of how much complex state ends up in ctx. A thorough implementation of resetting by default seems like the library would always have to store a history of activePlayers regardless of if that’s actually needed for the game.

With next the current reset behaviour might be achieved like this:

setActivePlayers({
  others: 'discard',
  next: {
    value: { ...ctx.activePlayers }
  },
})

[nicolodavis]

The next option cannot capture the following case:

Let's say you build a generic discard stage that is to be approached from many other scenarios in the game (this is common enough).

Each time you reach the discard stage, you want to revert back to where you came from. Having a hardcoded next will not allow for this sort of dynamic reset.

In fact, this is how phases work at the moment. When you call endPhase, it reverts back to the previous phase unless you override that with a next (details). We don't need this behavior in phases anymore because we now have stages, but we should still support some mechanism to revert back (either as the default behavior, or through an option like revert: true).

[delucis]

Hmmm, right. Ok, here’s my understanding of the challenges/goals:

1. Goals

   1. Where possible, prefer declarative syntax which will be easier for users to debug and anticipate all possible game states (although, some imperative API is necessary) — kind of the direction you were moving in in #447.

   2. Be consistent with current API. I think passing next to an endStage event could simplify some of the chaining/history issues discussed above. But…

2. Challenge

   We are in some senses dealing with two distinct ideas or uses of stages:

   1. A stage is a per-player state, so each stage can declare an individual next, moveLimit etc.

   2. A set of stages controls a broader game state, requiring some kind of co-ordination between per-player stages, for example taking an action once all players have played.

   For example, endStage helps with per-player flow, but revert: true (for example) is applicable to the whole set of stages, which is currently dealt with by setActivePlayers.

Is it worth exploring an extended activePlayers syntax that declares named configurations? That might clarify the API for the two distinct uses — you can control a flow of all-player configurations or control per-player flows from stage to stage (or jump between them).

// phases config
turn: {
  activePlayers: {
    setup: {
      start: true,
      currentPlayer: 'setup',
      next: 'main',
    },

    main: {
      currentPlayer: true,
    },

    militia: {
      others: 'discard',
      revert: true,
    },
  },

  stages: {
    setup: { moveLimit: 1, /* ... */ },
    discard: { moves: { /* ... */ } },
  }
}

// respects `next` or `revert` in active player configs, sets to `null` otherwise
events.endActivePlayers()

// but also possible with a passed argument
events.endActivePlayers({ next: 'militia' })

// shorthand set syntax
events.setActivePlayers('militia')

Not sure what the difference between set and end would be in the last two cases.

Considerations

• 👍 This syntax could also enable onBegin, onEnd, and endIf hooks for active player configurations

• 👎 It’s another (optional) layer of API complexity : phase → turn → [active player config →] stages

• 👍 Encourages as much declarative use as possible, potentially heavily simplifying calls to setActivePlayers in moves etc.

• ❓ Would it be practical to allow setActivePlayers to only use named configs? Manipulation would then be done with removeFromActivePlayers or with stage flows? Or would that be too restrictive?

• ❓ This doesn’t resolve the question of chained revert options. I would say ctx has to store a stack of states to unravel, but if it’s opt-in using revert that is only necessary when needed.

[nicolodavis]

Couldn't have articulated it better myself.

Yes, the key point is that with stages you're dealing with individual states (the stage that a particular player is in) and also a configuration (a set of stages). I actually did try to explore this direction (of using named configurations) when I was fleshing out the stages API, but decided against it because:

1. I couldn't find a name for it :) Sounds lame, but it can actually be a good guiding principle when designing an API. If you can't name a concept, it probably shouldn't exist as a concept because others will have trouble understanding it as well. Keeping the number of concepts down in an API is a very important consideration (like you've highlighted).

2. People can already name their configurations without any support from the API. For example, you can do:

const SETUP = {
  start: true,
  currentPlayer: 'setup',
  next: 'main',
};

const MAIN = {
  currentPlayer: true,
};

const MILITIA = {
  others: 'discard',
  revert: true,
};

...

setActivePlayers(MILITIA);

If supporting named configurations is merely going to force people to organize their code in a better way, I'd rather that we do that by publishing a style guide rather than adding to the API.

[nicolodavis]

This syntax could also enable onBegin, onEnd, and endIf hooks for active player configurations

This is an interested direction to explore as well. I think the main challenge would be communicating that these trigger for a player configuration, and not for individual players entering particular stages.

This doesn’t resolve the question of chained revert options. I would say ctx has to store a stack of states to unravel, but if it’s opt-in using revert that is only necessary when needed.

Yes, this is easy to do (and I'm for it). We should store a stack rather than just the previous value. It won't add any additional state unless you actually use it, so that's a good thing.

[delucis]

Ok, great — this clarified a lot of stuff for me. So the only advantage of actually declaring active player configurations would be supporting function fields like endIf etc. (not possible imperatively because functions can’t be stored in ctx)?

[nicolodavis]

Yes, that's right. I think let's revisit the named configuration thing after the branch is merged into master (and once we encounter a use case that's very painful to implement without things like endIf).

[nicolodavis]

So, it sounds like the things that we want to implement right now before merging the branch are the following. Just recording them for my own benefit so I know what to implement next. Feel free to contribute in any of these areas (just let me know so that we're not working on the same thing). Depending on how much help I get with these, maybe I'll start working on the docs first so things can happen in parallel.

• endStage: This just removes the player from activePlayers. Once activePlayers becomes empty, the reset logic is applied (just like it is applied if once: true is set). If next is present in the stage config, this just takes the player to that stage instead. You can also pass a next argument to endStage directly (similar to endPhase). However, I actually like the idea of splitting this into two (we should do it for endPhase as well then). setStage('B') and setPhase('B') are used instead of endStage({ next: 'B' }) / endPhase({ next: 'B' }).

• Make _prevActivePlayers a stack rather than just a single most recent value. The default reset behavior just pulls from this.

• Support next in setActivePlayers. This just overrides _prevActivePlayers.

• moveLimit in stage configs (or should we have it as an argument to setActivePlayers instead?). This will also deprecate once: true.

Have I missed anything?

[delucis]

This looks good to me. I’d add:

• Support revert in setActivePlayers (although that’s implicit in 2 I assume)

How would be best to support the current ALL_ONCE or OTHERS_ONCE active players config with these changes? Would users have to set an explicit stage and define moveLimit in the stage?

[delucis]

I’m working on the revert setting and converting _prevActivePlayers to a stack. One question that arises:

• What should we do with activePlayersDone?

Previously, once would end the active players and then be disabled, setting activePlayersDone to true, but now it might be possible to be in a sequence of revert stage sets, which would make activePlayersDone inaccurate if toggled to true in the middle. For now, I’ll only toggle it to true once the full _prevActivePlayers stack has been emptied, but it might be something to think about.

[nicolodavis]

Why don't we get rid of it entirely? People can check activePlayers directly:

turn: {
  endIf: (G, ctx) => ctx.activePlayers === null,
}

[nicolodavis]

Just to make sure we're on the same page when it comes to the semantics of revert:

If revert: true is passed to setActivePlayers:

1. When setActivePlayers is called, we push activePlayers to the stack.
2. When activePlayers becomes null, we restore activePlayers to the top value of the stack.

If revert is not passed (i.e. it is false by default):

1. When setActivePlayers is called, we empty the stack.
2. When activePlayers becomes null, it stays null because there is nothing on the stack to restore it to.

Does this match what you had in mind as well, or did you imagine it differently?

[delucis]

Does this match what you had in mind as well, or did you imagine it differently?

100% identical!

I’ll also remove activePlayersDone.

[delucis]

Why don't we get rid of it entirely? People can check activePlayers directly:

turn: {
  endIf: (G, ctx) => ctx.activePlayers === null,
}

It’s worth noting that that solution is not sufficient, because endIf is called during the beginning of a phase, before activePlayers has been set (see #442). I needed:

endIf: (G, ctx) => ctx.numMoves && ctx.activePlayers === null

[delucis]

I’ve been thinking about the next steps and here are the two discussion points I still think need tackling:

1. What’s the API for telling endStage or setStage which player’s stage to set? Some possibilties:

   // no arguments; default to playerID?
   endStage()
   // end the stage for a specific player
   endStage({ player: '1' })
   endStage('1')
   // set a stage for a specific player
   setStage({ next: 'discard', player: '2' })
   setStage('discard', '2')

   For comparison, the equivalent for phase is simpler because players don’t need specifying:

   endPhase()
   setPhase('main')

---

2. Where should moveLimit be? I wonder if we could continue the general API design where some properties can be overwritten by the same property for a more specific level of the game (e.g. a stage’s moves overrides a turn’s moves).

   const game = {
     turn: {
       activePlayers: {
         currentPlayer: 'play',
         others: 'discard',
         moveLimit: 1
       },
       stages: {
         play: {
           moves: {},
           moveLimit: 2
         },
         discard: {
           moves: {}
         }
       }
     }
   }

   This would allow setActivePlayers to set moveLimit (stored as ctx._moveLimit), but for individual stages to override that if necessary. In the example above, players in the discard stage would be limited to 1 move, while the player in the play stage would be allowed 2 moves.

   This seems the most flexible balance to me:

   • allow moveLimit to be declared via setActivePlayers — potentially permitting different limits for the same stage definitions

   • allow stage-specific moveLimit — which enables scenarios where different players should make different numbers of moves

   A third option would be to permit some way to set move limits analogous to setActivePlayers, e.g.

   setActivePlayers({
     all: 'play',
     moveLimit: {
       currentPlayer: 2,
       others: 1
     }
   })

   I certainly like the consistency of this with the rest of the activePlayers API (and it would more easily support scenarios where players have their numbers of moves limited by dice rolls or other game state). Any thoughts?

[nicolodavis]

What’s the API for telling endStage or setStage which player’s stage to set?

I think endStage and setStage should be just like their phase counterparts, with endStage taking no arguments and setStage taking one. Both work only on playerID (the player that made the move).

We can use setActivePlayers for everything else. Is there a use-case that won't be covered using this approach?

[nicolodavis]

re: moveLimit

Dynamic moveLimit (determined by a dice roll, for example)

This does suggest that we should support moveLimit in some form inside setActivePlayers. The third option that you suggest sounds like a good syntax to me.

Semantics

However, I think we should be quite careful about the overriding bit because moveLimit inside the turn config and inside the stage config mean different things.

turn: {
  moveLimit: 1
}

ends the turn after one move.

This can be overridden inside another phase just fine because they're referencing the same concept:

turn: {
  moveLimit: 1,
},
phases: {
  A: {
    turn: { moveLimit: 2 },   // you get an extra move inside Phase A
  }
}

Now, if we introduce:

turn: {
  moveLimit: 2,
  stages: {
    A: { moveLimit: 1 }   // does this end the turn after one move, or just the stage?
  }
}

the semantics become a bit confusing. I need to think about this a bit. I'm leaning toward just supporting this inside the setActivePlayers call and not having anything inside the stage config (because that might suggest some sort of override is happening).

[nicolodavis]

Also I just realized that there is probably no harm in merging the phases-overhaul branch with master as long as we keep the docs in a separate branch until the next NPM release goes live.

[nicolodavis]

So I'll probably do that once #450 is merged.

EDIT: phases-overhaul is now merged, so all future work can now happen on master.

[delucis]

1. I think it is possible someone might want to have a move that ends the stage for another player (like a “disable“ move or something), which endStage(target) would simplify, but I don’t know how common that’s likely to be. (In my experience, a move like that would usually disable that player’s next turn or something, rather than in a stages-style context where either player could move first.)

2. I forgot about turn.moveLimit. You’re right that we shouldn’t override the turn limits — the new API should end stages but not turns. I don’t think this necessarily rules out having a stage moveLimit, but it’s true it’s a potential point of confusion.

Ok, so the to do looks like:

• support moveLimit for activePlayers, allowing different limits for different players
• add setPhase, removing next option from endPhase
• add setStage and endStage, which operates on playerID

Later we could also add:

• moveLimit for stages, which would override moveLimit for activePlayers

[nicolodavis]

Yeah, let's go for the bare minimum since this is a new API and add stuff as we encounter use-cases that are difficult to implement.

I'm working on setPhase / endPhase BTW.

[nicolodavis]

Also, for moveLimit, I think we should support both:

// used to set different move limits for different players
setActivePlayers({
  all: 'play',
  moveLimit: {
    currentPlayer: 2,
    others: 1
  }
})

and a more concise version:

// used to set one move limit for all players
setActivePlayers({
  all: 'play',
  moveLimit: 2    // equivalent to moveLimit: { all: 2 }
})

The docs can introduce just the concise version initially and readers only need to learn about the more advanced version if they really need it.

[delucis]

I’m working on this and had the exact same thought. Should we even support moveLimit: { all: 2 } if it’s equivalent to moveLimit: 2?

[nicolodavis]

That's a good point. Maybe we should just support moveLimit: 2 instead of the more verbose version.

[delucis]

To support moveLimit we need to track numMoves for all players as previously discussed. I was about to make numMoves a map of player moves, but realised we actually need something separate from the current numMoves, because that tracks moves-per-turn, whereas we now need moves-per-stage.

Here’s my proposal:

• Add ctx._activePlayersNumMoves to track moves by currently active players
• Reset player counts whenever their stage changes and for everyone when the turn ends

Some implementations and their issues:

1. Reset all move counts whenever setActivePlayers is called and for a specific player when setStage/endStage is called.

   Issue: This is clean but it would mean that using setActivePlayers to change stage for one player, but maintain it for another would trigger a reset of all counts, which may not be the expected behaviour. Could be addressed by allowing set/end to target a player other than playerID.

2. As above, but we could diff the new stage against the previous stage and only reset the count if it changed

   Issue: What if a user wanted to reset the stage (including zeroing the move count) for an active player without changing the stage they are in? Could be addressed by adding a reset: true option, but that adds extra API complexity that doesn’t seem very helpful.

I’ll go with 1 for now, but let me know if you have any thoughts.