Sector/more fixes #3
Conversation
Cleans up some things in PojoEntityManager, such as updating TODO comments and making sure more things work on all pools where appropriate.
This makes error handling easier, as an empty optional can be returned in the event that the pool is not found, or an invalid id is passed in.
Convert notifyComponentRemovalAndEntityDestruction to operate on all stores, and make the notify methods protected, rather than public.
getPool is more concise and readable than getEntityPool, and the extra information that the pool is for entities is unnecessary in the context of the entity manager.
Add moveToPool method, and add a test for it. This also includes adding the helper methods remove and contains to the pool.
This removes a naming conflict with EngineEntityPool's remove method, and makes it clearer what the method does.
Improves setScope so that the same ref is used, rather than a new one being generated and having to be retrieved by the user.
The methods getGlobalPool, getSectorManager, and moveToPool are only intended for use within the engine, so they are moved to LowLevelEntityManager.
Previously, if create was called with a null prefab, any location data provided was ignored. This commit adds a location component to the entity even if it doesn't have a prefab.
| /** | ||
| * Does this pool contain the given entity? | ||
| * | ||
| * @param id the id to search for |
| @@ -34,6 +36,7 @@ | |||
| public abstract class BaseEntityRef extends EntityRef { | |||
|
|
|||
| protected LowLevelEntityManager entityManager; | |||
| private Logger logger = LoggerFactory.getLogger(BaseEntityRef.class); | |||
There was a problem hiding this comment.
should be private static final Logger logger = ...
| * @param id the id of the entity to remove | ||
| * @return an optional {@link BaseEntityRef}, containing the removed entity | ||
| */ | ||
| Optional<BaseEntityRef> remove(long id); |
There was a problem hiding this comment.
This is internal API that needs to be handled with care as I understand it. How big is the potential risk for leaking entity refs here, i.e., remove them from the pool but don't put them anywhere else?
There was a problem hiding this comment.
Yes, that would be a problem. If this is called without caution, all of the components will be removed from the entity, but the entity will still stay registered and the ref could be re-created.
The method is intended for use only by moveToPool(), but I had to put it in the interface because the entity manager currently deals in terms of EngineEntityPools, rather than PojoEntityPools. This makes it more general and means that an alternative implementation of pool could be easily substituted, however it means that any methods like this have to be included in the interface.
I don't think that's a huge problem, but it's probably worth adding a note warning against improper use (unless you can think of a better way of doing it).
|
Merged in MovingBlocks#3009 |
This contains fixes and improvements discussed in #2, such as making
getEntityPoolreturn anOptional, and addingmoveToPool.There are also some other small updates, such as renaming methods and adding more documentation.
Along with adding
moveToPool, I updatedsetScopeto automatically transfer the entities to the correct pool. It is worth noting that due to module permissions (of theEntity.Scopeenum inEntityData) this can't currently be used in module code. I imagine modules will want to use it, so either changing the permissions on the enum (or moving it to somewhere else) (I haven't looked into module permissions, so I'm not sure what this involves), or implementing different methods (e.g.setGlobalScope(),setSectorScope()), is probably worthwhile.I've also added tests for these new methods, which helped find some initial bugs (e.g. initially, the ref was destroyed, and you had to get a new ref with the id, but now the ref stays pointing to the entity). The setup code for
BaseEntityRefTestwas mostly copied fromPojoEntityManagerTest. I'm not sure if this is the best way to do things, and it's possible that bits of it can be cut or changed to better suit this class' needs, but the testing works fine.