Bug fixes and method cleanup #2

Vizaxo · 2017-07-05T01:43:35Z

This PR fixes some bugs with the sectors/pools implementation by consolidating the multiple entity retrieval methods into just one method, getEntity.

The existence of multiple methods to do the same thing led to code reuse, and some of the code wasn't properly changed in all locations. This caused a problem where the entity wasn't assigned a cache properly if it was created with one of the methods, leading to bugs in serialization and iteration. This PR fixes that problem, and reorganises the methods to help reduce similar bugs in the future.

The methods createEntityRef, createEntityRefWithId, getExistingEntity, getEntityRef, and getEntity all did the same job of returning the EntityRef corresponding to a given id. These are all consolidated into one method, getEntity, which I believe has the most descriptive name (the presence of 'create' in the name was confusing, as these methods aren't used to create entities. They are used to create an EntityRef corresponding to an id, but that should not have any effect on the method caller, so I believe it's best to leave it out of the name).

Entities were not being properly assigned to pools, and the pools were not being properly iterated over. This meant that serialization and iteration over entities that aren't in the global pool didn't work properly.

Combine createEntityRefWithId and createEntityRef into getEntityRef, which is a more accurate name for what the functions do.

Make the PojoEntityManager's createEntityWithoutLifecycleEvents methods call the global pool's methods, instead of duplicating the functionality.

The getEntityPool method makes it easier to get the pool of an entity, and debug when the entity doesn't have an assigned pool.

The EntityManager had a getEntity method, but also inherited the getEntityRef method from the EntityPool. Both of these methods did the same thing, so they weren't both needed on the EntityManager. This change renames EntityPool's getEntityRef to getEntity, and merges the two methods in EntityManager (so only getEntity remains).

EntityPool's method getExistingEntity was not needed, as getEntity did the same thing (but still worked even if there wasn't an EntityRef already saved). This commit removes getExistingEntity, and replaces calls to it with calls to getEntity.

The createEntity method of PojoEntityManager is now not used anywhere, so it can be removed. Its functionality has been replaced by the standard create methods.

PojoEntityManager's createEntityRef method was duplicating functionality from the EntityPool's getEntity method. It also meant that there were multiple ways that EntityRefs were created, so code had to be updated in multiple places. This led to bugs, such as entities not being assigned to a pool when they are created, because the code wasn't updated in all places. This commit removes the createEntityRef method, and replaecs all references to it with the getEntity method.

The internal EntityIterable class is no longer used, as its functionality can be simplified to using a lambda to return an instance of the EntityIterator class.

skaldarnar · 2017-07-05T19:34:19Z

engine/src/main/java/org/terasology/entitySystem/entity/EntityPool.java

     *
     * @param id
