Server lighting still broken in 1.14 pre-4

The skylight issues still exist in 1.14 pre-4.
An easy way to reproduce them is as follows:

• Create a new redstone ready world

• Restart the world after it is generated

• Run /setblock 0 96 0 stone

You will obtain the following

While the client light is correctly at 15, note that the server light is at 0! Reloading the world does not fix the issue.

I have done some code digging and found the root cause of this. The problem lies within the way new lightmaps for subchunks are created, initialized and added to the world.

New light maps are created inside

nwt.minecraft.world.chunk.light.LightStorage.java

protected void setLevel(long long_1, int int_1) {
  int int_2 = this.getLevel(long_1);
  if (int_2 != 0 && int_1 == 0) {
	 this.field_15808.add(long_1);
	 this.field_15804.remove(long_1);
  }

  if (int_2 == 0 && int_1 != 0) {
	 this.field_15808.remove(long_1);
	 this.field_15797.remove(long_1);
  }

  if (int_2 >= 2 && int_1 != 2) {
	 if (this.toRemove.contains(long_1)) {
		this.toRemove.remove(long_1);
	 } else {
		this.dataStorage.addForChunk(long_1, this.getDataForChunk(long_1));
		this.field_15802.add(long_1);
		this.method_15523(long_1);

		for(int int_3 = -1; int_3 <= 1; ++int_3) {
		   for(int int_4 = -1; int_4 <= 1; ++int_4) {
			  for(int int_5 = -1; int_5 <= 1; ++int_5) {
				 this.toNotify.add(ChunkSectionPos.toChunkLong(BlockPos.add(long_1, int_4, int_5, int_3)));
			  }
		   }
		}
	 }
  }

  if (int_2 != 2 && int_1 >= 2) {
	 this.toRemove.add(long_1);
  }

  this.hasLightUpdates = !this.toRemove.isEmpty();
}

The relevant part here is

this.dataStorage.addForChunk(long_1, this.getDataForChunk(long_1));
this.field_15802.add(long_1);
this.method_15523(long_1);

this.getDataForChunk(long_1) creates an uninitialized lightmap that is directly added to the world. Some sort of initialization is done via this.method_15523(long_1);, so let's look at that:

net.minecraft.world.chunk.light.SkyLightStorage.java

protected void method_15523(long long_1) {
  int int_1 = ChunkSectionPos.unpackLongY(long_1);
  if (((SkyLightStorage.Data)this.dataStorage).defaultHeight > int_1) {
	 ((SkyLightStorage.Data)this.dataStorage).defaultHeight = int_1;
	 ((SkyLightStorage.Data)this.dataStorage).heightMap.defaultReturnValue(((SkyLightStorage.Data)this.dataStorage).defaultHeight);
  }

  long long_2 = ChunkSectionPos.toLightStorageIndex(long_1);
  int int_2 = ((SkyLightStorage.Data)this.dataStorage).heightMap.get(long_2);
  if (int_2 < int_1 + 1) {
	 ((SkyLightStorage.Data)this.dataStorage).heightMap.put(long_2, int_1 + 1);
	 if (this.field_15817.contains(long_2)) {
		this.field_15815.add(long_1);
		this.field_15816.remove(long_1);
		if (int_2 > ((SkyLightStorage.Data)this.dataStorage).defaultHeight) {
		   long long_3 = ChunkSectionPos.asLong(ChunkSectionPos.unpackLongX(long_1), int_2 - 1, ChunkSectionPos.unpackLongZ(long_1));
		   this.field_15815.remove(long_3);
		   this.field_15816.add(long_3);
		}

		this.checkForUpdates();
	 }
  }
}

This method schedules the lightmaps to be initialized somewhere else. Without going into detail, the initialization will then set the topmost subchunk to a skylight value of 15 and propagate that to the neighbors. Lower subchunks are not initialized! (more on that later).

The problematic part here is the if (this.field_15817.contains(long_2)) check. As far as i understand it, this is meant as some way to disable skylight initialization for some chunks, eg. during worldgen.
But as it turns out, this disables basically all serverside skylight initialization.
Skipping some parts of the call tree, the only serverside place where skylight initialization is enabled for a chunk (ie. it is added to field_15817) is inside

