boardgameio / boardgame.io

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

Caller needs to know whether move returned as `INVALID_MOVE` #592

Open vdfdev opened 4 years ago

vdfdev commented 4 years ago

It would be useful to also know the reason of the invalid move, so allow for some extra data specific to the game.

delucis commented 4 years ago

To continue the thread from Gitter. Couple of options/considerations:

Move syntax

Moves could throw an error

Code examples ```js // define custom error class class InvalidMoveError extends Error { constructor(data) { super('INVALID_MOVE'); this.name = "InvalidMoveError"; this.data = data; } } ``` ```js // move definition const { InvalidMoveError } = require('boardgame.io/core'); function move() { // move is invalid, throw an error throw new InvalidMoveError({ message: 'something went wrong', player: '2' }); } ``` ```js // process move try { moveFn(G, ctx, ...args); } catch (error) { if (!(error instanceof InvalidMoveError)) throw error; // return error.data to client } ```

Notes

Moves could return an error object

Code examples ```js // define error creator function InvalidMove(data) { return { error: 'INVALID_MOVE', data }; // for example } ``` ```js const { InvalidMove } = require('boardgame.io/core'); function move() { // move is invalid, return an error object return InvalidMove({ message: 'something went wrong', player: '2' }); } ``` ```js // process move const result = moveFn(G, ctx, ...args); if (result && result.error === 'INVALID_MOVE') { // move is invalid, return result.data the client } ```

Notes

ctx could have an error method

On the surface this seems pretty similar to an InvalidMove function to me, just that the function is attached to ctx instead of exported in core.

The major advantage of this would presumably be that it could hook into ctx in deeper ways — perhaps firing other kinds of changes to state or flow. What would those be?

Client handling

How does the client receive the invalid move response? Currently when a move returns INVALID_MOVE the reducer logs an error to the console and unchanged state is returned:

Reducer code https://github.com/boardgameio/boardgame.io/blob/167690c5db2b2bfa09e432439a7948c50049b77a/src/core/reducer.ts#L155-L164 It’s worth noting that most of the `error` logging calls in the reducer, could potentially also benefit from reporting to the client what went wrong. ---

I feel like the error should not be returned within or alongside state, because it’s really a response to the imperative API that moves provide, and the error is client-specific, not a global event, but then I’m not sure how the client should get the error object.

It’s actually a broader architectural problem: the client moves are synchronous functions you fire off and hope all goes well, but under the hood these can trigger asynchronous or error-producing code. There’s no way of knowing directly when or if they’ve completed or if they were successful; you just observe the state changes the move may cause. This API is pretty nice for most simple use cases, but it has its drawbacks. It’s also a problem with multiplayer scenarios, where we don’t know if a server call timed out etc. (e.g. #218). Are there standard patterns for reducer errors or errors in general when using a Flux approach?

nicolodavis commented 4 years ago

One way to work around the asynchronous issue is to return a Promise from the move dispatcher. We just need to ensure that the client can still be called in a synchronous way when using an in-memory store.

nicolodavis commented 4 years ago

Of the approaches that we've discussed so far, I think I'm leaning toward moves returning an error object.

jeetmehta commented 3 years ago

👋 Hi there! My friends and I are using boardgame.io and the framework has been great. I'm currently trying to indicate to the user/player why their move might be invalid.

Looks like this is an active stream of work? Is there an alternate recommendation or workaround that we can take in the meantime?

I've tried setting things within G (i.e. G.invalidMove and G.invalidMoveErrorMessage) but returning INVALID_MOVE just resets G to the prior state, so the client doesn't see those props change.

Would love any advice 🙏 cc: @delucis @nicolodavis @shaoster

shaoster commented 3 years ago

👋 Hi there! My friends and I are using boardgame.io and the framework has been great. I'm currently trying to indicate to the user/player why their move might be invalid.

Looks like this is an active stream of work? Is there an alternate recommendation or workaround that we can take in the meantime?

I've tried setting things within G (i.e. G.invalidMove and G.invalidMoveErrorMessage) but returning INVALID_MOVE just resets G to the prior state, so the client doesn't see those props change.

Would love any advice 🙏 cc: @delucis @nicolodavis @shaoster

Hello! I'm currently working with @delucis to add a feature to help with this exact use case.

In the meantime, see https://github.com/boardgameio/boardgame.io/issues/292#issuecomment-431528906 for a workaround.

The prescriptive TL;DR there is to create a (hopefully pure) shared function (i.e. isValidOrWhyNot) that's called by both your UI and your move implementation.

For example: https://github.com/shaoster/unmuted2021/blob/3b4cb94f0828f84d146ff165b5ce211bedfd8039/src/Action.js#L63

jeetmehta commented 3 years ago

Hmm... the tricky part here is that we're trying to validate the turn as opposed to the individual move. A single turn can have many moves. The turn is ended on a button press. Apologies, I should've been clearer in my original comment. 🙏

Sometimes you may need to go deep into the move (pathfinding, for example) before you realize that there isn't a valid outcome. In these cases, it might also be expensive to both validate and run the move every time in the cases where the move is actually valid.

This comment stands out to me since it's relevant to our game. We're building something similar to Scrabble, so there's a lot of word validation / traversal that happens (doing the validation requires changing or manipulating G) before we can say that "the tiles you played this turn are invalid for x, y , z reasons". And if the turn is invalid, we need the state to go back to it's prior version before the most recent move - which is what using INVALID_MOVE gives us.

Wondering if that changes or affects your recommendation?

jeetmehta commented 3 years ago

Scratch that - your recommendation worked! I just created a separate validateTurn function which is called before the turn is ended.

Thank you!! 🙏

shaoster commented 3 years ago

Yeah, in general you may want your validate function to additionally return some intermediate artifact (maybe a pruned search tree or list of valid moves) if the computation to get there is nontrivial.