Skip to content

fix: fixing MTT-1299 #1227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 28, 2021
Merged

fix: fixing MTT-1299 #1227

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
efc6e21
fix: fixing MTT-1299
andrews-unity Sep 27, 2021
191bd7c
fix: correct fix
andrews-unity Sep 28, 2021
006e26f
chore: cleanup using statements
andrews-unity Sep 28, 2021
00e42a1
fix: standards
andrews-unity Sep 28, 2021
3c5ed83
address PR Feedback
andrews-unity Sep 28, 2021
c32d87d
Merge branch 'develop' into fix/MTT-1299
andrews-unity Sep 28, 2021
b2ba9f4
standards fix
andrews-unity Sep 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 44 additions & 6 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,20 @@ internal void __sendServerRpc(FastBufferWriter writer, uint rpcMethodId, ServerR
RpcData = writer
};

var rpcMessageSize = 0;

// If we are a server/host then we just no op and send to ourself
if (IsHost || IsServer)
{
var tempBuffer = new FastBufferReader(writer, Allocator.Temp);
using var tempBuffer = new FastBufferReader(writer, Allocator.Temp);
message.Handle(tempBuffer, NetworkManager, NetworkBehaviourId);
tempBuffer.Dispose();

return;
rpcMessageSize = tempBuffer.Length;
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
{
rpcMessageSize = NetworkManager.SendMessage(message, networkDelivery, NetworkManager.ServerClientId, true);
}

var rpcMessageSize = NetworkManager.SendMessage(message, networkDelivery, NetworkManager.ServerClientId, true);

#if DEVELOPMENT_BUILD || UNITY_EDITOR
if (NetworkManager.__rpc_name_table.TryGetValue(rpcMethodId, out var rpcMethodName))
Expand Down Expand Up @@ -129,24 +132,59 @@ internal unsafe void __sendClientRpc(FastBufferWriter writer, uint rpcMethodId,
};
int messageSize;

// We check to see if we need to shortcut for the case where we are the host/server and we can send a clientRPC
// to ourself. Sadly we have to figure that out from the list of clientIds :(
ulong serverClientId = ulong.MaxValue;

if (rpcParams.Send.TargetClientIds != null)
{
// Copy into a localArray because SendMessage doesn't take IEnumerable, only IReadOnlyList
ulong* localArray = stackalloc ulong[rpcParams.Send.TargetClientIds.Count()];
var index = 0;
foreach (var clientId in rpcParams.Send.TargetClientIds)
{
if (clientId == NetworkManager.ServerClientId)
{
serverClientId = clientId;
continue;
}

localArray[index++] = clientId;
}

messageSize = NetworkManager.SendMessage(message, networkDelivery, localArray, index, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think serverCanSendToServerId param should be removed from SendMessage. It was added specifically and only for RPCs and is now no longer relevant.

}
else if (rpcParams.Send.TargetClientIdsNativeArray != null)
{
foreach (var clientId in rpcParams.Send.TargetClientIdsNativeArray)
{
if (clientId == NetworkManager.ServerClientId)
{
serverClientId = clientId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a break here?

}
}

messageSize = NetworkManager.SendMessage(message, networkDelivery, rpcParams.Send.TargetClientIdsNativeArray.Value);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we remove serverCanSendToServerId (making the behavior always the "false" behavior) then modifying the lists here won't be necessary.

}
else
{
messageSize = NetworkManager.SendMessage(message, networkDelivery, NetworkManager.ConnectedClientsIds, true);
foreach (var clientId in NetworkManager.ConnectedClientsIds)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: in this case NetworkManager.IsHost should be a quicker/easier way than iterating again

{
if (clientId == NetworkManager.ServerClientId)
{
serverClientId = clientId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a break here?

}
}

messageSize = NetworkManager.SendMessage(message, networkDelivery, NetworkManager.ConnectedClientsIds);
Copy link
Contributor

Choose a reason for hiding this comment

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

And then in this case the serverCanSendToServerId is default false which will remove it here...

}

// If we are a server/host then we just no op and send to ourself
if (serverClientId != ulong.MaxValue && (IsHost || IsServer))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

{
using var tempBuffer = new FastBufferReader(writer, Allocator.Temp);
message.Handle(tempBuffer, NetworkManager, serverClientId);
messageSize = tempBuffer.Length;
}

#if DEVELOPMENT_BUILD || UNITY_EDITOR
Expand Down
3 changes: 1 addition & 2 deletions testproject/Assets/Tests/Runtime/MessageOrdering.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public IEnumerator SpawnRpcDespawn()
int expectedCount = Support.SpawnRpcDespawn.ClientUpdateCount + 1;
const int maxFrames = 240;
var doubleCheckTime = Time.realtimeSinceStartup + 5.0f;
while (Support.SpawnRpcDespawn.ClientUpdateCount < expectedCount)
while (Support.SpawnRpcDespawn.ClientUpdateCount < expectedCount && !handler.WasSpawned)
{
if (Time.frameCount > maxFrames)
{
Expand All @@ -165,7 +165,6 @@ public IEnumerator SpawnRpcDespawn()

Assert.AreEqual(NetworkUpdateStage.EarlyUpdate, Support.SpawnRpcDespawn.StageExecutedByReceiver);
Assert.AreEqual(Support.SpawnRpcDespawn.ServerUpdateCount, Support.SpawnRpcDespawn.ClientUpdateCount);
Assert.True(handler.WasSpawned);
var lastFrameNumber = Time.frameCount + 1;
yield return new WaitUntil(() => Time.frameCount >= lastFrameNumber);
Assert.True(handler.WasDestroyed);
Expand Down