net.minecraft.server.world.ServerLightingProvider.java

public CompletableFuture<Chunk> light(final Chunk class_2791_1, final boolean isLightOn)
    {
        final ChunkPos class_1923_1 = class_2791_1.getPos();
        this.enqueue(class_1923_1.x, class_1923_1.z, ServerLightingProvider.class_3901.field_17261, SystemUtil.debugRunnable(() -> {
            if (!isLightOn)
            {
                super.enableLight(class_1923_1, true);
            }

            final ChunkSection[] class_2826s_1 = class_2791_1.getSectionArray();

            for (int int_1 = 0; int_1 < 16; ++int_1)
            {
                final ChunkSection class_2826_1 = class_2826s_1[int_1];
                if (!ChunkSection.isEmpty(class_2826_1))
                {
                    super.updateSectionStatus(ChunkSectionPos.from(class_1923_1, int_1), false);
                }
            }

            if (!isLightOn)
            {
                class_2791_1.getLightSourcesStream().forEach((class_2338_1) -> {
                    super.method_15560(class_2338_1, class_2791_1.getLuminance(class_2338_1));
                });
            }

            class_2791_1.setLightOn(true);
            this.chunkStorage.method_20441(class_1923_1);
        }, () -> {
            return "lightChunk " + class_1923_1 + " " + isLightOn;
        }));
        return CompletableFuture.supplyAsync(() -> {
            return class_2791_1;
        }, (runnable_1) -> {
            this.enqueue(class_1923_1.x, class_1923_1.z, ServerLightingProvider.class_3901.field_17262, runnable_1);
        });
    }

This means that only chunks that had their initial lighting done recently are added to the set. This is why in the reproduction steps above you need to restart the world. That way, the chunks won't be inside this set anymore, and hence the skylight initialization is disabled. Because of that, new lightmaps stay at their initial light level of 0, which is exactly what was observed above.

The solution to this is rather easy: Just add the chunks that are already lighted to that set as well.

Now, with that out of the way, may I ask why you implemented the initialization the way you did? In my opinion, this is rather complicated and can be simplified quite a bit.
For newLight, I just initialize new lightmaps as follows which works perfectly fine:

• Before adding a new lightmap to the world, initialize it to whatever light value would have been returned before adding it. This means, for the topmost section, initialize it to 15. For lower sections, apply the usual skylight lookup, ie. inherit the next skylight value above the position.

• This makes sure that adding the lightmap does not change any light value. Currently, adding a new lightmap changes some skylight values to 0, so you have to do some work to compensate for that.

• You don't need to schedule any light checks for the subchunk or any neighbors, because adding the initialized lightmap didn't change any light values.

In my opinion this approach is much simpler.

