miwarnec / DOTSNET

Public DOTSNET issue tracker
20 stars 0 forks source link

Ghost observable object #19

Open dlunhor opened 4 years ago

dlunhor commented 4 years ago

Describe the bug Ghosting of one of the observable objects

proof

To Reproduce

  1. Clear install all
  2. Build
  3. Run server and connect in editor, run client standalone and move where players dont see each other
gabrjt commented 4 years ago

Seems like the same issues I was having and commented here.

What solved for me was modifying the SystemGroups in order to WriteMessageSystems to execute after all simulation in the frame has been completed and ReadMessageSystems to set received values before the simulation begins.

gabrjt commented 4 years ago

So I confirmed that the issue is indeed related to System execution order.

Here is the ghosting happening. And here is the ghosting fix.

I'm using Unity 2020.1.2f1, Entities 0.14.0-preview.18 and DOTSNET 1.15 (will update to 1.16 soon).

The changes are pretty straightforward, when I've updated from 1.14 to 1.15 it took me no time. It's just a matter of changing the UpdateInGroup attributes.

When I used the default SpawnSystem in the Benchmark example, which executes in the middle of the simulation, the bug still occured. But when I used my custom ReadSpawnMessageSystem, the bug was fixed.

using DOTSNET;
using Unity.Collections;
using Unity.Entities;

[DisableAutoCreation]
[UpdateInGroup(typeof(ReadMessageSystemGroup), OrderFirst = true, OrderLast = false)]
public class ReadSpawnMessageSystem : NetworkClientMessageSystem<SpawnMessage>
{
    NativeList<SpawnMessage> _list;

    protected override void OnCreate()
    {
        base.OnCreate();

        _list = new NativeList<SpawnMessage>(ClientExtensions.DefaultNativeContainerCapacity, Allocator.Persistent);
    }

    protected override void OnStopRunning()
    {
        base.OnStopRunning();

        _list.Clear();
    }

    protected override void OnDestroy()
    {
        base.OnDestroy();

        if (_list.IsCreated)
        {
            _list.Dispose();
        }
    }

    protected override void OnUpdate()
    {
        for (var index = 0; index < _list.Length; index++)
        {
            var message = _list[index];

            client.Spawn(
                message.prefabId,
                message.netId,
                message.owned != 0,
                message.position,
                message.rotation
            );
        }

        _list.Clear();
    }

    protected override void OnMessage(SpawnMessage message)
    {
        _list.Add(message);
    }
}

The general idea is still the same from what I've commented before in that closed issue, but I'll write it down and show it with some code soon.

gabrjt commented 4 years ago

Updating to 1.16 was a good moment to review these system execution order changes and write in detail :)

Problem: Reading (receiving) and writing (sending) messages arbitrarily in the middle of the simulation can lead to gameplay simulation issues. This happens because systems that depends on the received data may have a one frame delay in processing them. Systems that send messages may send messages that does not correspond to the final state after the simulation is complete.

Proposal: Segregate the simulation group into 3 three main groups:

Implementation:

1. Create the three main system groups: Simple as creating new system groups and defining their execution order.

using Unity.Entities;

namespace DOTSNET
{
    [ServerWorld]
    [ClientWorld]
    [UpdateInGroup(typeof(ApplyPhysicsGroup))]
    public class NetworkSimulationSystemGroup : ComponentSystemGroup
    {
        protected override void OnCreate()
        {
            UseLegacySortOrder = false;

            base.OnCreate();
        }
    }
}
using Unity.Entities;

namespace DOTSNET
{
    [ServerWorld]
    [ClientWorld]
    [UpdateBefore(typeof(ApplyPhysicsGroup))]
    public class ReadMessageSystemGroup : ComponentSystemGroup
    {
        protected override void OnCreate()
        {
            UseLegacySortOrder = false;

            base.OnCreate();
        }
    }
}
using Unity.Entities;
using Unity.Physics.Systems;

