-
Bug
-
Resolution: Fixed
-
Minecraft 1.14 Pre-Release 5
-
None
-
Confirmed
-
Lighting
While correcting my reproduction steps for MC-148580, I have found another skylight issue.
The image shows that skylight does not get properly propagated from empty subchunks, as will be clear from the code analysis.
This can be reproduced by the following steps:
- Create a new redstone ready world
- Run the following commands one after another
/fill 0 48 -1 0 191 -1 stone /fill 0 191 0 47 191 15 stone /fill 0 48 14 47 191 14 stone
- The first command creates lightmaps for the subchunks where we want to observe the issue. This is important, because light updates are only propagated to subchunks that have a lightmap.
- The second step puts a stone ceiling above the subchunks from the first step. This causes the light updates which don't pull the skylight correctly from the adjacent empty subchunks.
- The last step creates a "screen" for visualization purposes. It is important to create this only as the last step, because we need the neighbors to not have a lightmap before step 2, and lightmaps are created 1 subchunk around blocks.
So, let's look into the code to see what's going wrong.
We start with the code that calculates skylight values based on the neighbors.
@Override protected int getMergedLevel(long id, long sourceId, int limitLevel) { ... for (long15 = BlockPos.removeChunkSectionLocalY(long15); !((SkyLightStorage)this.lightStorage).hasChunk(long17) && !((SkyLightStorage)this.lightStorage).m(long17); long17 = ChunkSectionPos.offsetPacked(long17, Direction.UP), long15 = BlockPos.add(long15, 0, 16, 0)) {} ChunkNibbleArray chunkNibbleArray20 = ((SkyLightStorage)this.lightStorage).getDataForChunk(long17, true); if (long15 != sourceId) { int integer21; if (chunkNibbleArray20 != null) { integer21 = this.getPropagatedLevel(long15, id, this.getCurrentLevelFromArray(chunkNibbleArray20, long15)); } else { integer21 = (((SkyLightStorage)this.lightStorage).n(long17) ? 0 : 15); } if (integer7 > integer21) { integer7 = integer21; } if (integer7 == 0) { return integer7; } } } } ...
This part of the method deals with neighbors that don't have a lightmap, which is what we are interested in.
The problem lies within the line
integer21 = this.getPropagatedLevel(long15, id, this.getCurrentLevelFromArray(chunkNibbleArray20, long15));
The srcId that is passed to getPropagatedLevel(...) is not the neighbor of id. Instead, long15 is the position where the light level is taken from, ie. the lowest position with a lightmap above the neighbor.
It should be clear that this is wrong, for example the propagation would use the blockstate at position long15 instead of air, which is the blockstate of the neighbor.
However, in our example above, the ceiling was placed at the top face of the chunk, so the blockstate of long15 should actually be air. Let's see in detail what goes wrong by examining getPropagatedLevel(...). The important part there is the call to
protected int getLightBlockedBetween(long src, long dest) { this.lightStorage.updateAll(); this.srcMutablePos.setFromLong(src); this.destMutablePos.setFromLong(dest); int integer6 = ChunkSectionPos.toChunkCoord(this.srcMutablePos.getX()); int integer7 = ChunkSectionPos.toChunkCoord(this.srcMutablePos.getZ()); int integer8 = ChunkSectionPos.toChunkCoord(this.destMutablePos.getX()); int integer9 = ChunkSectionPos.toChunkCoord(this.destMutablePos.getZ()); BlockView blockView10 = this.a(integer8, integer9); if (blockView10 == null) { return 16; } BlockState blockState11 = blockView10.getBlockState(this.destMutablePos); BlockView blockView12 = this.chunkProvider.getWorld(); int integer13 = blockState11.getLightSubtracted(blockView12, this.destMutablePos); if (!blockState11.g() && integer13 >= 15) { return 16; } BlockView blockView14; if (integer6 == integer8 && integer7 == integer9) { blockView14 = blockView10; } else { blockView14 = this.a(integer6, integer7); } if (blockView14 == null) { return 16; } int integer15 = Integer.signum(this.destMutablePos.getX() - this.srcMutablePos.getX()); int integer16 = Integer.signum(this.destMutablePos.getY() - this.srcMutablePos.getY()); int integer17 = Integer.signum(this.destMutablePos.getZ() - this.srcMutablePos.getZ()); Direction direction18 = Direction.fromVector(this.mutablePosGetLightBlockedBetween.set(integer15, integer16, integer17)); if (direction18 == null) { return 16; } BlockState blockState19 = blockView14.getBlockState(this.srcMutablePos); return a(blockView12, blockState19, this.srcMutablePos, blockState11, this.destMutablePos, direction18, integer13); }
Because src is not a neighbor of dest,
Direction.fromVector(this.mutablePosGetLightBlockedBetween.set(integer15, integer16, integer17)); if (direction18 == null) { return 16; }
will fail and return an opacity of 16. Hence no light will be propagated from neighbors without lightmaps, which is exactly what the image shows.
Note: Simply passing the correct neighbor of id to getPropagatedLevel(...) inside getMergedLevel(...) won't work. That is because that neighbor would not have a lightmap, so getLightBlockedBetween(...) would fail in
BlockView blockView14; if (integer6 == integer8 && integer7 == integer9) { blockView14 = blockView10; } else { blockView14 = this.a(integer6, integer7); } if (blockView14 == null) { return 16; }
One possible solution to this is the following:
- Whenever that situation occurs, we know that both id and its neighbor have blockstate air. Otherwise, if at least one is not air, there would be lightmaps created for both subchunks, which we assumed not to be the case.
- In this case, we know that the propagated value is simply srcValue - 1.
(While this argument is not strictly true in an multithreaded environment, because there could be blockchanges between starting the lighting task and carrying out the update at that position, this issue arises for every light update. We can apply the usual argument that the assumption was true at the time the update was scheduled. In case the assumption gets invalidated, another update gets scheduled, so we don't need to worry.)
- relates to
-
- Resolved