You were warned:
https://github.com/minetest/minetest/blob/4eb3baa51ed95d7661bbeecbbdf040d51ac32187/builtin/settingtypes.txt#L2112-L2114

EDIT: Duplicate of #5618

Oh, somehow I missed that issue, thanks.

Edit: This isn't exactly a crash though, more like a minor glitch 😉 In fact it's the only problem I encountered with multiple mapgen threads so far after running a private server for > 1 year.

I had not noticed this bug before. This is probably due to how most mapgens 'overgenerate' terrain by 1 node above and below the currently generating mapchunk.
Looking at the 2nd screenshot in #5618 i think i can see a stone layer just above the cutoff trees, which i did not notice at the time.

This is not a duplicate of #5618 , although it is related, and the stone layer has appeared alongside the chopped trees in one screenshot. Reopening.

See #5618 (comment)
I think this issue is closely related to #5618 as terrain overgenerates vertically by 1 node. Mulitple emerge threads seem to cause bugs with overgenerating structures (decorations, randomwalk caves, dungeons).

I made a simple game that shows the nodes produced by this bug as pink, it also marks mapchunk edges. Perhaps someone can find it useful for debugging this.

https://codeberg.org/perfect_city_game_studio/mapgen_test

Edit: replaced screenshot as it also contained a bug on my side.

Played around with this a bit. This is stone being placed where it shouldn't be, so I just looked where c_stone went. In the valleys mapgen, there's only one place where this ends up being used. This is in a loop which exceeds block bounds by +-1 Y:

https://github.com/minetest/minetest/blob/486dc3288d7b8e8c55bbc20dea11fef32fd7119b/src/mapgen/mapgen_valleys.cpp#L422

I tried taking this "overgeneration" out, and it seems like it might fix this issue, but it completely messes up lighting.

I'm not sure how "overgeneration" of +-1 Y is supposed to work, especially in a multithreaded context. I imagine in a singlethreaded context it might be fine due to only replacing ignore nodes.

I'm pretty sure we've had the cutoff trees from #5618 with default settings in VoxeLibre... no weird stone layers though.

We have a label for such weird issues, most of which may be manifestations of this issue: https://git.minetest.land/VoxeLibre/VoxeLibre/issues?q=&type=all&sort=&state=open&labels=211&milestone=0&project=0&assignee=0&poster=0

And the issue I mentioned specifically: https://git.minetest.land/VoxeLibre/VoxeLibre/issues/4369

We tried to look for possible causes in our code, but failed.

Instance of this in VoxeLibre nether:

Notice one layer of the structure is missing as well as the stones changing at the block boundary; almost like a biome mismatch.
Note that here this seems to be like a layer of nodes not being set; but it may as well be a layer that is filled with air when it shouldn't have been.

@kno10 This is with num_emerge_threads == 1, right?

Yes, num_emerge_threads is unchanged at 1.
Note that this only appears to happen on the y axis, I have never noticed something similar on the x or z borders.

I took a fresh look at the glitch and it turns out overgeneration by 1 mapblock also plays a role in this:

Notice how the glitch happens with stripes that are 16 nodes wide and near a mapchunk's edge.

I believe this happens because unfinished chunks write their data to chunks that had already been generated.
If you allow data flow only from finished chunks to unfinished this issue should disappear?
A support for this hypothesis is that not all chunks suffer from this bug which implies that generation order matters here.
Feel free to ignore if what I said is complete nonsense, I don't know how the thing works under the hood.

Using air instead of stone to generate the terrain reveals the full scale of this bug:

EDIT: The statistics I posted previously turned out to be nonsense so I deleted them.

Does it also occur when num_emerge_threads=1 sometimes? For example if you slowly ascending up to an ungeneread block. Of if you slowly descending to an ungenerated block?

The problem is that two neighboring chunks (aka 5^3 blocks + border) could generate simultaneously.
This could be prevented by:

