fix: fixing MTT-1299 #1227
fix: fixing MTT-1299 #1227
Conversation
andrews-unity
commented
Sep 27, 2021
andrews-unity
commented
- This code breaks the Unity Transport Adapter and assumes specific behavior that wasn't present before, in UTA we assume that these kind of checks are not happening in the SDK. This is because its very possible that you're clientID on the server (which is synchronized via OwnerClientId) and your ServerId could indeed be the same and it shouldn't really matter if they are as one side should not impact the other side. So for this reason we have removed this check.
* This code breaks the Unity Transport Adapter and assumes specific behavior that wasn't present before, in UTA we assume that these kind of checks are not happening in the SDK. This is because its very possible that you're clientID on the server (which is synchronized via OwnerClientId) and your ServerId could indeed be the same and it shouldn't really matter if they are as one side should not impact the other side. So for this reason we have removed this check.
| tempBuffer.Dispose(); | ||
|
|
||
| return; | ||
| rpcMessageSize = tempBuffer.Length; |
There was a problem hiding this comment.
@becksebenius-unity don't you guys intentionally 0 out "loopback" sends elsewhere in the framework? I could have sworn I saw something like that in some networkvariable update code recently.
There was a problem hiding this comment.
We do. We're currently reconsidering this though - the question of how we represent loopback has been right in the middle of the "what information scope are showing" conversation as we heard some feedback during the hackweek that our current approach is still confusing. More info here - https://unity.slack.com/archives/C01D91UDGKH/p1632845411151700
| else | ||
| { | ||
| messageSize = NetworkManager.SendMessage(message, networkDelivery, NetworkManager.ConnectedClientsIds, true); | ||
| foreach (var clientId in NetworkManager.ConnectedClientsIds) |
There was a problem hiding this comment.
nit: in this case NetworkManager.IsHost should be a quicker/easier way than iterating again
| } | ||
| } | ||
|
|
||
| messageSize = NetworkManager.SendMessage(message, networkDelivery, rpcParams.Send.TargetClientIdsNativeArray.Value); |
There was a problem hiding this comment.
In the TargetClientIds loop above you remove the serverclientid from the list before passing to SendMessage but here you pass it along. I can't say that's wrong for sure, but it doesn't feel right... If it's intentional, adding a comment to the code would go a long way
There was a problem hiding this comment.
If we remove serverCanSendToServerId (making the behavior always the "false" behavior) then modifying the lists here won't be necessary.
| localArray[index++] = clientId; | ||
| } | ||
|
|
||
| messageSize = NetworkManager.SendMessage(message, networkDelivery, localArray, index, true); |
There was a problem hiding this comment.
I see the serverCanSendToServerId is true here which prevents a second walk of the list, but on first read it's counter-intuitive to remove the server from the list and then say serverCanSendToServerId = true here. Maybe a comment?
There was a problem hiding this comment.
I think serverCanSendToServerId param should be removed from SendMessage. It was added specifically and only for RPCs and is now no longer relevant.
| } | ||
| } | ||
|
|
||
| messageSize = NetworkManager.SendMessage(message, networkDelivery, NetworkManager.ConnectedClientsIds); |
There was a problem hiding this comment.
And then in this case the serverCanSendToServerId is default false which will remove it here...
| localArray[index++] = clientId; | ||
| } | ||
|
|
||
| messageSize = NetworkManager.SendMessage(message, networkDelivery, localArray, index, true); |
There was a problem hiding this comment.
I think serverCanSendToServerId param should be removed from SendMessage. It was added specifically and only for RPCs and is now no longer relevant.
| } | ||
| } | ||
|
|
||
| messageSize = NetworkManager.SendMessage(message, networkDelivery, rpcParams.Send.TargetClientIdsNativeArray.Value); |
There was a problem hiding this comment.
If we remove serverCanSendToServerId (making the behavior always the "false" behavior) then modifying the lists here won't be necessary.
| { | ||
| if (clientId == NetworkManager.ServerClientId) | ||
| { | ||
| serverClientId = clientId; |
| { | ||
| if (clientId == NetworkManager.ServerClientId) | ||
| { | ||
| serverClientId = clientId; |
| } | ||
|
|
||
| // If we are a server/host then we just no op and send to ourself | ||
| if (serverClientId != ulong.MaxValue && (IsHost || IsServer)) |
There was a problem hiding this comment.
Only question is... we're iterating through all these arrays looking for clientId == NetworkManager.ServerClientId to set serverClientId... but the value of serverClientId will always be NetworkManager.ServerClientId or ulong.MaxValue. Why not just use a boolean hasServer and then as Jesse said, when there's no specific target IDs, just hasServer = IsHost?
Nope and Nope |
* fix: fixing MTT-1299
5 participants
