feat: snapshot. MTT-1088 Snapshot acknowledgment gaps #1083
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hot-system-spawn3
…hot-system-spawn3
…Technologies/com.unity.multiplayer.mlapi into experimental/snapshot-prep2-spawn
…apshot-system-spawn3
…apshot-system-spawn3
…hot-system-spawn3
…hot-system-spawn3
…hot-system-spawn3
…hot-system-spawn3
…hot-system-spawn3
…function. This will allow reusing it from NetworkShow. But as-is, no impact
…<NetworkObject>) instead of calling a deeper-nested function. Make the code more uniform and prepares for incoming snapshot changes
…n' into experimental/snapshot-system-spawn3
…ide(List<NetworkObject>). Same reasons as for NetworkShow
…n' into experimental/snapshot-system-spawn3
…hot-mtt1092-mtt1056
…t1056' into experimental/snapshot-system-spawn3
…hot-system-spawn3
…hot-system-spawn3
…hot-system-spawn3
…just the latest
…hot-system-spawn3
jeffreyrainy
requested review from
LukeStampfli,
mattwalsh-unity and
SamuelBellomo
LukeStampfli
approved these changes
Aug 24, 2021
Darn good point. The reason it's not there is the dependency injection problem. SnapshotSystem depends on the NetworkManager and the timing code. To test this fully, we'd need a way to either:
Then the test could instrument/mock the scenarios where specific messages are ack'ed. It's a bit out-of-scope though. I did 1. manually, locally. I'll add a task for it and commit this without, if that works for you? |
|
| nonNullContext.NetworkWriter.WriteInt32Packed(m_CurrentTick); | ||
| nonNullContext.NetworkWriter.WriteUInt16(sequence); | ||
|
|
||
| var buffer = (NetworkBuffer)nonNullContext.NetworkWriter.GetStream(); | ||
|
|
||
| using (var writer = PooledNetworkWriter.Get(buffer)) | ||
| { | ||
| // write the snapshot: buffer, index, spawns, despawns |
There was a problem hiding this comment.
Have to remember to port this over to @ShadauxCat's new scheme
| // since each bit in ReceivedSequenceMask is relative to the last received sequence | ||
| // we need to shift all the bits by the difference in sequence | ||
| m_ClientData[clientId].ReceivedSequenceMask <<= | ||
| (sequence - m_ClientData[clientId].LastReceivedSequence); |
There was a problem hiding this comment.
Do we need to to bounds checking on sequence - m_ClientData[clientId].LastReceivedSequence to guard against it being negative or overflowing the 16 bit value?
| // because the bit we're adding for the previous ReceivedSequenceMask | ||
| // was implicit, it needs to be shift by one less | ||
| m_ClientData[clientId].ReceivedSequenceMask += | ||
| (ushort)(1 << (ushort)((sequence - 1) - m_ClientData[clientId].LastReceivedSequence)); |
There was a problem hiding this comment.
Same check here on under / overflow?
SamuelBellomo
added a commit
that referenced
this pull request
Aug 26, 2021
…hub.com:Unity-Technologies/com.unity.multiplayer.mlapi into sam/feature/interpolation-for-network-transform * 'sam/feature/interpolation-for-network-transform' of github.com:Unity-Technologies/com.unity.multiplayer.mlapi: test: NetworkTransformStateTests no longer uses ReplNetworkState (#1084) fix: networkmanager prefab validation and no scene management manual test (#1073) feat: snapshot. MTT-1088 Snapshot acknowledgment gaps (#1083) feat: Add a test to validate registration of metric types (#1072) chore!: Remove unsupported UNET Relay behavior (MTT-1000) (#1081) fix: 2+ inheritance from network behaviour causes compilation exception (#1078) (#1079) test: add networkscenemanager additive scene loading tests (#1076)
mollstam
pushed a commit
to Keepsake-Games/com.unity.netcode.gameobjects
that referenced
this pull request
Feb 13, 2023
…gies#1083) * feat: snapshot. Attempt at applying snapshot spawn branch over new message queue * feat: snapshot. Reusing spawn entries. * feat: snapshot fix, prefab handling * feat: snapshot. reduce log spam * snapshot: merge preparation. Removing old acks, removing unused variables * feat: snapshot. merge preparation. Removing old acks, removing unused variables * feat: snapshot. more reasonable sentinel checks around snapshot send/receive * feat: snapshot. small clenaup * feat: snapshot. Moving configuration items to NetworkConfig * feat: snapshot. Adjusting to code standard * refactor: moved code from SpawnInternal into separate SendToSnapshot function. This will allow reusing it from NetworkShow. But as-is, no impact * refactor: calling networkShow(NetworkObject) code in networkshow(List<NetworkObject>) instead of calling a deeper-nested function. Make the code more uniform and prepares for incoming snapshot changes * style: whitespace * refactor: also using NetworkHide(NetworkObject) to implement NetworkHide(List<NetworkObject>). Same reasons as for NetworkShow * feat: snapshot. Safer access to Connection RTT structures * feat: snapshot. placeholder comment for code to come later * feat: snapshot. Snapshot fully working for spawn test. Keeps reference to proper network manager. Allows sending spawn to individual instances * feat: snapshot. despawn going via snapshot. snapshot message size limitation. snapshot message packing in a LRU fashion * feat: snapshot. Incrementing the local span and despawn store (amortized linear) when game code demands too much spawn/despawns * feat: snapshot. Using different list to keep track of spawn despawn times. Probably not needed, might be revert in the future, but eases debugging. Also debug prints, obviously temporary * feat: snapshot. extra logging for debugging * fix: snapshot. MTT-1056 despawn loss * feat: snapshot. Proper test for length used in snapshot message. Removed redundant condition for target client list * fix: snapshot. NetworkHide in snapshot mode was incorrectly hiding the object on all machines * feat: snapshot. Keeping snapshot disabled for this release * style: coding standards fix * test: making the networkshow/hide test more resistant to timing difference on the test machine, waiting half a second instead of a tenth. * test: removing Sleep() code in NetworkShowHide that attempted to deal with timing. Used explicit timeout instead. Less flaky * feat: snapshot, computing the mask of past received messages. Not used yet. * feat: snapshot. First pass at PR code review * feat: snapshot. Adjusting to int ticks * feat: snapshot. Reducing the amount of logging, as this is getting merged to develop * feat: snapshot. MTT-1088. Acknowledgement for multiple messages, not just the latest * feat: snapshot. Enabling snapshot in this branch, by default * feat: snapshot. Fixing a bad merge from develop * feat: snapshot. prep for PR, MTT-1088, plus packet loss for testing * feat: snapshot. prep 2 for PR, MTT-1088 * feat: snapshot. prep 3 for PR, MTT-1088. disbaling snapshot, and whitespace fix Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Snapshot-only. Still not enabled by default.
This PR adds a bitmask of past messages being acknowledged, not just the most recent one.