[1.10] Buildings in village not fully forming

[ezterry]

As shown in the image at the bottom of the report some villages are forming with incomplete houses.

Reproducing this is mildly difficult as it appears to be a problem when parts of a village are generated, while the rest remains in unloaded chunks, Then when the chunks load the village continues to generate but not using the same rng. (problem loading Village.dat? seems generated correctly)

To reproduce I've followed the following process:

1. Generate a new superflat world (preset 'Bottomless Pit' and seed '12345')
2. Set render distance to 10 (if you have a fast computer you might otherwise generate more than my system)
3. teleport to -675 ~ 300
4. run (or better yet sprint fly) to the north east (this will load area around multiple villages)
   4b) If you keep going north east you will fly into a village at aprox -21 -380
5. f3+a to reload chunks (not sure if this is needed)
6. The village you are in is likely generated mostly normally, however if you fly west to aprox -228 -279 you will find another village ( usually find some wall missing here) also you can fly to villages at -125,355 and -676 93*

* This is the village the screenshot is from.

Ive run this in build 1983, note simply teleporting to the village causes the village to generate normally with all of the building walls.

[Actuarius]

@williewillus added labels [Bug]

[cpw]

This is probably reproducible in vanilla.

[ezterry]

So far (both before and after posting this issue) I've been unable to reproduce this in vanilla 1.10. Both following the script, or by how I found it initially, circling a super flat generated world (where I don't know the locations of villages in) searching for a village.

I've not run the test on forge 1.9.4 (so it may be outstanding from before the update) nor have I verified if there is or is not an issue with other larger structures (strongholds/mineshafts) to tell if this is village only, or a larger issue.. given the nature of the other structures it would be less obvious.

[KnightMiner]

I've seen a reddit post about this, so its likely a vanilla bug.

I also found this old bug report, so its possible that was somehow made worse in recent versions.

[LexManos]

Ya, from all random tests this is a vanilla bug.
If it can be tracked down and reliably reproduced, we can take a crack at it. But it's transient nature makes that difficult.
Let me know if you guys find a way to reproduce.

[ezterry]

Update:

So since I've been able to reproduce this with the steps above 98% of the time in forge I've been able to do some debugging. (vanilla and the MCP "Clean client" target I can't reproduce it)

My conclusion is its to do with a race condition from async loading (pull #2946) and not villages in particular.

in net/minecraft/world/gen/ChunkProviderServer.java
if (loader.chunkExists(this.worldObj, x, z)) will sometimes indicate a chunk dosn't exist (thus requires recreating, and preventing recreateStructures from being called when expected)

For debugging I've just added an else block to the condition to fall back to vanilla loading, the warning happens much more frequent than a village, but you may need to teleport a render distance or two from spawn and fly back to see it fully.

else{
    chunk = this.loadChunkFromFile(x, z);

    if (chunk != null)
    {
        this.id2ChunkMap.put(ChunkPos.chunkXZ2Int(x, z), chunk);
        chunk.onChunkLoad();
        chunk.populateChunk(this, this.chunkGenerator);
        net.minecraftforge.fml.common.FMLLog.bigWarning("Race Condition! chunk was loaded via non-async logic");
    }
}

The question is can an improvement be made to the chunk Exists check to fix the race condition or is this fall back the best approach (as this still leaves most requests that require diskio to use the async processing.)

[stephan-gh]

@ezterry Can you test if removing the chunkExists check completely (and always putting the chunk into the queue) fixes the problem? It is technically not necessarily required but more like an optimization.

E.g. instead of:

if (loader.chunkExists(this.worldOb, x, z)
{
    if (runnable != null)
    // ...
}

Try:

if (runnable != null)
// ...

[ezterry]

@Minecrell Yes it works,

Looking over the code closer, I'd say we can keep the async optimization (just need to ensure we don't optimize on sync calls) since ultimately if the async logic "misses" it calls sync to generate the chunk (thus will find it)

Thus you may wish to consider changing the if(runnable != null) condition to the following if/elseif

  if(runnable == null)
  {
    chunk = ...syncChunkLoad(...);
  }
  else if(loader.chunkExists(this.worldObj, x, z))
  {
    ...queueChunkLoad(...);
    return null;
  }

I did test this as well and all appeared to be well.