namespace DOTSNET
{
    [ServerWorld]
    [ClientWorld]
    [UpdateAfter(typeof(EndFramePhysicsSystem))]
    public class WriteMessageSystemGroup : ComponentSystemGroup
    {
        protected override void OnCreate()
        {
            UseLegacySortOrder = false;

            base.OnCreate();
        }
    }
}

2. Refactor ServerActiveSimulationSystemGroup and ClientConnectedSimulationSystemGroup: Those groups were used before to execute any netcode aware system. They still exist, but now with the resposability of enabling/disabling those three new groups depending on the network state.

// some server systems should only be updated while the server is active.
//
// for example:
//   * Transport: update all the time so we can actually start/stop the server
//   * MonsterMovement: update only while server is running
//
// usage:
//   add the UpdateInGroup attribute to a system:
//     [UpdateInGroup(typeof(NetworkSimulationSystemGroup))]
//   OnStartRunning / OnUpdate will only be called when the server is active!
//   OnStopRunning will be called when the server stops being active!

using Unity.Entities;

namespace DOTSNET
{
    // [ServerWorld] adds it to server world automatically. no bootstrap needed!
    [ServerWorld]
    [AlwaysUpdateSystem]
    // Systems in group may need to apply physics, so update in the safe group
    [UpdateInGroup(typeof(ApplyPhysicsGroup), OrderFirst = true, OrderLast = false)]
    public class ServerActiveSimulationSystemGroup : ComponentSystemGroup
    {
        // dependencies
        [AutoAssign] NetworkServerSystem _networkServerSystem;
        [AutoAssign] NetworkSimulationSystemGroup _networkSimulationSystemGroup;
        [AutoAssign] ReadMessageSystemGroup _readMessageSystemGroup;
        [AutoAssign] WriteMessageSystemGroup _writeMessageSystemGroup;

        protected override void OnCreate()
        {
            UseLegacySortOrder = false;

            base.OnCreate();
        }

        protected override void OnUpdate()
        {
            // enable/disable systems based on server state
            // IMPORTANT: we need to set .Enabled to false after StopServer,
            //            otherwise OnStopRunning is never called in the group's
            //            systems. trying to call OnUpdate only while ACTIVE
            //            would not call OnStopRunning after disconnected.

            _networkSimulationSystemGroup.Enabled =
                _readMessageSystemGroup.Enabled =
                    _writeMessageSystemGroup.Enabled =
                        _networkServerSystem?.state == ServerState.ACTIVE;

            // always call base OnUpdate, otherwise nothing is updated again
            base.OnUpdate();
        }
    }
}
// some client systems should only be updated while the client is connected.
//
// for example:
//   * Transport: update all the time so we can actually (dis)connect the client
//   * LocalPlayerMovement: update only while client is running
//
// usage:
//   add the UpdateInGroup attribute to a system:
//     [UpdateInGroup(typeof(NetworkSimulationSystemGroup))]
//   OnStartRunning / OnUpdate will only be called when the client is connected!
//   OnStopRunning will be called when the client disconnected!

using Unity.Entities;

namespace DOTSNET
{
    // [ClientWorld] adds it to client world automatically. no bootstrap needed!
    [ClientWorld]
    [AlwaysUpdateSystem]
    // Systems in group may need to apply physics, so update in the safe group
    [UpdateInGroup(typeof(ApplyPhysicsGroup), OrderFirst = true, OrderLast = false)]
    public class ClientConnectedSimulationSystemGroup : ComponentSystemGroup
    {
        // dependencies
        [AutoAssign] NetworkClientSystem _networkClientSystem;
        [AutoAssign] NetworkSimulationSystemGroup _networkSimulationSystemGroup;
        [AutoAssign] ReadMessageSystemGroup _readMessageSystemGroup;
        [AutoAssign] WriteMessageSystemGroup _writeMessageSystemGroup;

        protected override void OnCreate()
        {
            UseLegacySortOrder = false;

            base.OnCreate();
        }

