Closed mgrau closed 5 years ago
But then G
and ctx
would come from different snapshots, which doesn't make sense. The last element of _undo
(i.e. _undo[length-1]
) is the current game state, so it makes sense to restore to _undo[length - 2]
.
I tried to reproduce your problem with some examples, but I am not able to (things seem to be working correctly, even when the phase changes).
Also note that we have a bunch of unit tests for the undo logic here. Can you demonstrate your problem via an additional test here?
I agree, it doesn't make sense to use G and ctx from different game states. I need to look around more to see where else ctx is changing.
I noticed that if I restore ctx to _undo[_undo.length - 1].ctx, the undo unit tests still pass. I'll focus on trying to replicate my problem with a test.
On Mon, May 6, 2019 at 3:55 PM Nicolo John Davis notifications@github.com wrote:
But then G and ctx would come from different snapshots, which doesn't make sense. The last element of _undo (i.e. _undo[length-1]) is the current game state, so it makes sense to restore to _undo[length - 2].
I tried to reproduce your problem with some examples, but I am not able to (things seem to be working correctly, even when the phase changes).
Also note that we have a bunch of unit tests for the undo logic here https://github.com/nicolodavis/boardgame.io/blob/master/src/core/reducer.test.js#L267. Can you demonstrate your problem via an additional test here?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nicolodavis/boardgame.io/issues/389#issuecomment-489629283, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKKG3F7EM37NIWQVCK3NITPUA2EZANCNFSM4HJ2W46A .
I think that's probably because the tests only verify the state in G
. Maybe you can add a test that also checks ctx
to see if all looks good (I'm happy to get that in regardless of whether a bug exists).
It looks like G
and ctx
are appended to _undo
in processMove()
, however, this is done before ctx
is updated by updateAndDetach()
, which means that the current ctx
is not reflected in _undo
. I don't think this is intentional. I've made a test that highlights this behavior.
I'm not sure the best place to fix this, but I've added
state._undo[state._undo.length - 1].ctx = state.ctx;
after ctx is updated by updateAndDetach()
.
Thanks for the fix, @mgrau!
When performing an undo operation, ctx takes the value of 2 moves prior, instead of 1 move prior. This is particularly problematic when a move changes the phase. I've found that using
seems to give the correct behavior.