-
Notifications
You must be signed in to change notification settings - Fork 707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix “once” and rename stage
to activePlayers
#442
Fix “once” and rename stage
to activePlayers
#442
Conversation
If `ctx._stageOnce` is `true`, `flow.js` fetches keys from the `ctx.stage` object, but when it’s `null` (for example when ANY_ONCE has completed) there are no keys and a TypeError is thrown.
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 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? |
That makes sense to me. I’m not sure it’s safe to assume a reset to
setStage: {
others: 'discard',
once: true,
onEnd: (G, ctx) => {} // or onEmpty, onDone or something?
}
|
setStage: {
others: 'discard',
moveLimit: 2,
onEnd: (G, ctx) => {}
}
Edit Edit: Just realised hooks in |
OK, so I’ve pushed a fix that sets |
Adds a `stageCompleted` property, which is `null` when `once` is not active, `false` when it is, and `true` once all players have played during the stage.
I’ve now also added a Remaining questions:
|
`ctx.stage` ⇒ `ctx.activePlayers` `ctx._stageOnce` ⇒ `ctx._activePlayersOnce` `ctx.stageCompleted` ⇒ `ctx.activePlayersDone` `events.setStage` ⇒ `events.setActivePlayers` `turn.setStage` ⇒ `turn.activePlayers` `turn.stages` remains unchanged import { `Stage` ⇒ `ActivePlayers` } from 'boardgame.io/core' Internal: `SetStageEvent` ⇒ `SetActivePlayersEvent` `SetStage` ⇒ `SetActivePlayers`
Made the changes suggested in #441 Quick summaryGame 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, |
stage
to activePlayers
Great work @delucis! Found a minor bug that was not resetting |
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 |
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
|
Ok, I’ll have a go at the examples. Here are some thoughts on the above: Reset behavior of
|
* test: Add failing test for ANY_ONCE stage default If `ctx._stageOnce` is `true`, `flow.js` fetches keys from the `ctx.stage` object, but when it’s `null` (for example when ANY_ONCE has completed) there are no keys and a TypeError is thrown. * test: Add failing test for OTHERS_ONCE stage default * fix: Set _stageOnce to false once all players have played in stage * feat: Add `ctx.stageCompleted` property Adds a `stageCompleted` property, which is `null` when `once` is not active, `false` when it is, and `true` once all players have played during the stage. * refactor: Rename `stage` API to `activePlayers` `ctx.stage` ⇒ `ctx.activePlayers` `ctx._stageOnce` ⇒ `ctx._activePlayersOnce` `ctx.stageCompleted` ⇒ `ctx.activePlayersDone` `events.setStage` ⇒ `events.setActivePlayers` `turn.setStage` ⇒ `turn.activePlayers` `turn.stages` remains unchanged import { `Stage` ⇒ `ActivePlayers` } from 'boardgame.io/core' Internal: `SetStageEvent` ⇒ `SetActivePlayersEvent` `SetStage` ⇒ `SetActivePlayers` * refactor: `ANY` & `ANY_ONCE` ⇒ `ALL` & `ALL_ONCE` in `ActivePlayers` * fix: Fix comment to match rename * add test for actionPlayersDone * reset activePlayersDone at the start of each turn
ANY_ONCE
andOTHERS_ONCE
no longer end the phase as before. I needed anendIf
function to achieve that:First, a bug:
Once
stage
is empty, it is set tonull
. For example, withANY_ONCE
as moves are made: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 triggersprocessMove
, butprocessMove
currently tries to fetch keys from thectx.stage
object whenctx._stageOnce
istrue
, so when it encountersnull
aTypeError
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
andOTHERS_ONCE
turn order tests demonstrating this error.The
TypeError
could be avoided easily by checking thatstage
isn’tnull
before using it, but the real error is that a move is even possible at this point.There seem to be two options:
Instead of
null
, setstage
to a constant when all players have played. So, by defaultstage: null
, which indicates stage has not been set and move control stays with phase/turn, butstage: '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.(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, soonce
would always end the phase. Now, any of the following could be desirable once all players have played during the stage: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?