boardgameio / boardgame.io

State Management and Multiplayer Networking for Turn-Based Games
https://boardgame.io
MIT License
9.93k stars 707 forks source link

Events may be called twice when using `movesPerTurn` #242

Closed Stefan-Hanke closed 4 years ago

Stefan-Hanke commented 5 years ago

This is a follow-up issue from #241.

When a flow uses the movesPerTurn property (e.g. in the TicTacToe example), the library will call events automatically, even when a move also called into events.

Events should be called exactly once as they happen, regardless of whether a game calls into events or not.

nicolodavis commented 5 years ago

Should we just consider this user error, i.e. if you use movesPerTurn, your game logic is flawed if you also call endTurn from your move?

What about:

move: moveName(G, ctx) {
  ctx.events.endTurn();
  ctx.events.endTurn();
}

This will also result in multiple endTurn calls being emitted. Should we try to second guess this as well?

The reasons I'm OK with writing this off as user error without writing more code to handle it:

  1. The turn can only be ended exactly once. The following snippet logs "turn end" only once despite the fact that multiple end turn events are emitted.
move: moveName(G, ctx) {
  ctx.events.endTurn();
  ctx.events.endTurn();
},

flow: {
  onTurnEnd(G) {
    console.log('turn end');
    return G;
  }
}
  1. You can only end your own turn. There isn't a danger of a stray endTurn affecting another player's turn. Reference: https://github.com/google/boardgame.io/commit/c4a11a79292700cf9d6afde737cabd0089941b8d
Stefan-Hanke commented 5 years ago

Sounds reasonable.

I don't know whether this is documented, but there should be a paragraph about using movesPerTurn in a flow.

What I'm unhappy with is that such errors behaviours get silently ignored (e.g. it's not allowed to call that event? The framework will just ignore it...). The game should work given all those checks but I'd like it to be a bit more perceptible.

nicolodavis commented 5 years ago

Makes sense. I definitely think we should do more error logging (perhaps using a custom log function that is automatically stripped out in production builds). That way we can alert the developer to mistakes like this (and other things like forgetting to return G from a move).

I'll add an issue for this: #253