Affects Version/s: Minecraft 1.14 Pre-Release 5
Fix Version/s: Minecraft 1.14
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
- 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.
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
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
Because src is not a neighbor of dest,
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
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.)