EntityCollection initialization bug

[VincentToups]

Hi Paldepind,

I seem to have stumbled across a bug in the way entity collections are initialized. Here is some code that fails:

var k = new Kran();
console.log(k)

var cPlayer = k.component(function(){
  this.player = true;
})

var cHealth = k.component(function(amount){
  this.amount = amount;
})

var ent = k.entity()
  .add(cPlayer)
  .add(cHealth, 4);

k.system({
  component:[cPlayer, cHealth],
  every:function(a,b){ return undefined }
})

console.log("should get one: ",k.getEntityCollection([cPlayer, cHealth]).ents)
k.run("all")

var ent2 = k.entity()
  .add(cPlayer)
  .add(cHealth, 4);

console.log({'cHealth':ent.get(cHealth), 'cPlayer':ent.get(cPlayer)})
console.log({'cHealth':ent2.get(cHealth), 'cPlayer':ent2.get(cPlayer)})
console.log("should get two: ",k.getEntityCollection([cPlayer, cHealth]).ents)

The behavior is the same in node.js and the browser (Chrome) and the error messages are more or less the same:

{ components: [],
  systems: [],
  systemGroups: { all: { members: [] } },
  entityCollections: {} }
should get one:  { head: null, tail: null, length: 0 }

/home/procsaos/src/web/kranBug/node_modules/kran.js:140
      this.collection.forEachWithComps(this.every, this, ev)
                      ^
TypeError: Cannot call method 'forEachWithComps' of undefined
    at Object.runSystem [as run] (/home/procsaos/src/web/kranBug/node_modules/kran.js:140:23)
    at /home/procsaos/src/web/kranBug/node_modules/kran.js:126:14
    at Array.forEach (native)
    at Kran.run (/home/procsaos/src/web/kranBug/node_modules/kran.js:125:38)
    at Object.<anonymous> (/home/procsaos/src/web/kranBug/js-scratch.js:25:3)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)

That is, first of all, the initial getEntityCollection call should give us an entity collection containing one member. And then the line which tries to run the system produces an error.

If you run the lines after the error, you will see that both entities have the components they expect, but for some reason they do not appear in the entity collection. I am a bit surprised to discover this since I have a modest game engine running. I had not requently depended on the functionality of getEntityCollection until some recent modifications, though, and so maybe that uncovered the bug.

I am reading through the source now, but aside from a hunch that there is some old code in runSystem and that there is something funny going on with the cached entity collections, I am at a loss.

[VincentToups]

Further information: the two problems may be unrelated. The first problem has to do with the fact that the first time an entityColletion is requested it must be created, and kran does not populate that initial entity collection with entities which belong in it. Hence, to get the correct behavior one must request the entity collection once before any entities are created.

I believe this leads to the possible bug that some entities can be created but never belong to any entity collection, if they were created before such a collection exists.

This doesn't seem to effect the entities that systems run on, so I gather that that part of Kran doesn't cache the entities which belong in the same way. Not sure, however.

[paldepind]

Thanks for reporting this bug. I'm looking at it :)

[paldepind]