-     * @return The entityRef for the given id
+     * @return the {@link EntityRef}, if it exists; {@link EntityRef#NULL} otherwise


skaldarnar · 2017-07-05T19:48:43Z

engine/src/main/java/org/terasology/entitySystem/entity/internal/PojoEntityManager.java

@@ -467,7 +407,8 @@ public boolean isActiveEntity(long id) {
    @Override
    //Todo: implement iterating over multiple pools
    public Iterable<Component> iterateComponents(long entityId) {
-        return globalPool.getComponentStore().iterateComponents(entityId);
+        return Iterables.concat(globalPool.getComponentStore().iterateComponents(entityId),
+                sectorManager.getComponentStore().iterateComponents(entityId));


This is basically an "either ... or ..." in case an entity is either in sector or global scope, is that correct? If, for some reason, an entity ends up to be in both, this will duplicate all the components, won't it?

Ah, this was just the general form I was using to combine iterators. It works because the entity is only in one pool at a time, but it would be better to do it by just finding out which pool the entity is in and iterating over it in that pool. That would be more efficient, communicate better what's actually happening, and probably be more resistant to bugs.

There's currently no way of an entity ending up in multiple pools (and I don't think there should be), so I don't think this would actually be a problem in terms of program correctness, but it's certainly not the best way to do it.

skaldarnar · 2017-07-05T19:49:37Z

engine/src/main/java/org/terasology/entitySystem/entity/internal/PojoEntityManager.java

@@ -575,18 +504,13 @@ public void notifyComponentRemovalAndEntityDestruction(long entityId, EntityRef
    @Override
    //Todo: be able to save components for entities in any pool


Is this TODO item fixed with the new implementation? it looks like it will select the correct entity pool for the given id.

Yep, this is fixed. I need to go back and review my comments and TODOs and stuff to make sure everything's up to date.

skaldarnar · 2017-07-05T19:53:12Z

engine/src/main/java/org/terasology/entitySystem/entity/internal/PojoEntityManager.java

+    private EngineEntityPool getEntityPool(long id) {
+        EngineEntityPool pool = poolMap.get(id);
+        if (pool == null) {
+                logger.error("Entity {} doesn't have an assigned pool", id);


At this point, should the stray entity just be assigned to the global pool? What is the behavior here if I query for the pool of a non-existing entity id? It looks like I get the global pool, which looks like the a valid value from which I can retrieve the entity as a consumer. Thus, it either

should return null in case of error, which would lead to null checks everywhere, ...

assign the stray entity to the global pool before returning

I kept this in because it was very helpful to debug when I was having issues with entities not being put into the pools correctly.

With the latest round of fixes this shouldn't actually occur, but I think providing a sensible fallback in case of error is worthwhile.

Querying for the pool of a non-existing id isn't something I thought about; it would currently return the global pool, but should probably check for that and return some sort of null value.

We could introduce an EntityPool.NULL, analogous to the EntityRef.NULL. This would provide sensible defaults in the case of an error (i.e. the pool appears empty for any methods calling it).

I'm not sure about the case of an entity that isn't assigned to a pool. It shouldn't currently be possible to have that situation (I have just realised that I should probably make createEntity() protected, rather than public, to stop this kind of stuff). But I think assigning the stray entity to the global pool in case of error is a safe bet.

Alternatively, you can use an Optional<Pool> as return value (easier to deal with than with null types).

skaldarnar · 2017-07-05T19:55:28Z

engine/src/main/java/org/terasology/entitySystem/entity/internal/PojoEntityManager.java

+                return globalPool;
+        } else {
+            return pool;
+        }
    }

    protected void assignToPool(long entityId, EngineEntityPool pool) {


Do we also want to have a moveToPool, or how will the different contexts be modelled (switching from chunk to sector scope and vice versa)

Yep, I think that will be needed to move entities between scopes, and to allow the sector manager to split up/join the sectors.

Should it send an event when entities are moved between pools?

Hmm, I need to think about that... It probably should so that system can react on this change. It might be especially interesting if some systems wants to collect some resources or information just before the entity gets shifted from chunk scope to sector scope.

Vizaxo · 2017-07-23T17:00:28Z

Merged in MovingBlocks#3009

Vizaxo added 9 commits July 4, 2017 02:03

Fix bugs with serialization and iterating over entities

e512118

Entities were not being properly assigned to pools, and the pools were not being properly iterated over. This meant that serialization and iteration over entities that aren't in the global pool didn't work properly.

Clean up EntityPool methods

8ae26be

Combine createEntityRefWithId and createEntityRef into getEntityRef, which is a more accurate name for what the functions do.

Make EM's create w/o lifecycle methods call the pool's methods

12242e8

Make the PojoEntityManager's createEntityWithoutLifecycleEvents methods call the global pool's methods, instead of duplicating the functionality.

Add getEntityPool method to PojoEntityManager

a76f99d

The getEntityPool method makes it easier to get the pool of an entity, and debug when the entity doesn't have an assigned pool.

Remove now unused method createEntity

35f0d4f

The createEntity method of PojoEntityManager is now not used anywhere, so it can be removed. Its functionality has been replaced by the standard create methods.

Remove internal EntityIterable class from PojoEntityManager

fe8d5be

The internal EntityIterable class is no longer used, as its functionality can be simplified to using a lambda to return an instance of the EntityIterator class.

skaldarnar approved these changes Jul 5, 2017

View reviewed changes

This was referenced Jul 12, 2017

Sector/more fixes #3

Closed

[WIP] Sectors/pools: v2.0.0 branch MovingBlocks/Terasology#3009

Merged

Vizaxo closed this Jul 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixes and method cleanup #2

Bug fixes and method cleanup #2

Vizaxo commented Jul 5, 2017

skaldarnar Jul 5, 2017

skaldarnar Jul 5, 2017

Vizaxo Jul 5, 2017

skaldarnar Jul 5, 2017

Vizaxo Jul 5, 2017

skaldarnar Jul 5, 2017

Vizaxo Jul 5, 2017

skaldarnar Jul 5, 2017

skaldarnar Jul 5, 2017

Vizaxo Jul 5, 2017 •

edited

Loading

skaldarnar Jul 5, 2017

Vizaxo commented Jul 23, 2017

		@@ -575,18 +504,13 @@ public void notifyComponentRemovalAndEntityDestruction(long entityId, EntityRef
		@Override
		//Todo: be able to save components for entities in any pool

Bug fixes and method cleanup #2

Bug fixes and method cleanup #2

Conversation

Vizaxo commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vizaxo Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vizaxo commented Jul 23, 2017

Vizaxo Jul 5, 2017 •

edited

Loading