Add temperature and humidity values to biome #8
+68
−29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
skaldarnar
requested changes
Jun 28, 2020
skaldarnar
reviewed
Jul 4, 2020
There was a problem hiding this comment.
This looks good from the technical point of view now, but my concerns raised in Terasology/BiomesAPI#5 (comment) become apparent here.
We have an implicit conversion going on in BiomeProvider L68-77, interpreting the abstract facet temperature values directly as biomes. The relation of these numbers does not necessarily match with the relation of temperatures and humidity defined here.
On the other hand, maybe that's a good thing, and we should completely forget about the underlying facet temperature noise ... 🤔
| OCEAN("Ocean"), | ||
| BEACH("Beach"), | ||
| PLAINS("Plains"); | ||
| MOUNTAINS("Mountains", .3f, .09f), |
There was a problem hiding this comment.
This looks way better from the technical perspective now 👍
Cervator
requested changes
Aug 30, 2020
There was a problem hiding this comment.
Seems reasonable, just a couple trivial comments :-)
I haven't fully been up to speed with all this, but I guess with this PR we start tracking two new things per-block for just about any setup (since CoreWorlds is so fundamental). Are we concerned at all about memory usage / performance due to that change?
| Rect2i processRegion = facet.getWorldRegion(); | ||
| for (BaseVector2i position : processRegion.contents()) { | ||
| // clamp to reasonable values, just in case | ||
| float noiseAdjusted = TeraMath.clamp(humidityNoise.noise(position.x(), position.y()) + .6f, 0f,1f); |
There was a problem hiding this comment.
What are reasonable values, anyway? Not sure what the numbers here mean, may be good to name them via constants? There's also a space missing after 0f, for a quick trivial style fix :-)
| for (int i = 0; i < noise.length; ++i) { | ||
| noise[i] = TeraMath.clamp((noise[i] * 2.11f + 1f) * 0.5f); | ||
| for (BaseVector2i position : processRegion.contents()) { | ||
| // modify initial noise |
|
After some more testing I have a feeling there's a severe bug related to the extra data being added to CoreWorlds when a secondary client attempts to join a multiplayer game (leading to protobuf, network communication, and so on). Log below. This does not happen with FlowingLiquids in a similar multiplayer setup, despite also using |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a temperature and humidity value to each CoreBiome - it's required for Terasology/ClimateConditions#18 to work. It also relies on Terasology/BiomesAPI#5.