Unity-Technologies / multiplayer-community-contributions

Community contributions to Unity Multiplayer Networking products and services.
MIT License
429 stars 161 forks source link

3rd party assemblies copied into transports #132

Open JamesMcGhee opened 2 years ago

JamesMcGhee commented 2 years ago

I noticed this with the SteamP2P transport, and we did work to fix it ... that is the original transport SteamP2P had a deep copy of Steamworks.NET assemblies.

The reason this is horrible and will break so many things is that

1) these transports are installed by Package Manager so you can’t modify their code when installed 2) these are transports not Steam integrations they are not usually up to date with the latest versions of those assemblies, and they don’t do anything with the rest of the API

Unity needs to sit down a standard that you should not copy in 3rd party dependent assemblies ever. The right solution is to reference them. This way they can be updated independently from the transport and this way if as would nearly always be the case with Steam API be that Steamworks.NET or Facepunch or some homegrown version .... if they are already installed it won’t conflict with the immutable package manager install

Looking at the Facepunch transport I noted it has the same issue

com.community.netcode.transport.facepunch

That transport has a deep copy of Facepunch's assemblies ... this means that if you already have Facepunch installed you now have conflicts, if you use any other tool that uses facepunch you now have conflicts, if you need to update Facepunch ... you now have conflicts.

Please establish the guidance to ensure that transports and your own code for that matter do not deep copy plugins, assemblies, etc. that they are not the original source author for. It’s a horrible practice that causes a lot of issues more so when you’re installing from Package Manager

styliann-eth commented 2 years ago

Can someone from Unity please help with this?

I think having either the steamnetworking transport or the face punch transport working seamlessly going forward is pretty high in the priorities as most developers would first release on Steam.

JamesMcGhee commented 2 years ago

@ns2808 Just so your aware as far as I am aware there is now a properly working Steam Networking transport for MLAPI aka NetCode for GameObjects (such a long name)

SteamNetworking

Carsillas commented 2 years ago

+1

150 is a direct result of this

JamesMcGhee commented 2 years ago

Just a note:

We have worked with Steamworks.NET to insure it can be installed via Package Manager and that it has a Script Define "STEAMWORKS_NET" this gives some possibilities with regards to dependency checks

For example in our own asset we check for Steamwroks.NET being present by listing the installed Packages from PackageManager's client class and just looking for Steamworks.NET.

You could also simply check for the script define e.g.

if STEAMWORKS_NET

endif

Point is there are ways of handling dependency checking, fair enough its not ideal in Unity since for example while you can indicate a dependency on a Unity package manager package like Mathematics you cant declare a dependency on a 3rd party like Stemworks.NET

But using installer scripts and script defines you can fake that level of dependency check.

Why did I mention this here?

Unity needs to establish a preferred practice for dependency on 3rd parties. The lack of one is why so many deep copy which creates huge issues down the road as noted above.

The tools we have now can work for dependency checking ... its cumbersome a bit sloppy but works, with some guidance from Unity and maybe a bit of additional tooling (dependency node in Package Manager's JSON being able to understand git Urls would be AWSOME) life could be made much better for everyone really.