feat: Message Ordering #948
Conversation
…, still need to fix a couple of editor tests and check the manual tests.
…ing tests, more work required. # Conflicts: # com.unity.multiplayer.mlapi/Runtime/Configuration/NetworkConstants.cs # com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs # com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs # com.unity.multiplayer.mlapi/Runtime/Messaging/IInternalMessageHandler.cs # com.unity.multiplayer.mlapi/Runtime/Messaging/InternalMessageHandler.cs # com.unity.multiplayer.mlapi/Runtime/Messaging/MessageQueue/MessageQueueContainer.cs # com.unity.multiplayer.mlapi/Runtime/SceneManagement/NetworkSceneManager.cs # com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManager.cs # com.unity.multiplayer.mlapi/Tests/Editor/NetworkManagerMessageHandlerTests.cs # testproject/Assets/Tests/Runtime/RpcTestsAutomated.cs
- Internal commands sent from the server were getting routed to the server itself when the server was in host mode, which was incorrect and a change from previous behavior - Messages sent for Initialization and EarlyUpdate could not be processed due to order of execution between reading from the transport and processing the message queue. This was fixed for EarlyUpdate by changing the order, and for Initialization for declaring it to be an error to send a message targeted at Initialization.
|
|
…ization to earlyupdate
com.unity.multiplayer.mlapi/Tests/Runtime/Components/NetworkUpdateStagesComponent.cs
Show resolved
Hide resolved
com.unity.multiplayer.mlapi/Tests/Runtime/Transport/SIPTransport.cs
Outdated
Show resolved
Hide resolved
ShadauxCat
requested review from
0xFA11,
TwoTenPvP,
NoelStephensUnity,
mattwalsh-unity and
LukeStampfli
| @@ -169,19 +168,6 @@ public void HandleAddObject(ulong clientId, Stream stream) | |||
| var networkObject = NetworkManager.SpawnManager.CreateLocalNetworkObject(softSync, prefabHash, ownerClientId, parentNetworkId, pos, rot, isReparented); | |||
| networkObject.SetNetworkParenting(isReparented, latestParent); | |||
| NetworkManager.SpawnManager.SpawnNetworkObjectLocally(networkObject, networkId, softSync, isPlayerObject, ownerClientId, stream, hasPayload, payLoadLength, true, false); | |||
|
|
|||
There was a problem hiding this comment.
Yay!
This fixes the anoying race condition in #729 🎉
… channels - changed it to flush any time the channel changes.
| // Used to store targets, internally | ||
| private ulong[] m_TargetList = new ulong[0]; | ||
|
|
||
| // Used to mark longer lengths. Works because we can't have zero-sized messages | ||
| private const byte k_LongLenMarker = 0; | ||
|
|
||
| private void PushLength(int length, ref PooledNetworkWriter writer) | ||
| internal static void PushLength(int length, ref PooledNetworkWriter writer) |
There was a problem hiding this comment.
This looks oddly familiar to WriteIntPacked, is there a reason why we have a custom function for writing the length instead of using the default packed write?
There was a problem hiding this comment.
No idea! Maybe it's for efficiency since it knows the value will be either one or two bytes specifically?
There was a problem hiding this comment.
@TwoTenPvP can you remember?
com.unity.multiplayer.mlapi/Runtime/Messaging/CustomMessageManager.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| writer.WriteUInt64Packed(NetworkObjectId); // NetworkObjectId | ||
| writer.WriteUInt16Packed(NetworkBehaviourId); // NetworkBehaviourId | ||
| writer.WriteUInt32Packed(rpcMethodId); // NetworkRpcMethodId | ||
| writer.WriteByte((byte)serverRpcParams.Send.UpdateStage); // NetworkUpdateStage | ||
|
|
There was a problem hiding this comment.
Ok, so in the old code we wrote out the UpdateStage regardless, but now we only do so if we're not a host. Is this right?
There was a problem hiding this comment.
I think that is the "host loopback" and so it doesn't need to write it out because it is being directly inserted into the next frame's inbound local MessageQueue.
There was a problem hiding this comment.
UpdateStage is part of the unified header now. It's no longer part of the RPC message. The loopback code doesn't read the header for some reason... the fact that the behavior when pushing to the inbound queue is different than when pushing to the outbound is one of the things I'm addressing in my serialization RFC.
There was a problem hiding this comment.
To elaborate a bit further...
The message header (including UpdateStage) is only used on the receiving end during HandleIncomingData() in order to properly add the item to the correct queue. When sending to loopback, the header isn't written because HandleIncomingData() is bypassed, so we skip it when the only destination is the inbound queue because skipping it here emulates the behavior of HandleIncomingData in reading it (and thus pushing the read head past it).
There was a problem hiding this comment.
Ok, brilliant @ShadauxCat , can we hoist this last comment into a comment in the code? This is something subtle and worth placing here for future devs / users.
| } | ||
|
|
||
| if (addHeader) |
There was a problem hiding this comment.
Ultra nit, I would probably rename this / invert to 'skipHeader' and (even though it's pretty obvious when you look at the code) have a comment: "don't send a header for 1 client"
There was a problem hiding this comment.
I generally dislike using negative terms for variables and then inverting them. I find "if(do thing)" to be more intuitive and less prone to being misread than "if(not don't do thing)". But I won't die on that hill if you disagree.
There was a problem hiding this comment.
Ok, I can see that...but let's add the don't send a header for 1 client comment somewhere around addHeader
com.unity.multiplayer.mlapi/Runtime/Messaging/CustomMessageManager.cs
Outdated
Show resolved
Hide resolved
com.unity.multiplayer.mlapi/Runtime/Messaging/InternalMessageHandler.cs
Outdated
Show resolved
Hide resolved
| } | ||
| sendStream.NetworkChannel = item.NetworkChannel; | ||
| } | ||
| // If the item is a different channel we have to flush and change channels. |
There was a problem hiding this comment.
Couple of things:
- In the not-so-different future we want to go to "Channels as QoS"
- sometimes we think we're using different channels, but the transport is free to map to fewer or even just one channel. Is this a case where you'd want the transport to tell you if 2 channels aren't the same "physical" channel?
There was a problem hiding this comment.
Yes, you're right. What's the correct way to get that underlying "physical" channel information? Did a brief search and I'm not finding an obvious answer.
There was a problem hiding this comment.
Actually, I think this may need more thought...
While, yes, in logical terms, we want to batch by the physical channel because that's really where the batching matters, in practical terms, NetworkTransport.Send() expects the NetworkChannel as a parameter, not the NetworkDelivery type, and the receiving end also is able to read what channel it was sent on. So if we change the batching to work that way, then the receiving end will lose the ability to differentiate between channels with the same underlying delivery guarantees if that's ever relevant to them.
I think in order to make this change, we would need to make the NetworkChannel part of the message header for each message, and then change the transport API so that its operation isn't tied to channels, but only to the NetworkDelivery values. Until we either separate those two things, or decide to deprecate channels entirely and only provide delivery values (probably not going to happen), we can't batch messages of different channels together even if they have the same underlying NetworkDelivery value.
There was a problem hiding this comment.
Thing is, the design of MLAPI is such that we cannot actually know anything about what the transport is physically doing, and I'm wondering if this is a case that points to wanting to be able to query the transport for this. @andrews-unity this is that area of channel work I mentioned
There was a problem hiding this comment.
So I am all for re-working channels to being more of a QoS then what they are today because as @mattwalsh-unity stated all transports generally just map them to some sort of delivery that makes sense to that transport which is not consistent at all across transports.
With that said its not clear to me what you would actually query the transport for? would we pass in a channel and then have the Transport return back the delivery method it was going to use? Because if we move channels to QoS then its just going to be a 1:1 mapping of delivery method and that just feels like extra code.
I personally would love to get rid of all the MLAPI_CHANNEL stuff in the transport API re-design and move it somewhere else. I am very much liking the idea @ShadauxCat presented which is more of a soft deprecation which is we still have channel but they mean something only to the data consumer above but not to the actual transport, thus moving the channel concept to the header and only providing an API to send that pushes a delivery method vs an arbitrary byte that could mean anything to any transport.
| // clear the batch that was sent from the SendDict | ||
| sendStream.Buffer.SetLength(0); | ||
| sendStream.Buffer.Position = 0; | ||
| ProfilerStatManager.MessageBatchesSent.Record(); |
There was a problem hiding this comment.
@becksebenius-unity dunno if this 'channel context thing' is a big enough deal but would you want a metric for "interrupted batching from channel switches"?
com.unity.multiplayer.mlapi/Runtime/Messaging/MessageQueue/MessageQueueContainer.cs
Show resolved
Hide resolved
com.unity.multiplayer.mlapi/Runtime/Messaging/MessageQueue/MessageQueueContainer.cs
Outdated
Show resolved
Hide resolved
| { | ||
| networkManager.StopHost(); | ||
| } | ||
| else if (networkManager.IsServer) |
There was a problem hiding this comment.
Interesting...I would probably put this in its own PR to fix missing stuff in MultiInstanceHelper
# Conflicts: # com.unity.multiplayer.mlapi/Runtime/Configuration/NetworkConstants.cs # com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs # com.unity.multiplayer.mlapi/Runtime/Core/SnapshotSystem.cs # com.unity.multiplayer.mlapi/Runtime/Messaging/IInternalMessageHandler.cs # com.unity.multiplayer.mlapi/Runtime/Messaging/InternalMessageHandler.cs # com.unity.multiplayer.mlapi/Tests/Editor/NetworkManagerMessageHandlerTests.cs
- Moved call to `AdvanceFrameHistory` to the end of the frame.
…nt for the FixedUpdate stage to execute multiple times if the render framerate was lower than the physics framerate.
Fixes #700
Fixes #729
Fixes #790
Fixes MTT-620
Implements MTT-811
Implements MTT-812
Implements MTT-813