-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Sectors/pools: v2.0.0 branch #3009
Conversation
This allows entities to be split up into mulitple pools, rather than all being kept in one pool. It is the ground-work for sector-level entity stores, and in future could lead to multi-threaded systems.
This adds the SectorManager (and its implementation, PojoSectorManager), which implements sectors by having separate pools for sector-level entities.
Refactor various methods in PojoEntityPool to have nicer, more maintainable code, and refactor EntityManager to extend EntityPool to reduce API duplication.
Allow information about what scope an entity is in (either sector-scope or global) to be serialized and saved when the world is saved. No information about which sector the entity is in is stored; that is worked out by the SectorManager when the entity is loaded.
Consolidate multiple entity retrieval methods into getEntity(), and clean up many methods in PojoEntityManager.
Add the moveToPool() method, and add a tests for it. Also modify BaseEntityRef's setScope() to move the entity into the proper pool when the scope is changed.
Some small fixes to fix listComponents(), and make create(prefab, position) add a LocationComponent if the prefab doesn't have one (instead of silently discarding the location). Also moves some methods around in the EntityManager interfaces, to hide methods that aren't meant to be used in modules.
Update tests for moveToPool() and setScope() to be more comprehensive, and add some tests for PojoEntityPool. Make fixes in PojoEntityManager and PojoEntityPool to esure all tests pass, by properly assigning the entity to the new pool when moveToPool() is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good, just a few comments (and a tiny bug).
Maybe somebody else wants to give it a quick review, otherwise it's good to go.
|
||
private static Context context; | ||
private PojoEntityManager entityManager; | ||
//private Prefab prefab; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the commented line if not needed.
* @param prefab the prefab to add | ||
* @return whether the prefab was successfully added | ||
*/ | ||
public void addPrefab(Prefab prefab) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this method also have return type boolean
like the one above? At least the Javadoc suggests so and it would be consistent with the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I just copied the Javadoc without properly checking it.
It currently doesn't have anywhere it can fail, though I guess passing a null prefab might cause it to return false. The addPrefab(String) one only fails because a non-null string can fail to return a prefab (though, looking at it again, it will fail and not add an EntityInfoComponent on a nulll string, so changing one of them would make them consistent).
Currently adding a null prefab is fine, and is used to create an entity when you want to add a location, but not a prefab.
EntityRef create(Prefab prefab, Vector3f position); | ||
|
||
/** | ||
* @param prefab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add empty Javadoc, it serves no purpose (applies to the other code locations as well).
/** | ||
* @param prefab | ||
* @param position | ||
* @return A new entity, based on the given prefab, at the desired position |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think that documenting the return value is enough you can leave out the @param
lines. @emanuele3d what is your usual approach on this?
import java.util.Optional; | ||
|
||
/** | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you adding empty Javadoc all the time? Does the IDE or some tool (falsely) count this as "documented" then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, IDEA adds it automatically whenever I create a new class, I just never got around to actually writing the content. Now that the API is fairly well-defined I'll go through and add the necessary documentation.
This is the system behind the simulation of sector-scope entities. It allows sector-level entities to be used for simulation, even when the chunk they are in is not loaded.
This is easier to use without importing from the protobuf nested classes, and allows for more flexibility in the future for adding properties to different scopes.
Hooray Jenkins reported success with all tests good! |
Hooray Jenkins reported success with all tests good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave this a read plus some more testing. Very good work and high quality code :-)
I left a couple questions on Slack for later and found one potential bug which I'll document here in a separate comment + I made a card for it on the project board
Encountered the following - could reliably reproduce:
May be a consequence from an unrelated bug that none the less is able to affect the sector events / schedules.
|
Yeah, that's because I made sector entities not be Again, this goes back to whether to abandon |
This PR contains the work done so far on sectors/entity pools. There's nothing new in here that isn't in the following PRs:
#2975
Vizaxo#2
Vizaxo#3
Vizaxo#4
This PR just consolidates the work done so far, rebases it on top of the v2.0.0 branch (which is on par with develop, at the time of writing), and squashes the commits from 54 down to 8.
Also, any references to caches have been removed, so pool is used all the way through.
Summary of sectors/pools
This adds entity pools, which split up the storage of entities into multiple pools, rather than storing them all together. It implements sectors on top of that, which are a new scope for entities to allow better simulation when no players are close to the entity.
More details about the motivation behind sectors:
http://forum.terasology.org/threads/new-conceptual-layer-sector-plus-musings-on-multi-world-node.1420/
Discussion of sectors architecture:
#2976