Skip to content

refactor!: remove NetworkChannel and MultiplexTransportAdapter #1133

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

Jump to bottom
Merged
merged 16 commits into from
Sep 3, 2021
Merged

refactor!: remove NetworkChannel and MultiplexTransportAdapter #1133

merged 16 commits into from
Sep 3, 2021

Conversation

0xFA11
Copy link
Contributor

@0xFA11 0xFA11 commented Sep 2, 2021

FYI @Cosmin-B

@0xFA11 0xFA11 changed the title refactor!: remove NetworkChannel from the SDK (NetworkTransport, NetworkVariable etc.) refactor!: remove NetworkChannel from the Netcode SDK Sep 2, 2021
0xFA11
0xFA11 commented Sep 2, 2021
View reviewed changes
com.unity.netcode.gameobjects/Runtime/Transports/NetworkTransport.cs

/// <summary>
/// A network transport
/// </summary>
public abstract class NetworkTransport : MonoBehaviour
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are lots of API breaking changes (for good) in this file. in fact, these changes are influencing changes in other places as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you and @andrews-unity are coordinating since he's got his transport refactor coming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, we had a quick chat about this and he'll also review this PR too.

0xFA11
0xFA11 commented Sep 2, 2021
View reviewed changes
com.unity.multiplayer.transport.utp/Runtime/UTPTransport.cs Outdated
@@ -260,14 +254,14 @@ public void SendToClient(NativeArray<byte> packet, ulong clientId, int index)
}
}

public override unsafe void Send(ulong clientId, ArraySegment<byte> data, NetworkChannel networkChannel)
public override unsafe void Send(ulong clientId, ArraySegment<byte> data, NetworkDelivery networkDelivery)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes in this file are actually just some temporary fixes. I reckon we're going to have a much better UTPTransport implementation soon :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(for instance, reading/writing channel IDs can go away but I just wanted to leave "proper" UTPTransport implementation for later, for a separate PR)

@0xFA11 0xFA11 mentioned this pull request Sep 2, 2021
Closed
mattwalsh-unity
mattwalsh-unity reviewed Sep 2, 2021
View reviewed changes
com.unity.multiplayer.transport.utp/Runtime/UTPTransport.cs Outdated
{
var pipelineIndex = 0;

GetUTPConnectionDetails(clientId, out uint peerId);

var writer = new DataStreamWriter(data.Count + 1 + 4, Allocator.Temp);
writer.WriteByte((byte)networkChannel);
writer.WriteByte((byte)networkDelivery);
Copy link
Contributor

Choose a reason for hiding this comment

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

So, do we actually need to send the network delivery? I was assuming before we sent the channel so that we could on the receiving end route to the right channel, but if we don't do that... (or maybe I don't understand what this is for)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, you're right. we don't need this and channel ID at all. I wanted to keep better UTPTransport implementation for later, see my comments above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @andrews-unity, if you want me to kill this in this PR, I can do that here even before we make UTPTransport great again :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Kill it

Copy link
Contributor

Choose a reason for hiding this comment

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

Double Kill it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Duh, we have to also delete the reading of it in Execute and ServerUpdateJob

mattwalsh-unity
mattwalsh-unity reviewed Sep 2, 2021
View reviewed changes
mattwalsh-unity
mattwalsh-unity reviewed Sep 2, 2021
View reviewed changes
@0xFA11 0xFA11 changed the title refactor!: remove NetworkChannel from the Netcode SDK refactor!: remove NetworkChannel and MultiplexTransportAdapter Sep 2, 2021
@0xFA11 0xFA11 merged commit 92e5361 into develop Sep 3, 2021
@0xFA11 0xFA11 deleted the refactor/kill-channels branch September 3, 2021 09:11
@0xFA11 0xFA11 mentioned this pull request Sep 7, 2021
Closed
SamuelBellomo added a commit that referenced this pull request Sep 13, 2021
@SamuelBellomo
Merge branch 'develop' into sam/feature/interpolation-for-network-tra…
0260b4b 
…nsform

* develop:
  fix: add `link.xml` to prevent IL2CPP stripping `Unity.PerformanceTesting` (#1172)
  chore: add boilerplate for `ClientNetworkTransform` sample (#1168)
  chore: remove `ClientNetworkVariable` (#1167)
  chore: Disable test while we reevaluate the assumption that INetworkM… (#1163)
  docs: rename Manual.md to Index.md
  Only track one metric for scene sync and do not report scene name (#1159)
  test: create job definitions for mobile build and test (#1152)
  test: make test runner scene ignored by default for BaseMultiInstanceTest (#1154)
  fix: remove left-over reference to SyncTransform (#1155)
  chore: remove unused SyncTransform.cs (#1153)
  chore!: remove NetworkNavMeshAgent (#1150)
  fix: NetworkObject parenting support in scene transitioning (#1148)
  chore!: rename Prototyping asmdef to Components (#1145)
  feat: add bootstrap sample to package (#1140)
  chore: remove `--yamato` param from `standards.py` (#1144)
  fix: MTT-504 connection approval messages and comparing networkconfig (#1138)
  refactor!: remove NetworkChannel and MultiplexTransportAdapter (#1133)

# Conflicts:
#	com.unity.netcode.gameobjects/Components/Interpolator.meta
#	com.unity.netcode.gameobjects/Components/Interpolator/BufferedLinearInterpolator.cs
#	com.unity.netcode.gameobjects/Components/Interpolator/BufferedLinearInterpolator.cs.meta
#	testproject/Assets/Prefabs/PlayerCube.prefab
mollstam pushed a commit to Keepsake-Games/com.unity.netcode.gameobjects that referenced this pull request Feb 13, 2023
@0xFA11
refactor!: remove NetworkChannel and MultiplexTransportAdapter (Unity…
b43ad07 
…-Technologies#1133)

* cleanup NetworkTransport API

* polish NetworkTransport API

* port UNetTransport

* kill MultiplexTransportAdapter

* kill TransportChannel

* NetworkVariableChannel -> Delivery

* update IInternalMessageHandler interface

* kill NetworkChannel around the codebase

* fix MessageQueueContainer

* update NetworkVariableBase constructor (make NetworkDelivery of NetworkVariable changable)

* minor grammar fix

* revert to NetworkVariable.Delivery being an internal const

* remove channels from UTPTransport

* `./standards.py --fix`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cosmin-B Cosmin-B Cosmin-B left review comments

@andrews-unity andrews-unity andrews-unity approved these changes

@mattwalsh-unity mattwalsh-unity mattwalsh-unity approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0xFA11 @andrews-unity @Cosmin-B @mattwalsh-unity