Skip to content
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

Can't force endTurn on early moves, with moveLimit > 1 #934

Closed
samuelhwilliams opened this issue Apr 28, 2021 · 6 comments · Fixed by #985
Closed

Can't force endTurn on early moves, with moveLimit > 1 #934

samuelhwilliams opened this issue Apr 28, 2021 · 6 comments · Fixed by #985

Comments

@samuelhwilliams
Copy link

To reproduce, create a game with this in its declaration:

turn: 
{
  moveLimit: 2,
},

And then in a move, fire the endTurn event with the force argument:

ctx.events.endTurn( { force: true } );

If you take this move as your first action, it just prints a debug message saying eg INFO: cannot end turn before making 2 moves. I'm not sure if force isn't properly propagating through the event, as in flows.ts it does seem to have logic to allow forcing the turn to end early.

For context, I'm trying to build a game where players have two actions by default, but one of those actions should immediately end the turn if taken first.

@delucis
Copy link
Member

delucis commented Apr 28, 2021

Hi! The force parameter isn’t actually exposed via the public endTurn event. The argument passed to ctx.events.endTurn is arg in the signature, with force being set internally to permit forcing the turn end when the phase ends:

function EndTurn(
state: State,
{ arg, next, turn, force, automatic, playerID }: any
): State {

I’ve sometimes wondered whether moveLimit should be treated as just an upper limit rather than also creating a minimum number of moves, but that isn’t currently the case. (Perhaps even a minMoves/maxMoves API would be more flexible.)

For your use case, I would currently suggest not setting moveLimit and instead doing something like.

  • action 1 that always ends the turn: ctx.events.endTurn()
  • action 2 that ends the turn when taken second: if (ctx.numMoves > 0) ctx.events.endTurn()

@samuelhwilliams
Copy link
Author

Noted, thanks 👍

I haven't found anything that clearly defines what is exposed in ctx in one place - where's the best place to look that up? I ended up rolling my own action points tracker because I didn't read anything about numMoves. Good to know it exists though - thanks.

@delucis
Copy link
Member

delucis commented Apr 29, 2021

I don’t think ctx is documented in detail anywhere.

The most detailed source would probably be the Typescript interface although this doesn’t provide additional context or explanations.

export interface Ctx {
numPlayers: number;
playOrder: Array<PlayerID>;
playOrderPos: number;
playerID?: PlayerID;
activePlayers: null | ActivePlayers;
currentPlayer: PlayerID;
numMoves?: number;
gameover?: any;
turn: number;
phase: string;
_activePlayersMoveLimit?: Record<PlayerID, number>;
_activePlayersNumMoves?: Record<PlayerID, number>;
_prevActivePlayers?: Array<{
activePlayers: null | ActivePlayers;
_activePlayersMoveLimit?: Record<PlayerID, number>;
_activePlayersNumMoves?: Record<PlayerID, number>;
}>;
_nextActivePlayers?: ActivePlayersArg;
_random?: {
seed: string | number;
};
// TODO public api should have these as non-optional
// internally there are two contexts, one is a serialized POJO and another
// "enhanced" context that has plugin api methods attached
events?: EventsAPI;
log?: LogAPI;
random?: RandomAPI;
}

There’s a loose convention that fields prefixed with _ are “private” as they are mostly metadata used internally.

For a better insight into what’s in ctx I’d recommend taking a look at the debug panel. This will show you all the “public” ctx fields and you can see how they change as you prototype. (For an example, check out this Tic-Tac-Toe demo although because it has moveLimit: 1, numMoves always displays as 0 in this case.)

@samuelhwilliams
Copy link
Author

samuelhwilliams commented Apr 29, 2021

Very helpful, thank you 👍

I’ve sometimes wondered whether moveLimit should be treated as just an upper limit rather than also creating a minimum number of moves, but that isn’t currently the case. (Perhaps even a minMoves/maxMoves API would be more flexible.)

I would support either of these solutions - the latter does seem more flexible. I'm not particularly strong on the JS/TS front, otherwise I'd immediately volunteer to do it, but perhaps I'll investigate it once I'm a bit more comfortable with the framework.

@delucis
Copy link
Member

delucis commented Jun 13, 2021

A note here to say that in the short-term, we should stop blocking endTurn when a moveLimit is active. moveLimit would then literally be a limit on number of moves. If a user needed to enforce a minimum number of moves they would have to disable the endTurn client event.

Later we could explore renaming moveLimit to maxMoves and introduce a minMoves option. { minMoves: 2, maxMoves: 2 } would then be equivalent to the current behaviour.

@senritsu
Copy link
Contributor

As this was tagged "good first issue" I used this as an opportunity to get to know the codebase.

The actual fix for the issue would have been really small (removing just one check and updating test), so I went a step further and tried adding the proposed minMoves option as well. moveLimit kept its name though, to prevent adding a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants