Skip to content

ci: Enabling MLAPI UTP Transport package#736

Merged
wackoisgod merged 16 commits intodevelopfrom
feature/enable-utp-package
Apr 19, 2021
Merged

ci: Enabling MLAPI UTP Transport package#736
wackoisgod merged 16 commits intodevelopfrom
feature/enable-utp-package

Conversation

@wackoisgod
Copy link
Copy Markdown
Contributor

@wackoisgod wackoisgod commented Apr 15, 2021

This enables in CI files the building and packaging of the MLAPI Transport UTP Package. This is needed for the STAR document, it also ensures that we not breaking code in the package.

Also fixes code formatting issues in the MLAPI UTP Package.

Also makes the min version of things 2020.3.0f1

Note This is not the final version of the MLAPI Transport package implementation/testing this is just to get CI working and package validation passing.

Fixing naming of asmdef file and fixing yaml files
- 2021.1
- 2021.2
- 2020.3
- 2019.4
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

um, we want to disable 2019 validation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was called out in the Netcode slack :D that since UTP doesn't support 2019.4 and fails all 2019.4 we would up the min spec to 20.3


public override ulong GetCurrentRtt(ulong clientId) => 0;

private NetworkPipeline[] networkPipelines = new NetworkPipeline[3];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice cleanup to not need these anymore

"name": "com.unity.multiplayer.mlapi",
"displayName": "MLAPI Networking Library",
"version": "0.1.0",
"unity": "2019.4",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This means the min for MLAPI becomes 2020.3?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct :D

writer.WriteBytes(dataArrayPtr, data.Count);
}

SendToClient(writer.AsNativeArray(), peerId, pipelineIndex);
Copy link
Copy Markdown
Contributor

@mattwalsh-unity mattwalsh-unity Apr 16, 2021

Choose a reason for hiding this comment

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

I'm missing how pipelineIndex gets set. Else it will always use the NullPipelineState?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea as mentioned in the comment we are re-writing the this, but in order to move the STAR Document forward I need to be able to publish the package and pass validation and I also had to fix compile errors since this package wasn't in CI to begin with.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, ok...I guess if this feature is disabled and we can't ship without it re-enabled it would seem worth a comment here and/or ticket targeted for 1.0.0 so we don't forget

Copy link
Copy Markdown
Contributor Author

@wackoisgod wackoisgod Apr 16, 2021

Choose a reason for hiding this comment

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

We have a ticket in the Server Jira to re-write all this code :P

using UnityEngine.TestTools;
using NUnit.Framework;
using Unity.Networking.Transport;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be practical to add a test where we standup 2 UTPTransport instances and send to each other?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, we will do that :P as mentioned before had to add testing to get validation passing.

Copy link
Copy Markdown
Contributor

@JesseOlmer JesseOlmer left a comment

Choose a reason for hiding this comment

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

lgtm

@wackoisgod wackoisgod merged commit b7357f8 into develop Apr 19, 2021
@wackoisgod wackoisgod deleted the feature/enable-utp-package branch April 19, 2021 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants