-
Notifications
You must be signed in to change notification settings - Fork 13
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
test: add SettlementEntityManagerTest for placeParcel #107
Conversation
9cafd41
to
ccbe84e
Compare
ccbe84e
to
86ecc9c
Compare
Test set-up has been slow going because everything really, really wants to be configured by prefabs, and I don't know how to add a prefab only for tests. See related discussion in #gestalt. |
8ec6e0a
to
100ea32
Compare
100ea32
to
69b1234
Compare
After some fiddling to make this not dependent on other work-in-progress, this is something like a test case that runs. It doesn't pass, but it's starting to look like something that could pass. The changes to the non-test classes are intended to be small refactorings to make the tests a little easier to write, plus some things CheckStyle complained about along the way. |
this.coords.set(other.coords); | ||
this.radius = other.radius; |
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.
whoops, where did we forget this 🙈
An implementation of the same thing from engine utilities.
… for easier debugging
ResourceFacetComponent resourceFacetComponent = region.getComponent(ResourceFacetComponent.class); | ||
if (roughnessFacetComponent == null) { | ||
logger.error("No RoughnessFacetComponent found for region"); | ||
static class ParcelLocationValidator { |
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.
TODO: javadoc
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.
Code-wise this looks fine to me. I didn't see anything that sparked specific concerns.
I'll give this a go in-game before approving.
entityStore.addComponent(roughnessFacetComponent); | ||
entityStore.addComponent(resourceFacetComponent); | ||
entityStore.addComponent(treeFacetComponent); | ||
|
||
// FIXME: is TreeFacet 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.
I would not think so. CoreWorlds
as part of the core modules should always be enabled and has a TreeFacet
, so I would guess if all else fails, that one should always be there, should it?
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 don't know about that.
DynamicCities/src/main/java/org/terasology/dynamicCities/world/testbench/FlatFacetedWorld.java
Lines 36 to 46 in 4ec5c18
protected WorldBuilder createWorld() { | |
return new WorldBuilder(worldGeneratorPluginLibrary) | |
.addProvider(new FlatSurfaceHeightProvider(SURFACE_HEIGHT)) | |
.addProvider(new SeaLevelProvider(SEA_LEVEL)) | |
.addProvider(new ConstantBiomeProvider(BIOME)) | |
.addProvider(new RoughnessProvider()) | |
.addProvider(new SiteFacetProvider()) | |
.addProvider(new SettlementFacetProvider()) | |
.addProvider(new ResourceProvider()) | |
.addEntities(new RegionEntityProvider()) | |
.addRasterizer(new SolidRasterizer()) |
Can entity providers use the same @Requires
annotations as FacetProviders can?
If so, then this EntityProvider could declare that it does depend on having a TreeFacet. (Once MovingBlocks/Terasology#4924 is fixed.)
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.
You're absolutely right, only because a TreeFacet exists in at least one of the modules, the world generator doesn't need to use it.
I don't know whether or not they can 🤷
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.
Found a city in-game, different kinds of buildings spawn fine, checks run through - good enough for me 👍
Things to look for in review:
Should we try to work around nulls here, or should these be errors? Is there someplace higher-level that should enforce its requirements before it gets here?
Let me know if you want any of the checkstyle fixes or other refactorings split in to other PRs to reduce the noise in this one.