Significant re-write, changing the algorithm and lake shape. #14
Conversation
| "author": "The Terasology Foundation", | ||
| "displayName": "Lakes", | ||
| "description": "Plugin for Worldgenerator, which creates lakes based on random polygons", | ||
| "description": "Plugin for world generation, which creates small lakes", | ||
| "dependencies": [ | ||
| { "id": "CoreAssets", "minVersion": "2.0.1" }, |
There was a problem hiding this comment.
(just me thinking out loud)
Hm, I somehow expected some dependency update because of the new facets that are now required... 🤔 now I'm wondering whether we should move those to the CoreWorlds repo instead 🤔 (related engine commit MovingBlocks/Terasology@5d5c7a6)
There was a problem hiding this comment.
That's a good point. I probably should have updated the version number of CoreWorlds.
| public static final int MAX_DEPTH = 12; | ||
| public static final int MIN_VERTICES = 10; | ||
| public static final int MAX_VERTICES = 30; | ||
| public class Lake implements Iterable<Vector3i> { |
There was a problem hiding this comment.
Hm, I'm not sure I like that Lake is an Iterable<Vector3i> ... this way we can write something like the following to iterate over all block positions in the lake, right?
for (Vector3i position: lake) {
...
}There was a problem hiding this comment.
I wrote it this way so as to not give write access to the set of points. If there were an explicit function returning an Iterator, that wouldn't work with the for syntax. I guess I could just make the Set public instead. There isn't really any harm in it (and if other facet providers need to modify lakes, they might have a legitimate reason to access it anyway).
| -Missing biome modification to prevent grass and trees to spawn on lakes | ||
| ~~-surface lakes have difficulties appearing in perlin and other non flat world gens (unknown reason)~~ | ||
| -the "tubs" of surface lakes are sometimes incomplete | ||
| - Occasionally, lakes have uneven surfaces |
There was a problem hiding this comment.
Do they still have that? Does that happen if two lakes intersect with each other? Or is this fixed by the lake having a single surface height value? (https://github.com/Terasology/Lakes/pull/14/files#diff-f18b6a2bae9571d62ca21dbb5303170d4c694f954c0f5842b879bb6ef8c4fb96R33)
There was a problem hiding this comment.
I assume it's something to do with lakes intersecting, although I don't know how that would be possible. It is at least very rare, and if you have FlowingLiquids enabled, it would quickly stabilise to a lower flat surface anyway, so I just decided to not worry about it.
| for (Lake lake : lakeFacet.getLakes()) { | ||
| for (Vector3i pos : lake) { | ||
| if (chunkRegion.getRegion().encompasses(pos)) { | ||
| Block block = pos.y > lake.surfaceHeight ? air : lake.liquid; | ||
| chunk.setBlock(ChunkMath.calcRelativeBlockPos(pos), block); |
There was a problem hiding this comment.
It allows somewhat less explicit control over how intersecting lakes are dealt with than the previous approach, but it should also be much more efficient.
| @RegisterPlugin | ||
| @Produces(LakeFacet.class) | ||
| @Updates({ | ||
| @Facet(value = SurfacesFacet.class, border = @FacetBorder(sides = Lake.MAX_RADIUS * 3, bottom = Lake.MAX_DEPTH + 3, top = Lake.MAX_DEPTH + 3)), |
There was a problem hiding this comment.
A facet that is being updated is automatically also part of @Requires(...)?
There was a problem hiding this comment.
Yes, it's a bit like a combination of @Requires and @Produces, although I think there's a slight problem with borders and @Updates.
| SurfacesFacet surfacesFacet = region.getRegionFacet(SurfacesFacet.class); | ||
| DensityFacet densityFacet = region.getRegionFacet(DensityFacet.class); | ||
| ElevationFacet elevationFacet = region.getRegionFacet(ElevationFacet.class); | ||
| SeaLevelFacet seaLevelFacet = region.getRegionFacet(SeaLevelFacet.class); |
There was a problem hiding this comment.
I like how you retrieved all the facets right in the beginning.
(just thinking out loud again)
This looks quite similar to the way we @ReceiveEvent and that it would be nice to write
@ProcessRegion
public void process(GeneratingRegion region, SurfacesFacet surfacesFacet, DensityFacet densityFacet, ...) {
...
}There was a problem hiding this comment.
I guess that would be possible. This pattern is very common in facet providers and rasterizers. The @Requires and @Updates annotations could even be moved here too, to reduce duplication a bit (e.g. something like replacing a SurfaceHeightFacet with an ElevationFacet would just require one change, rather than three).
|
I increased the minimum radius from 3 to 5, and increased the minimum depth a bit too so that one-block deep lakes are rarer. Lakes smaller than 10 blocks across are still quite common, due to the noise that's added, and technically even lakes of only 1 block are possible, but that would be extremely rare. |
I started updating for MovingBlocks/Terasology#4237, then ended up re-writing pretty much the whole module instead. Now lakes are more rounded in shape, with the floor smoothly curving up to the shore, rather than there being a sudden cliff at the shore. Also, it's better at detecting when an area is too sloped for a lake, and the shape of the lakes' surfaces is somewhat different too, because now it's based on distorting a circle with brownian noise, rather than making polygons, as before.
The polygon based algorithm did give reasonable shapes for the lake surfaces, but it didn't integrate well into calculating the depth of the lakes, so I made it essentially calculate the depths first, then the lake is all those locations where the depth is greater than 0.
Resolves #11, #10, #9 (although that's only still current because I recently re-introduced in in a CoreWorlds PR), #1, and probably #7 and #12 too (basically it's a complete re-write, so all the old bugs are replaced with exciting new bugs).