miwarnec / DOTSNET

Public DOTSNET issue tracker
20 stars 0 forks source link

DOTSNET DestroyAllNetworkEntities() does not considers entities with LinkedEntityGroup that have entities that does not have NetworkEntity component #11

Closed gabrjt closed 4 years ago

gabrjt commented 4 years ago

Whenever the client or the server is going to shutdown, all network entities are destroyed. While this is fine, more complex entities may have a LinkedEntityGroup with another entities that do not have the NetworkEntity component (and they may not necessarily need to have).

Steps to reproduce:

  1. Spawn a NetworkEntity.
  2. Add an entity that has no NetworkEntity component to its LinkedEntityGroup.
  3. Stop server or client.

The following error will be logged:

ArgumentException: DestroyEntity(EntityQuery query) is destroying entity Entity(32:1) which contains a LinkedEntityGroup and the entity Entity(34:1) in that group is not included in the query. If you want to destroy entities using a query all linked entities must be contained in the query..
...
DOTSNET.NetworkClientSystem.DestroyAllNetworkEntities () (at Assets/Third Party/DOTSNET/Scripts/ECS/NetworkClient/NetworkClientSystem.cs)
...

A simple way to solve this bug is to defer this destroy action.

I've created to new files in the DOTSNET package:

DOTSNET/Scripts/ECS/Destroy/Destroy.cs

using Unity.Entities;

namespace DOTSNET
{
    public struct Destroy : IComponentData { }
}
DOTSNET/Scripts/ECS/Destroy/DestroySystem.cs

using Unity.Collections;
using Unity.Entities;

namespace DOTSNET
{
    [ServerWorld, ClientWorld]
    public class DestroySystem : SystemBase
    {
        EntityQuery _query;

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

            _query = GetEntityQuery(ComponentType.ReadOnly<Destroy>());
        }

        protected override void OnUpdate()
        {
            var entityList = new NativeList<Entity>(Allocator.Temp);

            Entities
                .WithAll<Destroy>()
                .ForEach(
                    (DynamicBuffer<LinkedEntityGroup> linkedEntityGroup) =>
                    {
                        entityList.AddRange(linkedEntityGroup.AsNativeArray().Reinterpret<Entity>());
                    }
                )
                .Run();

            EntityManager.AddComponent<Destroy>(entityList.AsArray());
            EntityManager.DestroyEntity(_query);
        }
    }
}

And then I've had to change DestroyAllNetworkEntities in both NetworkClientSystem and NetworkServerSystem to the following:

protected void DestroyAllNetworkEntities()
{
        // the query does NOT include prefabs
        EntityManager.AddComponent<Destroy>(GetEntityQuery(typeof(NetworkEntity)));
}

And now we have network entities batch destroy with LinkedEntityGroup support :)

LuisEGomezV commented 4 years ago

namespace DOTSNET { public class Destroy : IComponentData { } }

Maybe you didn't write like this in your actual code, but just in case, you're using class component for tagging

gabrjt commented 4 years ago

namespace DOTSNET { public class Destroy : IComponentData { } }

Maybe you didn't write like this in your actual code, but just in case, you're using class component for tagging

Woah, didn't notice that I was using class on this guy! Must have been automatically completed by the IDE. Thanks for pointing it out!

miwarnec commented 4 years ago

@gabrjt reading about LinkedEntityGroup right now. for what case is this needed in a game? is this for hybrid conversion only?

gabrjt commented 4 years ago

The article from gametorrahod explains in detail the use of LinkedEntityGroup, but it's basically a way of logically linking entities without a transform parent-child relationship.

It automatically happens when an entity gets converted and it has a child Game Object that is also converted, or when an entity is converted to a prefab. So its more related to entity relationships, not only for hybrid conversions.

That's why when you have this scenario, of a converted entity with a child entity, and the parent is destroyed, the child is destroyed too. Not because they were parent and child, but because they were linked by a LinkedEntityGroup.

It's also good for enabling/disabling entities. When you call EntityManager.SetEnabled(entity, enabled), it will automatically enable/disable all entities from the LinkedEntityGroup.

It's been useful for me both in the server and in the client.

For example, in the server, when a player joins the world, I do not spawn a playable entity right away for that player. An "EntityController" is spawned for the player that does not even have Translation/Rotation. Then, the player can decide what kind of entity he/she wants to play with (spectator camera, spaceship) and this spawned entity is linked to the EntityController using LinkedEntityGroup. So if SetEnabled() is called on the EntityController, the controlled entity will also be enabled/disabled even if they are not in a parent/child transform relationship.

Also in the server, I do some batch instantiating and set the instantiated entities disabled while I define their translations and rotations using parallel jobs, and only when they are done I reenable them and call NetworkServerSystem.Spawn() on them.

In the client, entities usually have more complex hierarchies than in the server, since they may have child entities for meshes for example. Even if meshes are not being processed in the server, they don't exist at all in the server in order to reduce the size of the entity archetype and avoid memory waste. But in the client, even if this child entity is not network aware, it will get destroyed alongside with it's parent since they are bound by a LinkedEntityGroup. Same happens for the Camera entity of a spectator camera.

But since this constraint exists when batch destroying entities of a LinkedEntityGroup, that they must all belong to the same query, just tagging them with a common "destroy" tag works good enough without adding too much complexity and in a performant way :)

miwarnec commented 4 years ago

@gabrjt thanks for explanation again. nice find. fix is in V1.14 :)

shinyclef commented 4 years ago

I am having the same issue in V1.16. I can see the fix with comments in NetworkServerSystem, but the issue still remains in NetworkClientSystem. I fixed it by copying the code over to NetworkClientSystem, and it works fine.

miwarnec commented 2 years ago

fix is in next version. thanks!