refactor!: remove channels, instead use delivery QoS directly#1116
Closed
mattwalsh-unity wants to merge 1 commit intodevelopfrom
Closed
refactor!: remove channels, instead use delivery QoS directly#1116mattwalsh-unity wants to merge 1 commit intodevelopfrom
mattwalsh-unity wants to merge 1 commit intodevelopfrom
Conversation
mattwalsh-unity
commented
Aug 31, 2021
|
|
||
| var writer = new DataStreamWriter(data.Count + 1 + 4, Allocator.Temp); | ||
| writer.WriteByte((byte)networkChannel); | ||
| writer.WriteByte((byte)networkDelivery); |
Contributor
Author
There was a problem hiding this comment.
I'm assuming actually all these writes can be deleted, because this is probably so that the receiver can route the packet to the right channel, and if we don't care about channels this can be deleted.
Contributor
There was a problem hiding this comment.
That is cirrect we can get rid of them.
0xFA11
changed the title
0xFA11
reviewed
Aug 31, 2021
Comment on lines
+57
to
+61
| //hrm for (int i = 0; i < Channels.Count; i++) | ||
| //hrm { | ||
| //hrm // Set the channels to a incrementing value | ||
| //hrm Channels[i].Id = (byte)((byte)NetworkDelivery.ChannelUnused + i); | ||
| //hrm } |
Contributor
There was a problem hiding this comment.
kill OnValidate() entirely here?
0xFA11
reviewed
Aug 31, 2021
| m_ChannelIdToName.Add(channelId, (NetworkChannel)Channels[i].Id); | ||
| m_ChannelNameToId.Add((NetworkChannel)Channels[i].Id, channelId); | ||
| } | ||
| // Built-in netcode channels |
Contributor
There was a problem hiding this comment.
we should probably convert "built-in" channels to use delivery types directly and get rid of this block here in this PR.
0xFA11
reviewed
Aug 31, 2021
Comment on lines
+22
to
+23
| NetworkEvent serverEvent = server.PollEvent(out ulong clientId, out NetworkDelivery _, out _, out _); | ||
| NetworkEvent clientEvent = client.PollEvent(out ulong serverId, out NetworkDelivery _, out _, out _); |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| NetworkEvent serverEvent = server.PollEvent(out ulong clientId, out NetworkDelivery _, out _, out _); | |
| NetworkEvent clientEvent = client.PollEvent(out ulong serverId, out NetworkDelivery _, out _, out _); | |
| var serverEvent = server.PollEvent(out ulong clientId, out NetworkDelivery _, out _, out _); | |
| var clientEvent = client.PollEvent(out ulong serverId, out NetworkDelivery _, out _, out _); |
| { | ||
| clientId = 0; | ||
| networkChannel = NetworkChannel.ChannelUnused; | ||
| networkDelivery = NetworkDelivery.ReliableSequenced; //?? |
Contributor
There was a problem hiding this comment.
Do we actually need to pop this now? because we did this before so the NetworkManager would know the channel but thats not required anymore?
| Marshal.Copy((IntPtr)message.Data, arr, 0, size); | ||
| var payload = new ArraySegment<byte>(arr); | ||
| InvokeOnTransportEvent((NetcodeEvent)message.Type, clientId, (NetworkChannel)message.ChannelId, payload, Time.realtimeSinceStartup); | ||
| InvokeOnTransportEvent((NetcodeEvent)message.Type, clientId, (NetworkDelivery)message.ChannelId, payload, Time.realtimeSinceStartup); |
Contributor
There was a problem hiding this comment.
Same as above does the above caller need to know the QoS?
| if (MessageQueueContainer.IsUsingBatching()) | ||
| { | ||
| m_MessageBatcher.ReceiveItems(messageStream, ReceiveCallback, clientId, receiveTime, networkChannel); | ||
| m_MessageBatcher.ReceiveItems(messageStream, ReceiveCallback, clientId, receiveTime, networkDelivery); |
Contributor
There was a problem hiding this comment.
I assume the batching needs to know about the QoS?
Contributor
|
superseded by PR #1133 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
We probably won't have time to get this in, but I got this far today while waiting to work on other things. It builds; I haven't checked all the transports.