Some important reasonings after I released Svelto ECS 2.6
Closed this issue · 3 comments
In the Survival Example I use just one EntityDescriptor to create three different types of enemies which is actually good because entities are specialized through data and not code. As matter of fact, this is how ECS should be actually used to take proper advantage of meaningful data iterations.
However, while refactoring the example using Svelto ECS 2.6, I had the chance to see the code under a different point of view and realize that some design decisions could take some advanced reasoning to be understood properly.
Specifically I am talking about what happened to me while refactoring the EnemyDeathEngine CheckIfDead method.
Inside the Svelto ECS database, EntityViewStruct and EntityStructs are not grouped per entity descriptor, this means that if multiple entity descriptors generate the same shared entity view, the array returned by QueryEntities will return all the entity views generated and not just the ones of that specific entity descriptor.
Because entity views can be shared over multiple EntityDescriptors, when an iteration needs to work on different array of different entity views, it's not guaranteed that these arrays can be indexed in the same way.
if I do
- entitiesDB.QueryEntities<EnemyEntityStruct>(out count);
- entitiesDB.QueryEntities<HealthEntityStruct>(out count);
- entitiesDB.QueryEntities<EnemyEntityViewStruct>(out count);
the HealthEntityStruct and the EnemyEntityStruct arrays won't have the same size and won't be indexable in the same way as HealthEntityStruct is generated both by the EnemyEntityDescriptor and the PlayerEntityDescriptor. This is why the QueryMappedEntities function exists, that is to have a way to find entity views through EGID.
On the other hand, since EnemyEntityStruct and EnemyEntityViewStruct are generated exclusively by the EnemyEntityDescriptor, I can guarantee by specification that the two arrays are indexable in the same way. Look at the code
IEnumerator CheckIfDead()
{
var enemyIterationInfo = new FasterList<EnemyIterationInfo>();
//this struct will allow use zero allocation lambdas. When c# 7 will be available in Unity
//this will be less awkward thanks to the new local functions feature
var valueTuple = new LambdaParameters {Item1 = enemyIterationInfo};
while (true)
{
//wait for enemies to be created
while (entitiesDB.HasAny<EnemyEntityStruct>() == false) yield return null;
valueTuple.Item2 = entitiesDB.QueryMappedEntities<EnemyEntityViewStruct>();
entitiesDB.ExecuteOnEntities(ref valueTuple,
(ref EnemyEntityStruct enemyStruct,
ref HealthEntityStruct healthEntityStruct,
ref LambdaParameters _parameters) => //this lambda doesn't capture any external variable
{
if (healthEntityStruct.dead != true) return;
uint index;
SetParametersForDeath(ref _parameters.Item2.entities(healthEntityStruct.ID, out index)[index]);
_parameters.Item1.Add(new EnemyIterationInfo(healthEntityStruct.ID,
(int) ECSGroups.EnemyDisabledGroups + (int) enemyStruct.enemyType));
});
...
} The QueryMappedEntities and the way I use the EGIDMapper to pass the parameters to the SetParametersForDeath is not intuitive neither is as fast as indexing directly the array of entity views.
However since performance is not an issue here I can decide to leave it as it is. Leaving as it is in the example could have the unwanted consequence to lead people to think that it's just fine to use this method, although it shouldn't be. Indexing directly should be preferred and in this case EnemyEntityViewStruct can actually be indexed directly. The code could become:
while (true)
{
//wait for enemies to be created
while (entitiesDB.HasAny<EnemyEntityStruct>() == false) yield return null;
//fetch the current EnemyEntityViewStruct array, avoid to do it inside the iteration
//to be faster
int count;
valueTuple.Item2 = entitiesDB.QueryEntities<EnemyEntityViewStruct>(out count);
//iterate over the EnemyEntityStruct and use the healthEntityStruct that are generated by the
//same entities. Use lambda without catching any external variable so that it won't allocate (which is
//very important! this will be less awkward when c# 7 is out)
entitiesDB.ExecuteOnEntities(ref valueTuple,
(ref EnemyEntityStruct enemyStruct,
ref HealthEntityStruct healthEntityStruct,
ref LambdaParameters _parameters, int iterationIndex) =>
{
if (healthEntityStruct.dead == false) return;
SetParametersForDeath(ref _parameters.Item2[iterationIndex]);
_parameters.Item1.Add(new EnemyIterationInfo(healthEntityStruct.ID,
(int) ECSGroups.EnemyDisabledGroups + (int) enemyStruct.enemyType));
});
...
yield return null;
}For example in ApplyingDamageToTargetEntitiesEngine ApplyDamage function, DamageableEntityStruct and HealthEntityStruct can be iterated with the same for-loop, because they are generated by the same entity descriptors and there is no entity descriptor that generates just one or the other.
ExecuteOnEntities make the code more awkward, especially with version of c# prior 7 as local functions cannot be exploited. However ExecuteOnEntities can ensure that Swap and Remove won't happen inside the iteration by mistake.
To conclude, I could change the database and introduce functions like entitiesDB.QueryEntities<EnemyEntityDescriptor, EnemyEntityViewStruct> and entitiesDB.QueryEntities<EnemyEntityDescriptor, HealthEntityStruct> so that the returning buffers are guaranteed to be indexable in the same way. It would make also the coder intention clearer, so it wouldn't be bad.
On the other hand it would make engines like ApplyingDamageToTargetEntitiesEngine slower, but probably not as slower as mapping an index for each EGID like it happens in the specialized engines.
If specialized engines are more common than abstract ones it could make sense to move toward this direction.
Since it's a big decision as it would break the current existing Svelto application, I need to know your point of view. If I change it, the QueryEntities function coudn't be used anymore without an EntityDescriptor parameter and for abstract engines it would be necessary to use ExecuteOnEntities to delegate to the framework the task to find the arrays that contains the entity view to iterate on
I have been thinking about the problem a bit and there are two things currently bothering me a lot. Relying on the fact that the entity views are generated by the same descriptors can cause several problems if an entity view will be used in an another entity descriptor. It basically introduces bugs without even knowing why.
The other thing that bothers me is the non safe Swap and Remove inside an iteration. I think the framework must handle this case to avoid the adoption of patterns and boiler plate code to solve the same issue.
What unity calls Archetype, I call EntityDescriptor. In my framework, entity must be build explicitly using BuildEntity. An entity descriptor can generate several components. Let's say that E1 are instances of the EntityDescriptor 1 and E2 are instances of the EntityDescriptor 2. Let's say that E1 generates component 1 and E2 generates component 1 and 2. In memory they are laid like this:
array of component 1 = [E1, E1, E1, E1, E1, E2, E2, E2]
array of component 2 = [E2, E2, E2]
I designed like this initially because I thought the more elements there are in an array, the better it is. This is actually true for abstract systems. What I am going to explain can result confusing if we see under the Unity ECS point of view, but it was quite natural for us so far. If two separate entity descriptors generate the same entity components, I don't need to know the entity structure. I can iterate over the components in this way:
int count;
var entities = entitiesDB.QueryEntities<DamageableEntityStruct>(out count);
var healths = entitiesDB.QueryEntities<HealthEntityStruct>(out count);
for (int i = 0; i < count; i++)
{
if (entities[i].damageInfo.damagePerShot > 0)
{
healths[i].currentHealth -= entities[i].damageInfo.damagePerShot;
entities[i].damageInfo.damagePerShot = 0;
entities[i].damaged = true;
}
else
entities[i].damaged = false;
}however I realized that this system can't work well in practice, because if a new EntityDescriptor generates just HealthEntityStruct, than the arrays returned won't have the same size. I would break this code and I would need to come back to this and fix it.
I have a solution that we use in practice to make it works always, but it involves mapped indices and when it's used it doesn't feel too awkward, but mapping indices is surely slower and less data would fit in the cache (because of the indices map).
if E1 was generating HealthEntityStruct
and E2 was generating
DamageableEntityStruct and
HealthEntityStruct
I wouldn't be able to query the arrays directly, but I would need to use something like:
var entities = entitiesDB.QueryEntities<DamageableEntityStruct>(out count);
var healths = entitiesDB.QueryMappedEntities<HealthEntityStruct>();
for (int i = 0; i < count; i++)
{
if (entities[i].damageInfo.damagePerShot > 0)
{
uint healthIndex;
healths.entities(entities[i].ID, out healthIndex)[healthIndex].currentHealth -= entities[i].damageInfo.damagePerShot;
entities[i].damageInfo.damagePerShot = 0;
entities[i].damaged = true;
}
else
entities[i].damaged = false;
}Knowing to use DamageableEntityStruct to pickup only the E2 components through a map is too much knowledge. It may not look too awkward, but the user must know too much and I realized it's not intuitive.
So I am thinking to move toward something similar to the UnityECS layout, it will break the cache for the global iteration, but at least it would be less awkward and error prone.
I can close the issue.
I don't know how many of you understood the problem or even if my explanation was clear enough, but the problem is now solved. I don't need to change the memory layout.
Groups were already the solution all along. The reason why I couldn't see it is because I wasn't forcing the user to set a group. The StandardGroup was very confusing anyway, so it was a great decision overall.
Better than the division in Archetypes that UnityECS does, you can decide how to split the entities arrays through the use of groups. There is more flexibility in doing so, so it's up to you how you want to organize the memory layout through groups.
All you have to know is that entity views arrays are split according the groups. In this way you will need much less the use of mapping as you will know exactly which entities and how many of them are in each group.
Example, using groups:
Group 1:
array of component 1 = [E1, E1, E1, E1, E1]
Group 2:
array of component 1 = [E2, E2, E2]
array of component 2 = [E2, E2, E2]
the array and index will now always coincide.
I can close the issue now. Making the swap safe is considered another issue.