-
Notifications
You must be signed in to change notification settings - Fork 14
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: Convert to MTEExtension #94
Conversation
…sion # Conflicts: # src/test/java/regions/RegionEntitiesTest.java
use parameterized test to help clarify in the test runner which things are passing
Results improve slightly, at the cost of a much much longer runtime, by using IsolatedMTEExtension. Which I find puzzling in itself, because the code executed in the tests looks like it should all be read-only operations? So why would they benefit from increased isolation? |
Also: The Except there is a failing test saying the cell at (98, -124) is loaded when it shouldn't be? |
This led to some MTE work for untangling interactions between test cases: Terasology/ModuleTestingEnvironment#64 also: a lot of the code under test here in RegionEntitesTest changed for JOML in #86. If these tests were disabled (or just ignored) at the time, something might have slipped through then. |
…EntityRefs just once
@@ -75,18 +72,18 @@ public void postBegin() { | |||
} | |||
regionEntitiesComponent = new RegionEntitiesComponent(gridSize); | |||
regionStoreEntity = entityManager.create(regionEntitiesComponent, new RegionMainStoreComponent()); | |||
regionStoreEntity.setAlwaysRelevant(true); | |||
regionStoreEntity.setScope(EntityScope.GLOBAL); |
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 @DarkWeird mentioned something about removing sectors (and therefore scopes?). Just to make people aware ...
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.
This change was because setAlwaysRelevant
is deprecated, and setScope
is what it pointed me to.
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.
Global scope exists yet.
1 similar comment
src/main/java/org/terasology/dynamicCities/region/RegionEntityManager.java
Outdated
Show resolved
Hide resolved
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.
@keturn I'm approving this assuming that you checked in MetalRenegades, that cities and buildings still show up with your change in the RegionEntitiesManager
. Please only merge if you did ^^
@@ -273,10 +272,8 @@ public void setNameTagForRegion(EntityRef region) { | |||
region.addComponent(nT); | |||
} | |||
|
|||
@Command(shortDescription = "Toggles the view of nametags for region entities", runOnServer = true, | |||
requiredPermission = PermissionManager.DEBUG_PERMISSION) |
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.
Is this permission not required after all? Or why was it removed?
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 inspection told me that DEBUG_PERMISSION
is the default and suggested omitting it.
src/main/java/org/terasology/dynamicCities/region/RegionEntityManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/terasology/dynamicCities/region/RegionEntityManager.java
Outdated
Show resolved
Hide resolved
Cities? We know, after several months of playtesting, that Cities are a myth. Propaganda by the liberal elite to destabilize the traditional way of life by putting stories in the minds of our children, fantasies about the great named Cities and their buildings taller than trees. Hah! Oh, you mean in single-player, I should still expect to see cities and buildings in single-player? Is that a city? As for buildings, well,
DynamicCities/src/main/java/org/terasology/dynamicCities/settlements/SettlementEntityManager.java Lines 522 to 535 in 3a609aa
|
review was conditional on seeing buildings; have not seen buildings
Second attempt: didn't get that crash, but didn't get buildings. Tried on |
the code is way to fragile ;/ might have something to do with all the terrain features. |
Can confirm an apparent and distinct lack of buildings - darn termites! Noticed some logging I don't recall seeing before about broken parcels?
Checked two city spots, then also 0, 0, 0 as some past bugs have sent buildings back there, but nope, nothing. So maybe something is keep parcels from forming correctly? Otherwise the new MR worlds are gorgeous :-) |
needs more investigation and fixing wrt cities spawning
Example to compare to: Earlier used seed |
# Conflicts: # module.txt # src/main/java/org/terasology/dynamicCities/region/RegionEntityManager.java
Okay, now that #107 is merged (thank you Niruandaleth for the review) we have a properly failing test in this branch! and I've confirmed that if I re-add the I still want to track down where the setter for that is and figure out an interface that lets us avoid re-doing the math in different places. |
The location that DynamicCities/src/main/java/org/terasology/dynamicCities/region/RegionEntityProvider.java Lines 64 to 65 in 8bd4203
(It makes entities with an UnregisteredRegionComponent , and the RegionEntityManager system has an OnActivatedComponent handler that receives those and invokes add() with them.)
My notes so far:
|
Next steps?
|
We had a discussion about Dynamic Cities in today's meeting, and it went sprawling as these things do, but I think the the parts that are relevant for next steps here are:
|
adjust tests to compensate for the mysterious -1
I've set aside my quest to track down the origin of the I started a new MR world with the known seed and did find a city with two buildings there. I'd offer a screenshot, but the screenshot pushed the RAM usage over the edge and it crashed. |
Fixes errors about tests failing to resolve modules.
Also re-enabled some tests and got them working. I changed enough of the test file that GitHub PR interface doesn't recognize it as a file that moved, but it is making the same assertions.
This PR is also notable because it takes care of the last use of the deprecated ModuleTestingEnvironment superclass in Omega.
To Do