As it turns out, the current Vanilla initialization code has some other bug. As can be seen in method_15523 posted above, initialization only happens when adding a new topmost subchunk. This new topmost subchunk gets then initialized (modulo the fact that it currently doesn't work on the server). Subchunks that were previously topmost get some special handling, namely lighting for their top face get reevalutaed (this is not necessary for my simple initialization approach). However, adding a new subchunk below the current topmost subchunk does not trigegr any initialization. This leads to an error as can be reproduced as follows:

• Create a new redstone ready world

• Restart the world after it is generated

• Run the following commands one after another

  /setblock 0 95 32 stone
  /fill 0 112 16 15 112 31 stone
  /setblock 0 112 -16 stone
  /fill 0 95 0 15 95 31 stone

and obtain the following picture

Note that clientside lighting is not affected by the above issue, so fixing it will not fix this new issue.

So, what is going on? Let's look at it step by step; I refer to the two subchunks seen in the picture as left and right subchunk.

• /setblock 0 95 32 stone makes sure that a lightmap for the right subchunk is created and added to the world. On the client, this lightmap is properly initialized.

• /fill 0 112 16 15 112 31 stone blocks direct skylight access for the right subchunk, so the right subchunk only gets its light values from the sides.

• Together with /setblock 0 112 -16 stone, this also adds lightmaps above the left subchunk and its neighbors (which are still properly initialized on the client).

• Now comes the interesting part: /fill 0 95 0 15 95 31 stone adds a lightmap for the left subchunk and its neighbors. In the previous step, we made sure that there are already lightmaps above it. By what I have said earlier, those new lightmaps don't trigger any initialization, because they are not topmost. Hence they stay at their initial light level of 0. This is why the left subchunk is completely dark.

• What's going on in the right subchunk? In step 2 we block the direct skylight access, so it only gets its light values from the sides. The fourth step adds another platform above it. But because the right subchunk already gets its light from the sides but not from above, this does not trigger the light values to decrease, because only the top face is blocked, which didn't contribute to the lighting anyway.

• The important point is now that the left subchunk didn't cause any initialization. No light checks are scheduled for itself or its neighbors. That means that the addition of a lightmap for the left subchunk suddenly switches light values from 15 to 0 without the lighting engine noting it. While spawning the stone floor causes light checks, they don't have any effect because the lighting is already 0. Hence the lighting engine never detects that the right subchunk shouldn't receive any light from the left subchunk anymore.

What does this rather long explanation tell us? The main problem is basically that adding a lightmap causes the light values to change without the lighting engine noting it. While one could solve this by scheduling light checks for this subchunk and its neighbor, I find my simple initialization approach described above much more intuitive and easier to implement. Because the simple initialization approach does not change any light values, there are no checks that need to be performed.

So, are there any problems with my initialization approach?
I think the only problem is how you handle not yet lighted chunks.
There are several possibilities:

1. Just use the simple initialization algorithm all the time. This is not really desired during worldgen. That is because spreading light is easier than removing light. You don't want things to be lit while you are changing large amounts of blocks.
   I assume that's the reasoning behind the Vanilla way of disabling skylight initialization before a chunk is initially lighted (ie. the mechanism from the first paragraph)

2. Keep everything dark before the chunk is initially lighted, even those subchunks that are above all blocks. Turn on the skylight during initial lighting.
   This approach has the problem that neighbor chunks now can spread light into subchunks that are above all blocks and would otherwise be at ligth level 15. This creates more unnecessary light updates, and furthermore causes more lightmaps to be created, which is not really desireable.

Hence I propose the following solution (which I basically implemented in newLight):

• Keep the whole chunk dark before it is initially lighted, as in the second approach. But don't spread any light updates into it.

• Whenever there is a light update that wants to propagate into the not yet lighted chunk, remeber it and carry it out during initial lighting. Also, don't create any lightmaps for this chunk yet, create them during initial lighting.
  This eliminates the issue of the second approach I described above.

• You don't need to remeber individual light updates. You can just remember that a face of a subchunk got hit by some light update and then reevaluate the whole face. Usually, when a face gets hit, it actually gets hit by many updates, so checking the whole face isn't any more expensive than remembering individual updates.

Initial skylight then looks as follows:

• Create lightmaps with the usual algorithm (ie. at chunk distance <= 1 from any block), but don't initialize them yet (the lighting is still everyhwere 0).

• Set the skylight above the topmost lightmap to 15, ie. switch to the usual skylight calculation for empty subchunks.

• Spread skylight to the top face of the topmost lightmap.

• Above the topmost lightmap, spread skylight to the neighbor subchunks. If they are not yet lighted, don't do the spreading directly but remember those light updates, as with any other light updates.

• Carry out the remembered light updates for this chunk. Skylight updates that point into a section above the topmost lightmap can be skipped, because we are already at ligth level 15. This eliminates unnecessary updates scheduled in the previous step.

• Note: in the second last step, ie. spreading light to neighbors, we cannot omit checks for subchunks above the topmost subchunk if those neighbors are not yet lighted. That is because we don't know yet which subchunks will be above the topmost lightmap at the point of initial lighting. For already lighted neighbors, however, we can omit them.
  As explained in the last step, those unnecessary checks for not yet lighted neighbors can be dropped again when doing the initial lighting for the neighbor.

In my opinion, this approach is cleaner and easier to implement than the current Vanilla implementation and avoids some bugs of the current implementation.

If you want to discuss, you can write me on Discord (or here ofc) 🙂

Hope this (rather long, sry for that 😛) analysis is helpful 🙂
PhiPro