First of all there is a spelling mistake in your code. The object passed to k.system should have a components property – not a component property (I found a spelling mistake in the documentation calling the property component that I've fixed).

Secondly. Due to the way Kran works entities are only added to entity collections when an entity is getting a component added and after that satisfies an entity collection that includes the added component. When you call add on an entity something like this happens

for all entity collections containing the added component:
    if entity satisfies collection criteria:
        add entity to entity collection

This is an efficient way to add entities to entity collections since only the collections containing the specific component are considered. But this also means that if the entity collection doesn't exist yet when the component is added, the entity won't be properly included in the entity collection.

As you've figured out the first time an entity collection is requested it is created if not already existing. When systems are created they properly request the entity collection matching their components to ensure that it exists. The intended way to use Kran is to

1. Create all components
2. Create all systems and request and store all needed entity collections
3. Create entities.

If one works in this way the above described problem is a non issue. But Kran should properly throw an error if a system is created after entities have been created to enforce this desired use case (what do you think)?

Of course adding existing entities to newly created entity collections is possible but doing so would be inefficient (one would have to loop over all entities) and thus I've decided to only support the above mentioned more efficient order of doing things. But if a proper use case for adding systems late if shown I will consider adding the feature.

This should be added to the documentation and better error reporting should be added to Kran. Once that is done I think this issue can be closed.

Btw, to fix your code change component to components and move the system definition above the entity initialization.

[VincentToups]

Got it. I've modified my code to do as you suggest. Incidentally, you mentioned a desire to document the library. I'd be happy to do it for you, if you'd accept contributions. I think Kran is really great and I've created a lot of stuff with it. More people should write small, concise and understandable libraries which do one thing very well.

[paldepind]

I'd be very grateful for any kind of contributions :)

I'm happy that you like Kran and it's motivating to me so I'll properly commit some things soonish.

One feature I'd like to add is support for some kind of entity models. Adding components is the most expensive operation in Kran since at that point Kran has to figure out what entity collections the entity should be inserted into. But if one creates a lot of very similar entities the process could be optimized. For instance if one has a function like this:

createEnemy = function() {
    return k.entity().add(health, 100).add(damage, 10).add(cleverAi).add(speed, 40).add(etc, somevar);
}

Then Kran will currently do a bunch of processing every time createEnemy is called. But I'm considering adding a way to precompute what entity systems an entity will end up in. Like this:

// Kran figures out what entity collection an entity with the given components should belong to
var enemyModel = k.entityModel([health, damage, cleverAi, speed, etc])
createEnemy = function() {
    // Add this point Kran needs to do no calculations and creating the entity leads to no overhead
    return k.entity(enemyModel).add(health, 100).add(damage, 10).add(cleverAi).add(speed, 40).add(etc, somevar);
}

The bad thing about this feature is that it complicates Kran, but creating many entities with the same components is quite common and in some cases the performance improvements could be very beneficial.

What do you think?

[VincentToups]

So far, performance hasn't been an issue for me, so I can't say whether such optimization would be worth it, but the idea seems good in general.

Incidentally, it occurs to me that the system method could initialize the entity collection indicated by the components argument and that would prevent many scenarios where entities are created before the first query of an entity collection, since frequently one called getEntityCollection with a list of components which matches some declared system.

One could even get rid of getEntityCollection, and replace it with getSystemEntities, where systems could be referenced by name or id, and that would force people to declare systems for any entity collection they anticipate using, and hence initialized the appropriate entityCollections. Since it is clear from the design of Kran that Components and Systems need to be initialized before any entities are created, this design would produce an interface which would prevent people from encountering the bug/behavior I encountered. It seems like a more consistent design than exposing entityCollections which may not always return all entities which are expected.

[paldepind]

Incidentally, it occurs to me that the system method could initialize the entity collection indicated by the components argument and that would prevent many scenarios where entities are created before the first query of an entity collection, since frequently one called getEntityCollection with a list of components which matches some declared system.

That is already the case. When a system is initialized it calls getEntityCollection to retrieve the collection it needs.

I actually considered doing something similar to what you suggests. The system objects do have a collection property and Kran.system could easily return the system object. But that would also expose a bunch of things on the systems that users have no need to access. Your idea of having a getSystemEntities would solve that though.

I think the main reason why I decided against it was that today there is no way (or need) to identify a specific system. A getSystemEntities function would need a system identifier. I kinda like that you don't need to hold on to them and simply organize them in groups. Also if a user needs an entity collection that is not used by any system one would have to create a dummy system just for that which is slightly ugly.

But you are indeed right that getEntityCollection can be misleading in the way it is exposed!

[VincentToups]

"Also if a user needs an entity collection that is not used by any system one would have to create a dummy system just for that which is slightly ugly."

Yes, but in the current design, in order to be totally safe, one has to call .getEntityCollection once before any entities are created anyway, essentially declaring an interest in that collection, in order to ensure that she gets all the entities she expects when she invokes it later, during processing.

One simpler solution is to expose a method .declareEntityCollection which just calls .getEntityCollection but at least implies with its name that entityCollections must be forward declared.

A slightly more complex possibility which still preserves the anonymous nature of systems is that kran.system(...) could return an anonymous function:

function(){
  return kran.getEntityCollection(componentsPassedIntoSystem);
}

One could then remove the .getEntityCollection interface, preserve the anonymity of systems, but indicate clearly by the API that the entity collections must be pre-declared. Since the behavior of .getEntityCollection is slightly confusing, this might be a cleaner API, even if the user must create dummy systems for certain entity collections: she has to do so already via initial calls to .getEntityCollection anyway, so why not?

[paldepind]

Yes, but in the current design, in order to be totally safe, one has to call .getEntityCollection once before any entities are created anyway, essentially declaring an interest in that collection, in order to ensure that she gets all the entities she expects when she invokes it later, during processing.

The intention is that getEntityCollection should only be should only be called during the initialization phase of the game.

var fireTowerCollection = k.getEntityCollection([position, height, shootingRange, fireArrows])

And then the retrieved collection is simply used later on instead of calling getEntityCollection again. I realize that my own example game actually calls getEntityCollection in-game. But the function is not optimized for performance so calling it early and saving the collection is best practic and not wasteful.

Renaming getEntityCollection to declareEntityCollection is actually a great idea! I'm seriously considering that.

A slightly more complex possibility which still preserves the anonymous nature of systems is that kran.system(...) could return an anonymous function:

Why not just return the entity collection that the system uses directly instead? Still, I think a function that creates systems should if anything return the created system. Returning anything else is odd.

[VincentToups]

"The intention is that getEntityCollection should only be should only be called during the initialization phase of the game."

I see - this could be made clearer in the documentation, because I use getEntityCollection all over the place in my code. I didn't realize that it was an object that would be updated with new entities as they are created and deleted, and hence doesn't need to be repeatedly fetched.

In that case, I guess just better documentation would solve most of these issues. Sorry for all the trouble! Doesn't getEntityCollection cache its results anyway, so it isn't THAT expensive to call it frequently, its just a bit weird.

[paldepind]

Yes, I agree with you. Actually it's not mentioned anywhere in the documentation and my own example game doesn't do it either >.<

I'm still considering renaming it declareEntityCollection. Do you still think that is a good idea?

It caches the result in that if the entity collection already exists a new is not created. But consider the problem: getEntityCollection is getting an array of IDs and it needs to figure out if this specific combination of IDs has been seen before. There is no overhead free way to do that! I do it like this:

var key = comps.slice(0).sort().toString()

This neatly generates a unique key that can be used to identify the entity collection. But it copies an array (which creates garbage), has to sort the array and then creates a string (which creates garbage). It also takes an array as its parameter (which also creates garbage).

When creating Kran I carefully considered what code should be run at initialization time and what code should be run in-game. The first part is optimized for convenience and expressiveness while the latter is optimized for performance and low garbage creation.

But obviously for many games the overhead of calling getEntityCollection at run time is totally negligible and not in any way an issue (except for the one you found above).

[VincentToups]

A clear API would be createEntityCollection(components) which returns an opaque ID and getEntityCollection which takes that ID and returns the collection object. The ID could be an integer and so looking up the collection in an array would be fast.

You could then remove the caching functionality from getEntityCollection altogether, and throw an error if the entity collection id passed in doesn't exist. In fact, you could remove getEntityCollection and instead expose methods like forEachEntityIn(collectionID, block), getEntities(collectionId, filterFun) etc.

That seems like a clear API which makes rather explicit that aspects of the entity management system are made opaque for the purposes of performance optimization.