boardgameio / boardgame.io

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

PluginPlayer doesn't issue changes for current player #368

Closed MathieuLoutre closed 4 years ago

MathieuLoutre commented 5 years ago

I have this piece of code:

        onPhaseEnd: (G, ctx) => {
          Object.values(G.players).forEach((player) => {
            G.decks.small = [...G.decks.small, ...player.tempHand]
            player.tempHand = []
          })

          G.decks.small = ctx.random.Shuffle(G.decks.small)

          ctx.events.endTurn()
        },

Inside the next phase onPhaseBegin, I'm checking is tempHand is empty but it turns out that for the current player at the time it was reset, it's not.

Object.values(G.players).forEach((player) => console.log(player.tempHand.length))

This logs: 0 0 2 instead of 0 0 0 as expected. Inside onPhaseEnd, the current player (available on G.player) is still the last player (see https://github.com/nicolodavis/boardgame.io/issues/367#issuecomment-464843807) and accessing it through the players object doesn't seem to affect it.

However, changing the value in G.player directly (G.player.tempHand = []) works. It would be great if the players object could reflect the changes on the current player too.

Edit: This is actually the same everywhere I'm iterating over the players object to make changes to every player.

nicolodavis commented 5 years ago

The way PluginPlayer works right now, it just copies over G.players[currentPlayer] to G.player before the turn, and copies G.player back to G.players[currentPlayer] at the end of the turn. It's a weak attempt at trying to create an alias (which explains the problem that you're running into).

Open to other suggestions on how we can make this better.

MathieuLoutre commented 5 years ago

My solution so far is to remove the player reference and just use const player = G.players[ctx.currentPlayer] at the start of each action. It feels a bit like boilerplate but it’s not that bad. Creating some sort of getter that would preserve the reference might not even help that much? Maybe wrapping the move with a 3rd argument (G, ctx, player) when the plugin is active?

On 4 Mar 2019, at 13:38, Nicolo John Davis notifications@github.com wrote:

The way PluginPlayer works right now, it just copies over G.players[currentPlayer] to G.player before the turn, and copies G.player back to G.players[currentPlayer] at the end of the turn. It's a weak attempt at trying to create an alias (which explains the problem that you're running into).

Open to other suggestions on how we can make this better.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

nicolodavis commented 5 years ago

I think a getter / setter combination might work. We can convert player and opponent to getters / setters so that they retrieve the underlying data on demand.

MathieuLoutre commented 5 years ago

The new getter/setter approach feels right. I forgot you could access directly on the object which essentially just create the shortcut ref that we need in this case. Your addition to the test case looks like you've got it under control!

nicolodavis commented 4 years ago

This is now obsolete after PluginPlayer has changed to use the new Plugin API.