1. Remove the queues from the individual threads.
2. Use 8 queues instead in the main thread protected by semaphores.
3. The threads consume blocks from those queues.
4. When pushing a block positions into those queues. The emerging block position is converted into the chunk-position, this chunk position is used to determine which of the 8 queues should is used.

v3s16 chunk_pos = getContainerPos(blockpos, 5);
int selected_queue =
    (chunk_pos.X % 2) << 2  |
    (chunk_pos.Y % 2) << 1  |
    (chunk_pos.Z % 2) << 0;

5. All threads simultaneously only consume blocks from the currently active queue. When a defined amount of blocks was consumed or the queue is empty and all threads are idle, the next queue becomes active.
   This way we can make sure that blocks of neighboring chunks never get created simultaneously.

To visualize which chunk member (aka block) goes in which queue.

index 1:
 x . x . x .
 . . . . . .
 x . x . x .
 . . . . . .

index 2:
 . x . x . x
 . . . . . .
 . x . x . x
 . . . . . .

index 3:
 . . . . . .
 x . x . x .
 . . . . . .
 x . x . x .

index 4:
 . . . . . .
 . x . x . x
 . . . . . .
 . x . x . x

index 5-8: Only in 3D

It is surely possible to make the emerge code not emerge neighboring map chunks, but is that the correct fix? I somehow doubt it.

It is surely possible to make the emerge code not emerge neighboring map chunks, but is that the correct fix? I somehow doubt it.

I've just implemented it, and it runs almost as fast as before. The artifacts are completely gone.

There needs to be a mechanism that stops neighboring chunks from generating simultaneously. And this way it works with almost no locking. An alternative would be to lock 26 neighboring chunks, per chunk that gets generated simultaneously. Each incoming block position needs to be tested if it is part of such chunk. And the thread needs to wait or redirect the position back into the queue and try another one. I've tried that as well, and it's worse than one thread.

Or do you imagine something entirely different like simultaneous writing to the same location and then merging after the fact?

@sfan5

It is surely possible to make the emerge code not emerge neighboring map chunks, but is that the correct fix?

I believe it could be a good solution to get around the locking issue. This is likely to improve the generation speed at large mapblock generation ranges where we can allow an 1 chunk gap in all directions.
The worst case scenario would be the same as with num_emerge_threads = 1, where a 2*2*2 arrangement of chunks is generated in series because each would overwrite neighbouring mapblocks.

@cosin15 Would you please be so nice to provide a PR? That would allow us to perform benchmarks to check whether such change would make sense.

