Skip to content

refactor!: remove NetworkChannel and MultiplexTransportAdapter#1133

Merged
0xFA11 merged 16 commits intodevelopfrom
refactor/kill-channels
Sep 3, 2021
Merged

refactor!: remove NetworkChannel and MultiplexTransportAdapter#1133
0xFA11 merged 16 commits intodevelopfrom
refactor/kill-channels

Conversation

@0xFA11
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@0xFA11 0xFA11 Sep 2, 2021

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
Copy Markdown
Contributor

@mattwalsh-unity mattwalsh-unity Sep 2, 2021

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
Copy Markdown
Contributor Author

@0xFA11 0xFA11 Sep 2, 2021

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
}

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
Copy Markdown
Contributor Author

@0xFA11 0xFA11 Sep 2, 2021

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
Copy Markdown
Contributor Author

@0xFA11 0xFA11 Sep 2, 2021

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 writer = new DataStreamWriter(data.Count + 1 + 4, Allocator.Temp);
writer.WriteByte((byte)networkChannel);
writer.WriteByte((byte)networkDelivery);
Copy link
Copy Markdown
Contributor

@mattwalsh-unity mattwalsh-unity Sep 2, 2021

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
Copy Markdown
Contributor Author

@0xFA11 0xFA11 Sep 2, 2021

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
Copy Markdown
Contributor Author

@0xFA11 0xFA11 Sep 2, 2021

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
Copy Markdown
Contributor

@andrews-unity andrews-unity Sep 2, 2021

Choose a reason for hiding this comment

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

Kill it

Copy link
Copy Markdown
Contributor

@Cosmin-B Cosmin-B Sep 2, 2021

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
Copy Markdown
Contributor

@mattwalsh-unity mattwalsh-unity Sep 2, 2021

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

3 more 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

Reviewers whose approvals may not affect merge requirements

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