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.

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

Network system #269

Merged
merged 14 commits into from Jul 10, 2017

Conversation

Projects
None yet
4 participants
@Acruid
Contributor

Acruid commented Jul 10, 2017

There were three goals to this. The first was to combine the client and server network managers into one shared manager that can be accessed from anywhere. The second was to remove the lidgren networking code from SS14Server. The third was to encapsulate the server packets so that some form of sanitizing can be done.

Things that were not in the scope of this were changes to the client/server communication protocol, and converting the client/shared to the new network system. This is currently designed to work with minimal changes to the client, and minimal changes outside of SS14Server (now named BaseServer, maybe the content assembly could inherit from it?), SS14.Server.Networking*, and SS14.Server.Player*.

The public interfaces for the NetManager are pretty much what you would expect, check out the doc comments on them. Packets are now their own object that serializes and deserializes itself. The big benefit to this is the system can enforce a template and easily detect malformed packets.

Systems define their own packets and register them with the NetManager and a callback. Currently since Client code has not been migrated, they still rely on the static shared NetworkMessages enum. This should be removed in the future. You can currently find the existing NetMessages for the game inside SS14.Shared.Network.Messages. Yes, the protocol is a mess.

I'm not super satisfied with how this turned out but it does meet my goals, and I think it is way better than what we had before. I do believe it would be a good base to keep building off of. I have been coding this in a vacuum though, there may be something major I missed. Comments are always welcome.

Acruid added some commits Jul 2, 2017

Saving progress of the SS14.Server.Network*, SS14.Server.Player*, and…
… SS14.Server.SS14Server.cs rewrite.

It compiles, but does not run.
Nightly commit, Still bugged, but compiles.
Made progress, it can now show the map!
Finished removing most of lidgren from SS14.Server.
General cleanup of networking system.
Adds the INetServerManager interfaces for servers to use.
Remade the StringTable system into it's own class.
Fixed a few random annoying bugs.
Got the OnConnecting event working, other modules can accept/deny inc…
…oming connections.

Removed the giant block of private config vars in BaseServer.
Fixed exception issues, now networks exceptions are caught when a pac…
…ket fails to deserialize.

Final housekeeping.

Acruid added some commits Jul 10, 2017

@PJB3005

Pretty good overall, definitely a good idea.

/// <summary>
/// The Client version of the INetManager.
/// </summary>
public interface INetClientManager : INetManager

This comment has been minimized.

@PJB3005

PJB3005 Jul 10, 2017

Member

Probably better to rename this to IClientNetManager

/// <summary>
/// Is this a server, or a client?
/// </summary>
bool IsServer { get; }

This comment has been minimized.

@PJB3005

PJB3005 Jul 10, 2017

Member

Using an enum here is probably a better idea. Though neither is probably best, having code need to check whether it's client or server like this seems like poor design.

This comment has been minimized.

@Acruid

Acruid Jul 10, 2017

Contributor

After further discussion, it was decided that these fields will stay unmodified. If they are not needed by shared code in the future, they can be removed at a later date. An enum is overkill because there are only 2 states, being a client or a server.

/// <summary>
/// The server version of the INetManager.
/// </summary>
public interface INetServerManager : INetManager

This comment has been minimized.

@PJB3005

PJB3005 Jul 10, 2017

Member

IServerNetManager

_callbacks = new Dictionary<Type, ProcessMessage>();
_channels = new Dictionary<NetConnection, NetChannel>();

_config = IoCManager.Resolve<IConfigurationManager>();

This comment has been minimized.

@PJB3005

PJB3005 Jul 10, 2017

Member

IoC resolution from constructors like this is an implementation detail and incredibly prone to race conditions. Implement SS14.Shared.IoC.IPostInjectInit and do this from PostInject() with a hard [Dependency] to the manager.

public MsgAdmin(INetChannel channel) : base(NAME, GROUP, ID) { }
#endregion

public int EntityId;

This comment has been minimized.

@PJB3005

PJB3005 Jul 10, 2017

Member

Sonar is gonna kill you over these public fields, just an FYI. Make them auto implemented properties ({ get; set; }) and it'll be happy.

}

#if DEBUG
config.RegisterCVar("net.fakelag", false, CVarFlags.CHEAT);

This comment has been minimized.

@PJB3005

PJB3005 Jul 10, 2017

Member

Seems inconsistent to register part here and part in the constructors.

/// </summary>
/// <param name="connection">The raw connection of the peer.</param>
/// <returns>The NetChannel of the peer.</returns>
private INetChannel GetChannel(NetConnection connection)

This comment has been minimized.

@PJB3005

PJB3005 Jul 10, 2017

Member

Should probably make this throw an exception if it can't be found.

return null;
try
{
retVal = GetChannel(_netPeer.Connections[0]);

This comment has been minimized.

@PJB3005

PJB3005 Jul 10, 2017

Member

How could this throw an exception?

/// <summary>
/// Error state, the message needs to set a different one.
/// </summary>
ERROR = 0,

This comment has been minimized.

@PJB3005

PJB3005 Jul 10, 2017

Member

Are all caps names like these even standard in .NET land? Shouldn't they simply be PascalCase?

@unusualcrow

This comment has been minimized.

unusualcrow commented Jul 10, 2017

holy fuck nice work

Acruid added some commits Jul 10, 2017

@PJB3005 PJB3005 merged commit 8eaf11c into space-wizards:master Jul 10, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@wafflebot wafflebot bot removed the W: In Progress label Jul 10, 2017

@Silvertorch5

This comment has been minimized.

Member

Silvertorch5 commented Jul 10, 2017

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment