EcsRx / ecsrx

A reactive take on the ECS pattern for .net game developers
MIT License
506 stars 36 forks source link

CustomComponentLookups not working with structs? #19

Open GetFunctional opened 4 years ago

GetFunctional commented 4 years ago

Hi,

I'm new to your framework and followed your guide from this page ( https://ecsrx.gitbook.io/project/performance/component-type-lookups ).

I created a Module for the Customlookups and added it to my application like this:

 protected override void LoadModules()
  {
        base.LoadModules();
        this.Container.LoadModule(new CustomComponentLookupsModule());
   }

When adding a component, that is a struct and was registered in the dictionary, to an entity via Blueprint I get an exception:

System.InvalidCastException HResult=0x80004002 Message=Unable to cast object of type 'EcsRx.Components.ComponentPool1[GF.FriendsInSpace.Core.Components.EntityIdComponent]' to type 'EcsRx.Components.IComponentPool1[EcsRx.Components.IComponent]'. Source=EcsRx StackTrace: at EcsRx.Components.Database.ComponentDatabase.GetPoolFor[T](Int32 componentTypeId) at EcsRx.Components.Database.ComponentDatabase.Set[T](Int32 componentTypeId, Int32 allocationIndex, T component) at EcsRx.Entities.Entity.AddComponents(IReadOnlyList`1 components) at EcsRx.Extensions.IEntityExtensions.AddComponents(IEntity entity, IComponent[] components)

I can work around this when replacing the struct with a class. But it would be great if you could make it work again or if I made a mistake maybe you can offer a solution :-)

grofit commented 4 years ago

Can you show what your registration logic looks like and what the component looks like?

It seems odd, as I know lots of bits (especially the Batching plugin) get the components and run with structs, but maybe they are not getting the pool, but either way lots of bits set components as structs in examples.

Are you able to make a simple repro that I can just clone and look at directly? my time is limited at the moment or I would go dig a bit deeper. I have kids running around trying to injure each other until late at night, and then I have a raft of other stuff to catch up on.

Also in most cases you never need to manually register stuff, its only if you want to save the lookup cost incurred if you know ahead of time the type you want, so if you are doing this for performance reasons then carry on as you are, if you are doing this for any other reasons it may be best to just use the default component type lookup system which does all the heavy lifting for you.

GetFunctional commented 4 years ago

Hey grofit

I'm pretty busy as well at the moment but I will try to provide a small reproduceable project soon :-)

GetFunctional commented 4 years ago

I think i found the issue. I think it has to do with the extensionmethods I used from EcsRx.Extensions. I created a small reproduceable console project for you to have a look. Issue19.zip

Making the EntityIdComponent a class "resolves" the issue

grofit commented 4 years ago

Right having a look over and I think there could do with being a few changes, as currently there is only one method to add a struct component, and you dont pass in the component you pass in the generic and the id for it, then it provides you the default component. There is a workaround where with structs you can actually just call GetComponent<T>(tyoeId) and get back an empty/current component for that entity.

So the other simpler methods for adding components are all for class based ones.

In hindsight this seems rubbish developer experience, so I think this could do with some changes, and hopefully it shouldn't be a case of moving mountains, but will need to look into it some more to see what the ramifications are for it, as struct and classes have slightly different flows through the underlying parts of the system due to the way they are handled.

Anyway to get you up and running if you change your code to this:

        public void Apply(IEntity entity)
        {
            var gameTimeComponent = new GameTimeComponent  {CurrentTime = this._currentGameTime};
            entity.AddComponents(gameTimeComponent);

            ref var gameEntityIdComponent = ref entity.AddComponent<EntityIdComponent>(ComponentLookupTypes.EntityIdComponentId);
            gameEntityIdComponent.EntityId = _currentGameEntity;
        }

It should do what you expect, however as I say this probably needs better documentation and potentially an improvement to the way you can create and add components.

grofit commented 4 years ago

I started a conv on the discord server about this topic as it touches on a few problem areas and I dont think I can easily wrangle the existing API to be able to let classes/structs mix nicely in the same method.

There was a lot of chat a while ago about moving entities to be structs, and some research was done into it, but it may be that rather than spending more time on allowing variations of adding classes/structs together look more into the entity changes then revisit this topic, as then the storage/interaction will be in 2 separate classes (well class and struct).

So feel free to drop in if you have any strong feelings on stuff.

Also worth pointing out, originally you never really bulk added components you did them one at a time, but the performance hit on group changes was quite large, so we added the AddComponents as a sort of performance improvement there as you could batch together multiple additions. So one thing that was originally going to be possible (potentially) with the newer entity separation was better batching/transaction-like handling, so you could just do lots of operations then just "commit" them (which would basically remove these pre-batched methods).