Implement support for Conditional Biomes#3
Conversation
|
Pinging @Adrijaned, @vedant-shroff, and @ktksan for possible review (really should sort through our GitHub teams to make people more assignable for reviews and such) |
skaldarnar
left a comment
skaldarnar
left a comment
There was a problem hiding this comment.
Looking at this, a few questions come to my mind:
-
Is a binary check enough? That obviously helps to in some cases to rule out some biomes, but falls short in other cases. For instance, as the biome info is done "per block" it could happen that multiple biomes are valid at that point - so how do I decide on which biome to use now? One often finds fitness functions that determine a non-binary value for each biome (which could be sorted by).
-
Should conditions be encoded in biomes or the biome registry? This approach introdcues a new
ConditionalBiomesubtype. If we've got non-conditional biomes, there is no way to add restrictions to them 😕 However, if theBiomeRegistrywould hold the information for each biome, we could enrich all kinds of biomes with conditions...
|
|
||
| /** | ||
| * Returns all biomes that do not forbid themselves from the given location. | ||
| * @param region The world region |
There was a problem hiding this comment.
| * @param region The world region | |
| * @param region The world region to generate, holding the facet data |
There was a problem hiding this comment.
The reason I created the implementation inside the biome instead of inside the registry is that it is a much simpler matter to add abstract conditional logic as an overridden method of the biome than it is to insert such logic directly into a map.
Honestly, it leaves me wondering if biomes should not be changed to a form of asset that can be handled by the entity system, like prefabs... but I believe that doing so would likely require an overhaul of the game loading system so that the entity system is initialized before facets are created.
There was a problem hiding this comment.
Well, the facet providers already use components to hold their configuration (not that this is working for actual world gen, though). Turning biomes into assets sounds like a good idea, overall...
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| public abstract class BaseConditionalBiome implements ConditionalBiome { |
There was a problem hiding this comment.
Please provide Javadoc what this base class offers.
| */ | ||
| Set<Class<? extends FieldFacet2D>> getLimitedFacets(); | ||
|
|
||
| void setLowerLimit(Class<? extends FieldFacet2D> facetClass, Float minimum); |
There was a problem hiding this comment.
This is missing Javadoc on the semantics of minimum. In the BaseConditionalBiome I can see that both values or inclusive - that should be noted here.
Likewise, for setUpperLimit.
| @Override | ||
| public void setLowerLimit(Class<? extends FieldFacet2D> facetClass, Float minimum) { | ||
| limitedFacets.compute(facetClass, (k, v) -> { | ||
| if (v == null) v = new Vector2f(minimum, Float.MAX_VALUE); |
There was a problem hiding this comment.
Please use braces even for these simple one-line if-statements.
There was a problem hiding this comment.
Also, I'd do
if (v == null) v = new Vector2f(Float.MIN_VALUE, Float.MAX_VALUE);for the initialization in both cases, and only set the minimum or maximum once afterwards.
| import java.util.Set; | ||
|
|
||
| public abstract class BaseConditionalBiome implements ConditionalBiome { | ||
| protected Map<Class<? extends FieldFacet2D>, Vector2f> limitedFacets = new HashMap<>(); |
There was a problem hiding this comment.
Instead of using Vector2f to store the range, maybe consider using the Range class from Guava?. Handily, it would come with range.contains(value) for even simpler checks 😉
Adrijaned
left a comment
Adrijaned
left a comment
There was a problem hiding this comment.
This code seems to promote the use of per-column biomes where it should work well, and probably couldn't be used in worlds making use of per-block biomes.
| @@ -0,0 +1,56 @@ | |||
| /* | |||
| * Copyright 2019 MovingBlocks | |||
| @@ -0,0 +1,74 @@ | |||
| /* | |||
| * Copyright 2019 MovingBlocks | |||
4 participants
This PR introduces the ability for biomes to implement arbitrary conditions that must be met for them to be selected by the world generator for a specific generating region.
These conditions are primarily intended to be world facets such as humidity, temperature, and surface elevation.
With this addition, a generator may receive a list of valid biomes to choose from for a certain area, rather than hardcoding specific conditions. This allows for modules to more easily add its own new biomes to existing world generators.