Skip to content

Commit

Permalink
allow switching playerID from Debug Panel
Browse files Browse the repository at this point in the history
  • Loading branch information
@nicolodavis
nicolodavis committed Oct 30, 2019
1 parent f63d51c commit 6c0a9b7
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 10 deletions.
1 change: 1 addition & 0 deletions src/client/client.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ class _ClientImpl {
updateCredentials(credentials) {
this.credentials = credentials;
this.createDispatchers();
this.notifySubscribers();
}
}

Expand Down
21 changes: 18 additions & 3 deletions src/client/debug/main/Main.svelte
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@
}
return r;
}
let playerID = client.playerID;
let ctx = {};
let G = {};
client.subscribe((state) => {
if (state) {
G = state.G;
ctx = state.ctx;
}
playerID = client.playerID;
});
</script>

<style>
Expand Down Expand Up @@ -53,7 +64,11 @@

<section>
<h3>Players</h3>
<PlayerInfo ctx={$client ? $client.ctx : {}} playerID={client.playerID} />
<PlayerInfo
on:change={(e) => client.updatePlayerID(e.detail.playerID)}
ctx={ctx}
playerID={playerID}
/>
</section>

<section>
Expand All @@ -76,12 +91,12 @@

<section>
<label>G</label>
<pre class="json">{JSON.stringify($client ? $client.G : {}, null, 2)}</pre>
<pre class="json">{JSON.stringify(G, null, 2)}</pre>
</section>

<section>
<label>ctx</label>
<pre class="json">
{JSON.stringify(SanitizeCtx($client ? $client.ctx : {}), null, 2)}
{JSON.stringify(SanitizeCtx(ctx), null, 2)}
</pre>
</section>
16 changes: 14 additions & 2 deletions src/client/debug/main/PlayerInfo.svelte
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,19 @@
export let playerID;
export let onClick = () => {};
const players = ctx ? [...Array(ctx.numPlayers).keys()] : [];
import { createEventDispatcher } from 'svelte';
const dispatch = createEventDispatcher();
function OnClick(player) {
if (player == playerID) {
dispatch("change", { playerID: null });
} else {
dispatch("change", { playerID: player });
}
}
let players;
$: players = ctx ? [...Array(ctx.numPlayers).keys()].map(i => i.toString()) : [];
</script>

<style>
Expand Down Expand Up @@ -40,7 +52,7 @@
class="player"
class:current={player == ctx.currentPlayer}
class:active={player == playerID}
on:click={() => onClick(playerID)}>
on:click={() => OnClick(player)}>
{player}
</div>
{/each}
Expand Down
4 changes: 2 additions & 2 deletions src/client/transport/local.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export class LocalTransport extends Transport {
this.gameID = this.gameName + ':' + id;
const action = ActionCreators.reset(null);
this.store.dispatch(action);
this.master.onSync(this.gameID, this.playerID, this.numPlayers);
this.connect();
}

/**
Expand All @@ -144,7 +144,7 @@ export class LocalTransport extends Transport {
this.playerID = id;
const action = ActionCreators.reset(null);
this.store.dispatch(action);
this.master.onSync(this.gameID, this.playerID, this.numPlayers);
this.connect();
}
}

Expand Down
10 changes: 7 additions & 3 deletions src/client/transport/local.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,20 +72,24 @@ describe('LocalMaster', () => {

describe('LocalTransport', () => {
describe('update gameID / playerID', () => {
const master = { onSync: jest.fn() };
const master = { connect: jest.fn(), onSync: jest.fn() };
const store = { dispatch: () => {} };
const m = new LocalTransport({ master, store });

beforeEach(() => {
jest.resetAllMocks();
});

test('gameID', () => {
m.updateGameID('test');
expect(m.gameID).toBe('default:test');
expect(master.onSync).lastCalledWith('default:test', null, 2);
expect(master.connect).toBeCalled();
});

test('playerID', () => {
m.updatePlayerID('player');
expect(m.playerID).toBe('player');
expect(master.onSync).lastCalledWith('default:test', 'player', 2);
expect(master.connect).toBeCalled();
});
});

Expand Down

13 comments on commit 6c0a9b7

@nicolodavis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes #499

@delucis
Copy link
Member

@delucis delucis commented on 6c0a9b7 Nov 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolodavis Just tried the master branch and it looks like this is still buggy. Have you had success using it?

  1. On clicking on a player, the orange box showing the player is selected only shows up/changes when you toggle to one of the other panels. Presumably the state change doesn’t make the component re-render.

  2. The value also doesn’t seem to be getting passed to the <WrappedBoard> component. If you look at the React dev tools at the bottom right, playerID stays null the whole time:

Debug panel bug

@nicolodavis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which of the following modes are you seeing the problem?

  1. Singleplayer
  2. Local multiplayer
  3. Remote multiplayer

@delucis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using the React Client like this:

Client({
  game,
  board,
  numPlayers: 3,
})

Which means Singleplayer, right?

@delucis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried it with the new multiplayer: Local() pattern. Using that the players toggle as you click on them (after a small delay for some reason), but the playerID prop passed to the board component still stays null.

@delucis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried digging to see if I could fix this. I don’t understand the transport system thoroughly enough to open a PR, but here’s what I found:

Previously, <WrappedBoard> passed updatePlayerID and similar methods to the <Debug> React component to be called directly.

Now, the transport implementations seem to be designed to support these update actions instead of having direct communication between the Svelte debug panel and the React <WrappedBoard>. However, <WrappedBoard> isn’t getting these updated values from the client. There is only a way of updating the client, which is called when props change.

The simplest fix seemed to be passing in the client’s values instead of the <WrappedBoard>’s own values to the user’s board component:

-          gameID: this.gameID,
+          gameID: this.client.gameID,
-          playerID: this.playerID,
+          playerID: this.client.playerID,

(at src/client/react.js#L155-L156)

Should that be sufficient? This seemed to work, but I was running up against some bugs with my board component that I hadn’t seen before.

Also, this is all with a client using multiplayer: Local(). When I don’t pass a multiplayer option to the React client, clicking on the players has no effect (as shown in the GIF above).

@nicolodavis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had time to look at this yet (traveling this week), but my sense is that we're just not calling notifySubscribers() when the playerID is updated.

@delucis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s true (source), but notifySubscribers() just calls the subscriber’s callback with the return value of getState() (source). playerID, gameID etc. aren’t included in state and the React client doesn’t ever read them from the client, so the current notifySubscribers() implementation won’t update them.

@nicolodavis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that be sufficient? This seemed to work, but I was running up against some bugs with my board component that I hadn’t seen before.

What errors were you running into? The fix that you suggested is what I had in mind as well and I don't see a reason why it should introduce new errors in the board component.

@nicolodavis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've committed a fix at 457b29d. Can you let me know if that resolves the issue?

@delucis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! One thing I’ve noticed is that the entire board component seems to be re-rendering in a way it didn’t previously (I see some component state that seems to be re-triggered on toggling player), but for testing it works fine. Thanks!

@nicolodavis
Copy link
Member Author

@nicolodavis nicolodavis commented on 6c0a9b7 Nov 22, 2019 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@delucis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if I check the Profiler it looks like the entire <WrappedBoard> is being re-rendered. There are a few sub-components that should change because they show player-specific information, but the main board component shouldn’t change.

I think the clearest visual thing is that there are some tokens that have a reveal animation managed by their component state and this gets retriggered, so I guess that state is being wiped. Here’s the effect each time I change player:

token reveal animation

Please sign in to comment.