        protected override void OnUpdate()
        {
            // enable/disable systems based on client state
            // IMPORTANT: we need to set .Enabled to false after Disconnect,
            //            otherwise OnStopRunning is never called in the group's
            //            systems. trying to call OnUpdate only while CONNECTED
            //            would not call OnStopRunning after disconnected.

            _networkSimulationSystemGroup.Enabled =
                _readMessageSystemGroup.Enabled =
                    _writeMessageSystemGroup.Enabled =
                        _networkClientSystem?.state == ClientState.CONNECTED;

            // always call base OnUpdate, otherwise nothing is updated again
            base.OnUpdate();
        }
    }
}

3. Refactor Core Systems to make usage of the new System Groups:

First, the NetworkServerSystem and the ClientNetworkSystem. I've changed their execution order to reflect their original execution order.

// NetworkServerSystem should be updated AFTER all other server systems.
// we need a guaranteed update order to avoid race conditions where it might
// randomly be updated before other systems, causing all kinds of unexpected
// effects. determinism is always a good idea!
// (this way NetworkServerMessageSystems can register handlers before
//  OnStartRunning is called. remember that for all server systems,
//  OnStartRunning is called the first time only after starting, so they
//  absolutely NEED to be updated before this class, otherwise it would be
//  impossible to register a ConnectMessage handler before
//  OnTransportConnected is called (so the handler would never be called))
[ServerWorld]
[UpdateAfter(typeof(WriteMessageSystemGroup))]
// use SelectiveAuthoring to create/inherit it selectively
[DisableAutoCreation]
public class NetworkServerSystem : SystemBase
{ 

        ...

        protected override void OnStartRunning()
        {
                ...

                // do we have a simulation system group? then set tick rate.
                // (we won't have a group when running tests)
                NetworkSimulationSystemGroup group = World.GetExistingSystem<NetworkSimulationSystemGroup>();
                if (group != null)
                {
                    // CatchUp would be bad idea because it could lead to deadlocks
                    // under heavy load, where only the simulation system is updated
                    // so we use Simple.
                    float tickInterval = 1 / tickRate;
                    // TODO use EnableFixedRateSimple after it was fixed. right now
                    // it only sets deltaTime but updates with same max frequency
                    FixedRateUtils.EnableFixedRateWithCatchUp(group, tickInterval);
                    Debug.Log("NetworkServerSystem: " + group + " tick rate set to: " + tickRate);
                }

                ...
        }

        ...
}
// NetworkClientSystem should be updated AFTER all other client systems.
// we need a guaranteed update order to avoid race conditions where it might
// randomly be updated before other systems, causing all kinds of unexpected
// effects. determinism is always a good idea!
[ClientWorld]
[UpdateAfter(typeof(WriteMessageSystemGroup))]
// use SelectiveAuthoring to create/inherit it selectively
[DisableAutoCreation]
public class NetworkClientSystem : SystemBase
{ ... }

One thing that I didn't understand were these comments that existed before:

// Server may need to apply physics, so update in the safe group
[UpdateInGroup(typeof(ApplyPhysicsGroup))]
// Client may need to apply physics, so update in the safe group
[UpdateInGroup(typeof(ApplyPhysicsGroup))]

It's not clear to me how these systems are related to updating the physics simulation group. Seems to be working with this setup though. In this example there are 65k dynamic bodies (asteroids) being (un)spawned and the cameras are also physics simulated. The ghosting is also not happening anymore in this example.

With that said, let's move on to the next DOTSNET core systems. The default (read) message systems should now be updated in the ReadMessageSystemGroup.

