feat: client-authoritative character select #401
Conversation
fernando-cortez
added
the
1-Needs Review
| var isLockedInEvent = (changeEvent.Value.SeatState == CharSelectData.SeatState.LockedIn && !m_HasLocalPlayerLockedIn) || | ||
| (changeEvent.Value.SeatState == CharSelectData.SeatState.Active && m_HasLocalPlayerLockedIn); | ||
|
|
||
| if (!isLockedInEvent && changeEvent.Value.SeatIdx != -1) |
There was a problem hiding this comment.
magic number, should be a const somewhere (also other -1 should use that const)
| // has been reset to -1 (both server-authoritative) | ||
| if (changeEvent.Value.ClientId == NetworkManager.Singleton.LocalClientId) | ||
| { | ||
| var isLockedInEvent = (changeEvent.Value.SeatState == CharSelectData.SeatState.LockedIn && !m_HasLocalPlayerLockedIn) || |
There was a problem hiding this comment.
why do we use local values for locked in? With server authority, we should only take the server value as the authoritative value and never rely on local values
There was a problem hiding this comment.
So on OnPlayerClickedSeat(..), that's where the local player makes their anticipated selection, but also sends a ServerRpc for that change to be applied by the server to the NetworkList, which is then replicated to other clients.
Since the local player will still receive that same change event, I need to identify if it's just a local character selection change which can be ignored. What shouldn't be ignored is seat status changes.
NetworkListEvent doesn't provide that granularity on exactly what value was modified, so I'd need to compare it against something locally, no?
I could also be comparing it against the current seat from m_PlayerSeats, since that is what is actually modified before the ServerRpc is sent. But that's still also something stored locally.
There was a problem hiding this comment.
discussed IRL, should have previous value coming from SDK, need following up with Fatih
| { | ||
| var otherPlayerSharesSeat = false; | ||
|
|
||
| // if any other user shares seat, set them as owner; else deactivate seat |
There was a problem hiding this comment.
"owner"? I thought the lockedIn state was server driven?
There was a problem hiding this comment.
When a player chooses a new seat, I need to clear the local player's previous seat selection from the UI. But sometimes that seat is shared by another player.
Whether that previous seat now becomes empty, or "held" by another player is determined by the last update that was received by the server on the seats.
| { | ||
| // populate this seat with a shared player's lobby player state | ||
| otherPlayerSharesSeat = true; | ||
| m_PlayerSeats[m_LastSeatSelected].SetState(lobbyPlayer.SeatState, lobbyPlayer.PlayerNum, lobbyPlayer.PlayerName); |
There was a problem hiding this comment.
If we're to show this in doc, let's rename m_PlayerSeats. I had to go to definition to figure out this was UI elements only (since it has a "SetState" method, I thought this was a netvar of some sort)
There was a problem hiding this comment.
Might be worth to note too that we're doing a pass over all seats. This serves as a sort of "merge conflict resolution" where instead of just trying to apply deltas, we're resetting everything.
| } | ||
|
|
||
| // get local player, and populate the seat that is anticipated to be taken | ||
| TryGetLobbyPlayer(NetworkManager.Singleton.LocalClientId, out var localLobbyPlayerState); |
There was a problem hiding this comment.
might be worth to note this is the client drive part where we take the seat without waiting for the server driven value
|
State of PR: fix containing a Will place on hold until Netcode SDK version has been upgraded. |
fernando-cortez
added
4-On Hold
SamuelBellomo
added
2-Reviewed with Comments
|
Moving this to blocked. Awaiting possible refactors to NetworkVariable implementation. |
fernando-cortez
added
4-On Hold
|
So coming back to this, an owner netvar solution might still be weird. After discussions with Luke, we'd still need a server driven network list to manage locked in state, with an additional owner netvar for selection. Syncing between the two could be weird and buggy. Owner netvars can't be corrected by the server, so we'd effectively need to sync the network list and the owner netvar's state, with all the latency in between. |
|
Adding this for all PRs that have been opened since this new flow has been added. Please add your changelog here project changelog file and/or package changelog file |
1 similar comment
|
Adding this for all PRs that have been opened since this new flow has been added. Please add your changelog here project changelog file and/or package changelog file |
fernando-cortez
deleted the
feature/client-authoritative-character-select
branch
fernando-cortez
restored the
feature/client-authoritative-character-select
branch
Description (*)
This PR introduces a more reactive flow for the local player's character select screen.
The local player's character choice is shown instantaneously, instead of previously waiting for a confirmation event from the server. This clears the local player's previous choice and sets the new seat visuals right away.
The server will still override changes for the local player when they are locked in/out, or set to inactive due to another player confirming the same player, and of course any other player's selection.
The confirm button is server-authoritative. I figure a visual update for that when waiting for server confirm can be done as a follow up to this PR.
Related Pull Requests
Issue Number(s) (*)
Fixes issue(s): MTT-1438
Manual testing scenarios
Questions or comments
Contribution checklist