[WIP] Sectors #2975

Vizaxo · 2017-06-14T15:20:47Z

Sectors WIP PR

The first stage of the implementation of sectors as part of my GSOC project, for mentor review.

The EntityManager stores multiple entities both in the global scope, and the local scope. The method createSectorEntity is used to create an entity in the sector scope.

Also makes EntityCache an interface, to seperate the interface from the implementation.

GooeyHub · 2017-06-14T16:05:17Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2017-06-14T19:00:40Z

Hooray Jenkins reported success with all tests good!

skaldarnar

The current implementation adds a sector cache for entities (in addition to the global cache). This (hard-coded) separation implies that we duplicate the API to create and manage entities in different caches, i.e., global (backwards compatible methods) and sectors (indicated by method name). This makes some assumptions:

there is only one other entity cache, and entities live either in one or the other;
even if other scopes were implemented, they would still be statically known. This disallows for dynamic usage of different caches (e.g., sectors for different, unrelated context).

I'm wondering whether we can parameterize the existing methods with some kind of sector identifier (SectorRef) - method overloading should allow to keep it backwards compatible? Alternatively, the sector ref parameter could be an Optional<SectorRef> where Option.empty() defines that the entity is created in global scope...

skaldarnar · 2017-06-14T20:30:34Z

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

+import org.terasology.math.geom.Quat4f;
+import org.terasology.math.geom.Vector3f;
+
+/**


Please do a final iteration over the Javadoc in the end. Most of it is pretty self-documenting. However, try to avoid "empty" Javadoc where just the parameter names and such are auto-generated. A general summary of what the EntityCache actually is might be helpful.

Yep, I will. I didn't want to go documenting lots of stuff now when it's still rapidly changing.

skaldarnar · 2017-06-14T20:39:17Z

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

+
+    private static final Logger logger = LoggerFactory.getLogger(PojoEntityCache.class);
+
+    private Map<Long, BaseEntityRef> entityStore = new MapMaker().weakValues().concurrencyLevel(4).initialCapacity(1000).makeMap();


The mapping is specific to each cache instance, right? Can entities be in several caches at once, should this be allowed?
I'm not sure whether we need a way to transfer entities from one cache into another. Do you see a use case where this might be necessary?

I hadn't considered entities being in multiple caches. It might be useful (e.g. for entities near a sector boundary), but it could cause lots of problems with systems trying to update the same entity at the same time in two different caches (especially if we're thinking about using multi-threading to handle sectors in the future). So I think it's best to have each entity in a single cache.

I think that'll definitely be necessary when we have multiple sectors, so sectors can be moved or split (and entities can change sector if they move around, or find themselves interacting with lots of things outside of their sector).

skaldarnar · 2017-06-14T20:39:52Z

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

+
+    @Override
+    public void clear() {
+        //Todo: should also clear out ids from the EntityManager


This assumes that each entity is only contained in a single cache, is that correct?

skaldarnar · 2017-06-14T20:45:08Z

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

+    @Override
+    public EntityRef create(String prefabName, Vector3f position) {
+        if (prefabName != null && !prefabName.isEmpty()) {
+            Prefab prefab = entityManager.getPrefabManager().getPrefab(prefabName);


Above in line 103 you check for the prefab to not being null, but here the check is missing.

The create() methods below also assume that prefab is not null. Is the assumption (somewhat) safe?

Ah, I'll add the checks where they're needed.

Some of them are mostly safe because the associated create(String prefabName) does the null check, but I'll add the checks there as well, in case it is called with a null prefab from somewhere else (currently nowhere within the engine calls the create() methods with a Prefab directly, but it doesn't hurt to be safe).

skaldarnar · 2017-06-14T20:47:26Z

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

+            return;
+        }
+        EntityRef ref = createEntityRef(entityId);
+        if (entityManager.getEventSystem() != null) {


Introduce local field for entityManager.getEventSystem()

skaldarnar · 2017-06-14T20:49:47Z

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

+        long entityId = ref.getId();
+        entityStore.remove(entityId);
+        entityManager.remove(entityId);
+        if (ref instanceof PojoEntityRef) {


Can we get around this instanceof somehow? Should it be generally possible to invalidate EntityRef?

I don't see why you shouldn't be able to invalidate any EntityRef, even if that doesn't do anything for some implementations of it. I've added that in 15ca85c; I think that getting rid of the instanceof makes it a lot cleaner.

skaldarnar · 2017-06-14T20:52:40Z

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

+    @Override
+    public EntityRef createEntityWithId(long id, Iterable<Component> components) {
+        if (!entityManager.registerId(id)) {
+            return EntityRef.NULL;


This "silently" fails creating the entity with the specific id. Is there a chance for the consumer to know that the id was already taken? Does this happen often? Should we log a warning in this case?

An error gets logged by registerId. I'm not sure if it happens often or if it's a problem; this is only currently called for storage/serialization tasks, which already know the ID they're trying to create for from the entity they already had (so presumably they know that it's not invalid).

Most uses would only use the normal create methods, which will guarantee a valid ID. Perhaps a note should be added about this to the documentation?

skaldarnar · 2017-06-14T20:57:35Z

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

+    }
+
+    @Override
+    public EntityBuilder newBuilder(String prefabName) {


What is the cost of using a builder? Do reduce code duplication, does it make sense to implement create(prefab) in terms of the builder?

public EntityRef create(String prefabName) { newBuilder(prefabName).build() }

The builder class says that the whole entity can be setup before creation without any events being sent in-between. It also seems like a good way of creating lots of the same entity, so might be useful for spawning lots of cities, for example.

Implementing some of the create() methods in terms of the builder should reduce code duplication, especially the code to extract components from a prefab.

skaldarnar · 2017-06-14T20:58:12Z

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

+        return builder;
+    }
+
+    public Map<Long, BaseEntityRef> getEntityStore() {


Should we care about mutability of the map?

I hadn't considered that; I think an unmodifiable map would be more sensible, and then have a putEntity method so that the EntityManager can put entities into the map (from createEntityRef). But perhaps that should be changed too: maybe the EntityManager should just call the cache's createEntityRef? I think restricting external changes to the internal map would be an improvement; I'll look into the best way of doing it.

skaldarnar · 2017-06-14T21:02:42Z

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

    }

    @Override
+    //Todo: Depreciated, maybe remove? Not many uses


What should be used instead? This just creates a new EnityRef for an entity with the same set of components. Can we create a new builder from a give entity instead?

EntityManager says that it's depreciated in favour of EntityRef.copy(). Creating a builder from an existing entity is an interesting idea, especially if entities need to be duplicated multiple times (e.g. maybe for duplicating inventory items).

GooeyHub · 2017-06-15T17:45:26Z

Hooray Jenkins reported success with all tests good!

Also add the putEntity method, to allow the internal store to be modified by the entity manager.

This introduces a mapping between entityId and cache, so the EnityManager can properly store/retrieve components from the correct cache.

GooeyHub · 2017-06-16T16:35:06Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2017-06-17T00:31:32Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2017-06-18T14:53:43Z

Hooray Jenkins reported success with all tests good!

Add addPrefab methods to EntityBuilder so that calling functions don't have to expand the prefabs.

Add the getExistingEntity method to EntityCache and EntityManager, which will only return an entity if it already exists in the cache.

This allows searching of individual caches for whether an entity has a component, so the EntityManager doesn't have to access the internal store of the cache.

This removes a lot of code duplication, and makes the methods easier to read.

GooeyHub · 2017-06-20T23:35:41Z

Hooray Jenkins reported success with all tests good!

Also updates the implementation of said method to be a more concise stream-based operation.

GooeyHub · 2017-06-27T01:04:35Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2017-06-27T01:18:46Z

Hooray Jenkins reported success with all tests good!

msteiger

I find it very hard to review this, due to the large amounts of moved code. Does the actual implementation of the entity manager change? I would try to use other components as fields rather than creating an inheritance tree. Also see https://en.wikipedia.org/wiki/Composition_over_inheritance for details.

Sorry, I'm not very familiar with the GSOC proposal. What exactly are you aiming for in this PR?

There is a temp. storage that buffers entities that are created (in parallel) during world gen. and fed to the the EntityManager as soon as it world gen. is complete. Is it something similar?

msteiger · 2017-06-28T10:48:15Z

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

-     * @return
-     */
-    EntityRef create(Prefab prefab, Vector3f position, Quat4f rotation);
+    default EntityRef createSectorEntity() {


I think that null should not be returned, if possible. What about an empty sector entity?

Yep, you're right. EntityRef.NULL is the preferred way of returning a null entity, as it behaves sensibly by avoiding NullPointerExceptions.

I'm actually not sure why I implemented this as a default method; the only EntityManager in the engine is the Pojo one, for which I've implemented the method anyway (I expect my thinking was that I didn't want to have to go and implement it in multiple places while the interface is still changing). It's probably best to just remove the default implementation and leave it abstract.

Vizaxo · 2017-06-28T13:52:38Z

@msteiger Thanks for the feedback.

Does the actual implementation of the entity manager change?

The implementation of most of the methods is moved into the EntityCache, and I have been refactoring those methods to reduce duplicate code, but their functionality remains the same. The EntityManager now calls the methods on the global cache for lots of its methods by default, so the interface remains the same if you are just dealing with entities in the global scope.

Sorry, I'm not very familiar with the GSOC proposal. What exactly are you aiming for in this PR?

This issue might help explain it, and I've just posted a blog post (skip to "What is my project?" section) which should outline what I'm aiming for. This first stage will just be to get a basic sector/cache system working; doing things like multi-threading and multi-world will have to come later (after things like re-integrating Gestalt-EntitySystem have been worked out (as mentioned in that issue thread)).

If that's still unclear, please let me know.

I would try to use other components as fields rather than creating an inheritance tree. Also see https://en.wikipedia.org/wiki/Composition_over_inheritance for details.

The caches are implemented as fields; for example, the PojoEntityManager has a global cache, and a PojoSectorManager. The PojoSectorManager in turn has caches for each of its sectors (just one, at the moment).

But I did create an inheritance tree by making the EntityManager and SectorManager implement EntityCache. This is because the managers are caches in their own right, and can be used in place of individual caches when it's appropriate (so the SectorManager is the cache for all sector entities, and only the SectorManager needs to know about how each cache is represented internally).

I initially represented this with pure composition, but the interfaces had to be duplicated because many of the same operations have to be performed on the managers and caches (e.g. creating an entity in that cache).

I think this way of working works in this scenario, because the caches are separate fields, but the inheritance means the interfaces don't have to be duplicated. There might be a possibility for turning the inheritance of EntityManager -> LowLevelEntityManager -> EngineEntityManager (and the equivalent for caches) into a composition relationship, but I can't immediately see much advantage to that, as inheritance seems to fit their relationship well already.

Saying that, i tend to think about this kind of stuff in terms of inheritance, but can certainly see the advantage of favouring composition instead. I might be missing the point here or thinking about it in the wrong way, so I'd like to keep considering alternate ways of doing things if they lead to better code, even if it's not how I naturally think about these kind of problems. If you think this can be better represented without the inheritance in the interfaces, I'm all ears.

GooeyHub · 2017-06-28T18:58:41Z

Uh oh, something went wrong with the build. Need to check on that

GooeyHub · 2017-06-28T19:13:53Z

Hooray Jenkins reported success with all tests good!

Cervator · 2017-07-21T02:07:43Z

Closing as superseded by #3009 which has been merged into the v2 branch :-)

Vizaxo added 6 commits June 14, 2017 15:47

Implement sector-level entity stores

83cf26a

The EntityManager stores multiple entities both in the global scope, and the local scope. The method createSectorEntity is used to create an entity in the sector scope.

Rename EnittyStore to EntityCache (EntityStore already existed)

c4e6658

Also makes EntityCache an interface, to seperate the interface from the implementation.

Refactor EntityManager into EntityManager and EntityCache

4bf43c8

Implement clearing of EntityCache

ea66429

Add fixes and notes regarding EntityManager methods on sectors

25e5cd1

Reorder methods in PojoEntityManager to be in their original order

aae66e2

Make createEntityRef check all caches

b1ae5d1

skaldarnar requested a review from msteiger June 14, 2017 20:25

skaldarnar requested changes Jun 14, 2017

View reviewed changes

skaldarnar mentioned this pull request Jun 14, 2017

[Discussion] Sectors Implemenation/API #2976

Closed

Allow any EntityRef to have invalidate() called on it

15ca85c

Vizaxo added 3 commits June 16, 2017 12:43

Make getEntityStore return an unmodifiable map

4bf88fa

Also add the putEntity method, to allow the internal store to be modified by the entity manager.

Make the EntityManager store entity components in the correct cache

47511ee

This introduces a mapping between entityId and cache, so the EnityManager can properly store/retrieve components from the correct cache.

Make small code style improvements

0a537c6

Vizaxo added 2 commits June 17, 2017 00:29

Add checks to handle null prefabs

ae7a6d1

Add checks to treat null component lists as empty lists

1ee645b

Add the getAllEntities method to the EntityCache

28eb9dd

Vizaxo added 7 commits June 19, 2017 21:13

Implement getEntitiesWith method for EntityCache

5261096

Simplify creation of EntityBuilders with prefabs

93542c9

Add addPrefab methods to EntityBuilder so that calling functions don't have to expand the prefabs.

Add getExistingEntity method

fe0aa62

Add the getExistingEntity method to EntityCache and EntityManager, which will only return an entity if it already exists in the cache.

Add hasComponent method to EntityCache

5263091

This allows searching of individual caches for whether an entity has a component, so the EntityManager doesn't have to access the internal store of the cache.

Add the SectorManager and update PojoEntityManager to use it

3cb07c2

Refactor entity creation methods to use EntityBuilder

bd8225e

This removes a lot of code duplication, and makes the methods easier to read.

Refactor createEntityWithoutLifecycleEvents to use EntityBuilder

19b710b

Vizaxo added 4 commits June 24, 2017 17:36

Refactor EntityManager to extend EntityCache & add EngineEntityCache

6d92ec0

Refactor PojoSectorManager to implement EngineEntityCache

d73c2f5

Update getAllEntities method to iterate over all caches

74411b3

Update getEntitiesWith method to iterate over all caches

dc5ea5f

Also updates the implementation of said method to be a more concise stream-based operation.

Vizaxo force-pushed the sectors branch from 6c9e508 to dc5ea5f Compare June 27, 2017 01:09

msteiger reviewed Jun 28, 2017

View reviewed changes

Allow serialization of sector-scope entities

487ae2d

Vizaxo force-pushed the sectors branch from 4920341 to 487ae2d Compare June 28, 2017 19:04

Vizaxo mentioned this pull request Jul 16, 2017

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

Merged

Cervator closed this Jul 21, 2017


		private static final Logger logger = LoggerFactory.getLogger(PojoEntityCache.class);

		private Map<Long, BaseEntityRef> entityStore = new MapMaker().weakValues().concurrencyLevel(4).initialCapacity(1000).makeMap();

[WIP] Sectors #2975

[WIP] Sectors #2975

Conversation

Vizaxo commented Jun 14, 2017

Sectors WIP PR

GooeyHub commented Jun 14, 2017

GooeyHub commented Jun 14, 2017

skaldarnar left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooeyHub commented Jun 15, 2017

GooeyHub commented Jun 16, 2017

GooeyHub commented Jun 17, 2017

GooeyHub commented Jun 18, 2017

GooeyHub commented Jun 20, 2017

GooeyHub commented Jun 27, 2017

GooeyHub commented Jun 27, 2017

msteiger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vizaxo commented Jun 28, 2017

GooeyHub commented Jun 28, 2017

GooeyHub commented Jun 28, 2017

Cervator commented Jul 21, 2017