namespace DOTSNET
{
    [ServerWorld]
    [UpdateInGroup(typeof(ReadMessageSystemGroup))]    
    public abstract class NetworkServerMessageSystem<T> : SystemBase
        where T : unmanaged, NetworkMessage
    { ... }
}
namespace DOTSNET
{
    [ClientWorld]
    [UpdateInGroup(typeof(ReadMessageSystemGroup))]
    public abstract class NetworkClientMessageSystem<T> : SystemBase
        where T : unmanaged, NetworkMessage
    { ... }

And the NetworkBroadcastSystem now belongs to the WriteMessageSystemGroup.

namespace DOTSNET
{
    // note: [AlwaysUpdateSystem] isn't needed because we should only broadcast
    //       if there are entities around.
    [ServerWorld]
    [UpdateInGroup(typeof(WriteMessageSystemGroup))]
    // ComponentSystem for now because Jobs can't send packets
    // We can try a Job later if we queue packets.
    public abstract class NetworkBroadcastSystem : SystemBase
    { ... }

Finally, the InterestManagementSystem is also in the WriteMessageSystemGroup, updating before the broadcast systems by default.

namespace DOTSNET
{
    // ComponentSystem for now. Jobs come later.
    [ServerWorld]
    [AlwaysUpdateSystem]
    [UpdateInGroup(typeof(WriteMessageSystemGroup), OrderFirst = true, OrderLast = false)]
    // IMPORTANT: use [UpdateBefore(typeof(BroadcastSystem))] when inheriting
    // IMPORTANT: use [DisableAutoCreation] + SelectiveSystemAuthoring when
    //            inheriting
    public abstract class InterestManagementSystem : SystemBase
    { ... }

And this covers the core changes.

4. Refactor Game specific Systems to make usage of the new System Groups:

It's now a matter of refactoring the other systems to be in one of the three possible update system groups. Usually, systems that read messages will already be in the ReadMessageSystemGroup by default since they inherit from the NetworkServerMessageSystem or the NetworkClientMessageSystem. Similarly, Broadcast and Interest Management systems will be in the WriteMessageSystemGroup by default for inherintance reasons. This leaves us with gameplay simulation systems, that should be in the NetworkSimulationSystemGroup and custom systems that sends messages, that should be in the WriteMessageSystemGroup.

Here are some DOTSNET systems that were refactored to match this new pattern.

namespace DOTSNET.Examples.Benchmark
{
    [ClientWorld]
    [UpdateInGroup(typeof(WriteMessageSystemGroup))]
    // use SelectiveSystemAuthoring to create it selectively
    [DisableAutoCreation]
    public class AutoJoinWorldSystem : SystemBase { ... }
}
namespace DOTSNET.Examples.Benchmark
{
    [ClientWorld]
    [UpdateInGroup(typeof(NetworkSimulationSystemGroup))]
    // use SelectiveSystemAuthoring to create it selectively
    [DisableAutoCreation]
    public class PlayerMovementSystem : SystemBase { ... }
}
namespace DOTSNET
{
    // note: [AlwaysUpdateSystem] isn't needed because we should only broadcast
    //       if there are entities around.
    [ClientWorld]
    [UpdateInGroup(typeof(WriteMessageSystemGroup))]
    // use SelectiveAuthoring to create/inherit it selectively
    [DisableAutoCreation]
    public class NetworkTransformClientSystem : SystemBase { ... }
}

This pretty much covers it all and with these changes the samples should still work and without ghosting. But there is still more to be said.

Final Considerations and Caveats:

The gameplay simulation group may run in fixed update but the read/write groups may not. Thus, it's recommended to avoid gameplay logic in the read/write systems at all costs. Read message systems should set values to be processed in the simulation. Write message systems will send the desired simulation data results. One may have to make use of dynamic buffers to avoid data loss on the simulation when processing the messages received and when sending sensible data from the simulation. But for example, this is not the case for sending Transform messages where we only care about the final Transform after the simulation is complete.

An example would be the Server receiving inputs in a read message system and adding these inputs in the target entity input buffer. Then, there would be another system in the simulation group that would process these inputs. I've actually done that with the spectator cameras in the video.

Speaking of which, another thing worth noticing is that the same system from the simulation group may easily run in the client and in the server since we now only have a NetworkSimulationSystemGroup instead of one specific simulation group for the server and another for the client.

This new architecture brings a little more complexity upfront, but it is something that may be inherently complex from network multiplayer games.

Also, this is something that worked for me and I was willing to share, not that this is the correct and only way to be done hahaha 😅 Would love to discuss more about these strategies in general.

It will probably get more complex as we keep diving in DOTS/ECS/DOTSNET.

Thanks!