As the overlap of map chunk generation is -- if I am not mistaken -- 16 nodes, and the error only every is one layer and only on the y axis -- doesn't this suggests it rather is a off-by-one in some if condition? But I haven't completely understood how overgeneration in Minetest works (and it's limits, #14437). It seems there are two kinds of overgeneration, one by a 16 node block, and the other by a single node on the y axis only (probably to determine what is a surface node and what is not?)
Limiting parallelism to non-neighbouring blocks is more of a workaround than a fix.

As the overlap of map chunk generation is -- if I am not mistaken -- 16 nodes, and the error only every is one layer and only on the y axis -- doesn't this suggests it rather is a off-by-one in some if condition? But I haven't completely understood how overgeneration in Minetest works.
Limiting parallelism to non-neighbouring blocks is more of a workaround than a fix.

It surely looks like that, but this does not account for the sliced trees. My solution only fixes the bug with the sliced trees, but it also prevents the other bug from coming to the surface. It is indeed not a fix for this particular issue.

Limiting parallelism to non-neighbouring blocks is more of a workaround than a fix.

Well, I'd rather pick a workaround that works rather than a proper fix that never arrives (this issue is 5 years old).
I tested the solution and it appears to work.

Limiting parallelism to non-neighbouring blocks is more of a workaround than a fix.

Well, I'd rather pick a workaround that works rather than a proper fix that never arrives (this issue is 5 years old). I tested the solution and it appears to work.

It is a workaround for the 'unfinished y-slices' as a side effect but a legit fix for the sliced trees. Because the locking mechanism that prevents neighboring chunks from generating simultaneously is not just broken, it appears to be absent to this day.

Sure, a workaround is better than nothing. But if this means every map chunk writes 1 layer into the chunk above, that also means unnecessary IO right?

Sure, a workaround is better than nothing. But if this means every map chunk writes 1 layer into the chunk above, that also means unnecessary IO right?

I don't know if that's the case. I bet that the 1 layer-bug becomes malicious dead code, that is lurking in the shadows.
We should fix it first before fixing the tree slices. Otherwise we will forget about this bug.

Mapgen v7 overgenerates terrain y+-1, writes to the VM:
https://github.com/minetest/minetest/blob/f467bde6ac3f906fcb3cf0a7e80b25f4db3beca0/src/mapgen/mapgen_v7.cpp#L540
These extra borders +-1 in each axis are "allocated" here:
https://github.com/minetest/minetest/blob/f467bde6ac3f906fcb3cf0a7e80b25f4db3beca0/src/servermap.cpp#L212-L214
and a VManip for this region is set up:
https://github.com/minetest/minetest/blob/f467bde6ac3f906fcb3cf0a7e80b25f4db3beca0/src/servermap.cpp#L258-L259
But then later on it calls blitBackAll on the entire VM. This is probably where this 1-layer write comes from?
Maybe instead of a blitBackAll, a blitBackArea should be used that only writes the main area of map generation?

Mapgen v7 overgenerates terrain y+-1, writes to the VM:

https://github.com/minetest/minetest/blob/f467bde6ac3f906fcb3cf0a7e80b25f4db3beca0/src/mapgen/mapgen_v7.cpp#L540

These extra borders +-1 in each axis are "allocated" here:

https://github.com/minetest/minetest/blob/f467bde6ac3f906fcb3cf0a7e80b25f4db3beca0/src/servermap.cpp#L212-L214

and a VManip for this region is set up:

https://github.com/minetest/minetest/blob/f467bde6ac3f906fcb3cf0a7e80b25f4db3beca0/src/servermap.cpp#L258-L259

But then later on it calls blitBackAll on the entire VM. This is probably where this 1-layer write comes from?
Maybe instead of a blitBackAll, a blitBackArea should be used that only writes the main area of map generation?

blitBackAll should be fine, because the whole purpose of this overgeneration is to write trees into the blocks above the chunk.

Nevermind the lower part is in blocks, not in node coordinates.
So maybe the problem is simply that mapgen does this +- 1 row overgeneration and not only uses this for trees and dust?

So maybe the problem is simply that mapgen does this +- 1 row overgeneration and not only uses this for trees and dust?

The code appears to overgenerate that stuff by 1 mapblock, yeah. I had that problem with voxel manipulators spawned for one whole chunk right after generation. It can run into a race condition with a neighboring chunk. In my case I tried changing soil from one thing to another but a neighboring chunk altered that change. This can be seen here: #13712
I misdiagnosed the issue back then. (notice 16x16 or 16x80 stripes typical for overgeneration)

Disabling the overgeneration apparently causes lighting issues, but there are known lighting issues even as-is, which prompted for example creation of https://gitlab.com/sztest/szutilpack/-/tree/master/szutil_fixhack

Maybe the proper way to fix this and other issues is to look in depth at how we do lighting, and how we could do it better?

Or do you imagine something entirely different like simultaneous writing to the same location and then merging after the fact?

AFAIK this is what we're trying to do right now.
The idea is to emerge one block more in each direction, but since it's filled with ignore only the changed nodes will be applied to the map, allowing multiple threads to work on the overlapping area.
So no cut-off trees should happen (if the trees are < 16 nodes).

Or do you imagine something entirely different like simultaneous writing to the same location and then merging after the fact?

AFAIK this is what we're trying to do right now. The idea is to emerge one block more in each direction, but since it's filled with ignore only the changed nodes will be applied to the map, allowing multiple threads to work on the overlapping area. So no cut-off trees should happen (if the trees are < 16 nodes).

That would be a really cool solution. I have some ideas for that:

• Rely only on ignore nodes might not be sufficient, because there are up to 8 generation threads that write to the same area at the same time. I would suggest, to implement some sort of node priority, for example leafs have lower priority than logs. This makes the generation deterministic.
• Instead of emerging one block in each direction, I would suggest to emerge blocks lazy. This might be faster, and at the same time more flexible in terms of how many blocks of padding there are. It would also be possible to make trees even larger.

for example leafs have lower priority than logs. This makes the generation deterministic.

This is a game-specific solution. There are Luanti games that don't have leaves/logs so modders would need to set priority themselves. What if the decoration is actually a tower?

In my Luanti game I wrote a custom mapgen where overgeneration happens by mirroring movement of a "brush" for each map division unit and that movement is deterministic. In other words for structure/decoration placement you could just completely remove overgeneration (the shell of 1 mapblock) and simply spawn the structure twice (or more) for each mapchunk with due offset and just ignore nodes that poke out of each mapchunk. As long as structure placement is deterministic it works.

EDIT: doing the above would actually increase the size limit for structures. In my game I generate 800x800 node "citychunks" and the brush mirroring (overgeneration) has a 200 node margin. Citychunks are generated together (with locking and all that stuff) so generating one involves generating all adjacent neighbors (including the "fake" overgeneration - mirroring generation of a structure for adjacent citychunks).

Or do you imagine something entirely different like simultaneous writing to the same location and then merging after the fact?

AFAIK this is what we're trying to do right now. The idea is to emerge one block more in each direction, but since it's filled with ignore only the changed nodes will be applied to the map, allowing multiple threads to work on the overlapping area. So no cut-off trees should happen (if the trees are < 16 nodes).

Except that, as far as i can tell, most mapgens (including mpgv7) do write one layer of nodes into the next chunk, not only for trees. And that is probably one of the causes this problem here.

In my Luanti game I wrote a custom mapgen where overgeneration happens by mirroring movement of a "brush" for each map division unit and that movement is deterministic. In other words for structure/decoration placement you could just completely remove overgeneration (the shell of 1 mapblock) and simply spawn the structure twice (or more) for each mapchunk with due offset and just ignore nodes that poke out of each mapchunk. As long as structure placement is deterministic it works.

Only if the structure placement is unconditional. This may work for your cities project, but will not work for many other cases.

For example, cavegen might remove the place the tree was supposed to be placed. Without computing cavegen also in the upper chunk (and hence without recomputing almost the entire lower chunk twice!) you do not know if the tree placement below will even be possible. Or some other structure may collide and prevent the tree from spawning. Or for more complex structures, terraforming may need to happen to make the ground flat enough, which may slightly move the position suggested by randomness.

I think the best would be if we had multiple tiers of chunk generation. If a chunk gets generated it is guaranteed that the neighboring chunks are on the same tier or higher.
This would allow us to place trees, with the knowledge of the landscape that is surrounding the chunk.

This would allow us to place trees, with the knowledge of the landscape that is surrounding the chunk.

Sounds reasonable. I actually plan to solve placing buildings in my game this way: first generate roads, then place buildings randomly and finally connects the buildings with streets and roads. It creates a nice hierarchy and makes sure further steps have all the data needed.

Only if the structure placement is unconditional. This may work for your cities project, but will not work for many other cases.

Fair point. My mapgen actually does conditional placing right now and is going to do even more, but I use a top-down strategy (the layout is defined before doing any actual mapgen). It could still work though as long as each thread would be given the whole context. You'd need to communicate the threads with each other for that, which could slow down generation.

As for your cave example I've seen trees and grass with nothing below them as a result of cavegen so the mapgen is still pretty blind to context as is right now.

@cosin15 You may also be interested in a similar bug that was not fixed with your proof of concept: #15643
It is related to river/ocean generation.