Uploaded image for project: 'Minecraft: Java Edition'
  1. Minecraft: Java Edition
  2. MC-170010

Sky-lightmaps not properly initialized

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Open
    • Resolution: Unresolved
    • Affects Version/s: 1.15.1, 1.15.2 Pre-release 2, 1.15.2, 20w06a, 20w20b, 20w21a, 1.16.2
    • Fix Version/s: None
    • Confirmation Status:
      Confirmed
    • Category:
      Lighting
    • Mojang Priority:
      Important

      Description

      This is a direct continuation of MC-148580. I post it as a new report since the old one already contains several similar bugs and I don't want it to digress too much, given that it's already closed for quite a while now.

      The issue remains the same, namely that the topmost lightmap is not correctly initialized to a light value of 15, but rather to a value of 0.

      net.minecraft.world.chunk.light.SkyLightStorage.java
      protected ChunkNibbleArray createLightArray(long pos) {
          ChunkNibbleArray chunkNibbleArray = (ChunkNibbleArray)this.lightArraysToAdd.get(pos);
          if (chunkNibbleArray != null) {
              return chunkNibbleArray;
          } else {
              long l = ChunkSectionPos.offset(pos, Direction.UP);
              int i = ((SkyLightStorage.Data)this.lightArrays).topArraySectionY.get(ChunkSectionPos.withZeroZ(pos));
              if (i != ((SkyLightStorage.Data)this.lightArrays).defaultTopArraySectionY && ChunkSectionPos.getY(l) < i) {
                  ChunkNibbleArray chunkNibbleArray2;
                  while((chunkNibbleArray2 = this.getLightArray(l, true)) == null) {
                      l = ChunkSectionPos.offset(l, Direction.UP);
                  }
      
                  return new ChunkNibbleArray((new ColumnChunkNibbleArray(chunkNibbleArray2, 0)).asByteArray());
              } else {
                  return new ChunkNibbleArray();
              }
          }
      }
      

      The lightmap will then be registered for relighting inside onLightArrayCreated(...) and finally be relighted in updateLightArrays(...). There are however some problems with this initialization code:

      • If the subchunk is non-empty, light is only propagated from above, but not from the sides. There are cases where this is not sufficient. However, this issue is completely shadowed by the next one.
      • The initialization is completely skipped if another lightmap is added above it.

      I attached a world producing wrong lighting using this second problem. Since I lag the lighting engine to spawn mutible blocks at once before the lighting engine can react, the provided world might not reliably work on some systems, but at least it does reliably work on mine. Would be nice if someone can confirm this on their system.
      The setup is as follows:

      • We have some subchunk that does not have a lightmap and no lightmap above, but all its 4 neighbors do have one.
      • Schedule a bunch of lightmap creations and light updates to lag the lighting engine, so we can spawn several blocks at once before the lighting engine can react to it. (This is similar to MC-169913, but a fix for it will not help here.)
      • Spawn a stone platform in the beforementioned subchunk.
      • Now this subchunk will get a lightmap, which gets scheduled for initialization. However, afterwards the subchunk above it will get a lightmap as well, canceling the initialization again.
        In the end, the lightmap of the subchunk containing the stone platform will remain uninitialized.
      • Since all its neighbors already had lightmaps, the initialization code does not spread light form there to the subchunk containing the stone platform, so light contributions from the side are missing.
        This is also the reason for the first problem I mentioned above regarding the initialization code. wouldn't it be shadowed by the second one.
      • Since block updates schedule light checks not only for the affected blocks but also for all 6 neighbors, we do get a contribution from the blocks directly below the stone platform, but not from any other blocks.
      • This produces the following image

       

      There is another bug caused by the initialization code, namely MC-169498. The problem here is that upon removing a lightmap, the new topmost lightmap is scheduled for reinitialization. In case the corresponding subchunk is empty, initialization then sets the whole lightmap to 15 and spreads light to its neighbors. In the setup described in the bug, as soon as the stone block is broken, this will cause the subchunk it was in to be set to a lightvalue of 15. Note that also MC-169913 plays a role here as it causes lightmaps to be removed before the block removal is properly processed; however, that doesn't really matter for the solution discussed below.
      As always, the problem with this is that this initialization changes light values without informing everyone about it; in this case, the subchunk containing the sandstone blocks is not marked dirty and hence not rerendered. As can be seen, the lighting is actually correctly at a value of 15, but the subchunk is not rerendered and hence still shows the value of 14.

       

      My suggested solution is the same as for MC-148580: Don't change light values by creating lightmaps, but initialize them to the exact values that would have been returned before. As no light values change, you don't have to inform anyone about any changes and you don't have to schedule any additional light and darkness propagations in order to compensate for changed values.
      Concretely, this means to initialize topmost skylightmaps of already lighted chunks with a value of 15 and not doing anything else. For not yet lighted chunks, simply initialize the lightmap with 0, as is currently done, and also do nothing else in this case (except potentially some actions required for initial lighting, see below).
      This will get rid off both issues and in fact allows to remove some now obsolete code, reducing code complexity by quite a bit.

       

      As far as I understand it, the current lightmap initialization code basically handles the initial skylight. So, let's look into that in more detail and see that my suggested solution doesn't interfere with this. In fact, it will allow to simplify and optimize the code a bit. In the following, I will assume MC-170012 to be fixed.

      Starting with some definition, let's call all subchunks of a given chunk that do not have a lightmap above them (or associated directly to them) the empty region, and all subchunks that do not have a block above (or inside) them the pseudo-empty region; so the empty-region is that part of the pseudo-empty region that does not have associated lightmaps above (or directly associated to) them.
      Before a chunk is lighted, it does not receive any direct skylight, ie. the empty region is completely dark.
      The invariant we want to prove is:

      • Before a chunk is lighted, it properly receives and propagates all skylight contributions from its neighbors, but its pseudo-empty region might be opaque to skylight. More precisely, the interior of every subchunk in the pseudo-empty region is transparent to skylight, but its faces (except the top face) might be opaque, whereas the top face is also transparent.
      • Once a chunk is lighted, its pseudo-empty region is now properly transparent and has a skylight value of 15. All skylight is properly propagated to neighbors (except to their pseudo-empty regions which might be opaque).

      During initial lighting, the topmost lightmap gets scheduled for initialization inside

      net.minecraft.world.chunk.light.SkyLightStorage.java
      protected void setLightEnabled(long l, boolean bl) {
          this.updateAll();
          if (bl && this.lightEnabled.add(l)) {
              int i = ((SkyLightStorage.Data)this.lightArrays).topArraySectionY.get(l);
              if (i != ((SkyLightStorage.Data)this.lightArrays).defaultTopArraySectionY) {
                  long m = ChunkSectionPos.asLong(ChunkSectionPos.getX(l), i - 1, ChunkSectionPos.getZ(l));
                  this.method_20810(m);
                  this.checkForUpdates();
              }
          } else if (!bl) {
              this.lightEnabled.remove(l);
          }
      }
      

      The initialization then basically pushes skylight from above to the topmost lightmap and then propagates it, using some optimization in case the subchunk is empty as mentioned before (but that's not relevant here).
      Now let's prove the invariant inductively.

      • Since not yet lighted chunks do not receive direct skylight, skylight can only exist in a 3x3 neighborhood around lighted chunks. Assuming MC-170012 has been fixed, all relevant lightmaps have been created in that region. Hence, all skylight updates will be properly propagated, except for the case when they get stuck at the boundary to the empty region because of missing lightmaps. This case is however in accordance with our inductive hypothesis, so this is not a problem.
      • When a chunk gets lighted, it already receives all contributions from neigbor chunks. The only remaining defects are that it does not yet receive direct skylight and the pseudo-empty region might be opaque. The beforementioned initialization code will now propagate direct skylight to the topmost blocks of the topmost lightmap, which will then propagate and light up the whole pseudo-empty region, setting it to a value of 15 and hence making it transparent to skylight.
        Hence, the chunk itself receives all skylight contributions after it has been lighted.
      • The initialization code also pushes skylight from lightmaps in the pseudo-empty region, and more generally from everywhere except the empty region, to all neighbor chunks. We need to show that all lightmaps in the non-pseudo-empty region of a neighbor chunk properly receive their contributions, ie. that no lightmap in the non-pseudo-empty-region of a neighbor chunk is adjacent to the empty region of the chunk that is currently being lighted. However, this is clear, since a subchunk in the non-pseudo-empty region has a block above it by definition. This causes a lightmap to be created in the chunk that is currently being lighted that is above the lightmap in question, ie. the lightmap in question might be adjacent to the pseudo-empty region of the chunk that is currently being lighted, but not to the empty region, which is what we wanted to show.

      Note that this argument never used the complicated initialization logic for lightmaps of already lighted chunks. All the relevant logic is carried out by the initialization code as scheduled by setLightEnabled(...). Hence, it is safe to simply initialize topmost lightmaps in already lighted chunks with a skylight value of 15, without causing any interference with the initial lighting code.

       

      There might in fact be a missing point in the above argument, namely in case the pseudo-empty region of not yet lighted chunks can change over time. This would cause bugs already in the current version of the code and would need a fix independ of my suggested fix for the issue described here. I'm not familiar enough with the worldgen code to say whether or not this event is possible. For completeness, I will describe the issue more precisely and show that the current code would not deal with it.
      Suppose we have a chunk that has a lighted neighbor, so it is sufficiently far through worldgen to have reached the pre_ligth stage (and hence probably shouldn't receive any worldgen block updates; but as I said, I am not sure about this) but is not yet lighted itself (and hence probably shouldn't receive any block changes caused by actual game ticks). Now suppose it is possible for this chunk to receive block updates, in particular, we may spawn a stone platform in the empty region. Now, the subchunk containing the stone platform and all subchunks below are pushed to the non-pseudo-empty region. This breaks our invariant because they must no longer be opaque.
      The current initialization code manages to push in light from the side in case a new lightmap is spawned in the respective neighbor chunks. However, if the neighbor chunk already had a lightmap, the initialization code won't do anything and we are missing contributions from all sides. As the stone platform blocks light from above and there is no mechanism that will eventually push in light from the sides, this will indeed result in a presistent lighting error.
      Fixing this is rather easy: When a subchunk in a pre_light ed but not yet light ed chunk turns non-empty, we need to pull in skylight from all sides for this subchunk and all subchunks below that were previously in the pseudo-empty region. Note that we actually don't need to pull in light from the top faces, as these were already transparent before, but we do need to pull in light from all bottom faces, which might have been opaque before.
      As you can see, this fix is independent of lightmap initialization for already lighted chunks.

      As I said, I can't say whether this is relevant or not.

       

      I hope that my suggested fix is now sufficiently convincing

      Finally, let's discuss some simplifications and optimizations that are now possible:
      Since the (current) lightmap initialization code now solely deals with initial skylight, we can now properly move it to the rest of the initialization code, where it belongs, instead of being scattered around.
      As lightmap initialization now indeed no longer changes any light values, we can remove those parts of the code that compensated for changes in light values. Concretely, that is the last part of updateLightArrays(...) that currently spreads darkness to the previous topmost lightmap

      net.minecraft.world.chunk.light.SkyLightStorage.java
      this.pendingSkylightUpdates.clear();
      if (!this.field_15816.isEmpty()) {
          var4 = this.field_15816.iterator();
      
          label90:
      	while(true) {
      	    do {
      		    do {
      				if (!var4.hasNext()) {
      			    	break label90;
      			    }
      
      			    l = (Long)var4.next();
      		    } while(!this.field_15820.remove(l));
      	    } while(!this.hasLight(l));
      
      	    for(ag = 0; ag < 16; ++ag) {
      		    for(j = 0; j < 16; ++j) {
      		        long ai = BlockPos.asLong(ChunkSectionPos.getWorldCoord(ChunkSectionPos.getX(l)) + ag, ChunkSectionPos.getWorldCoord(ChunkSectionPos.getY(l)) + 16 - 1, ChunkSectionPos.getWorldCoord(ChunkSectionPos.getZ(l)) + j);
      			    lightProvider.updateLevel(Long.MAX_VALUE, ai, 15, false);
      		    }
      	    }
      	}
       }
      

       

      Also, there is no need to relight the new topmost lightmap after removing a lightmap (in fact, that doesn't even seem to be necessary with the current code).
      Removing those two pieces of code should already simplify the code by quite a bit and hence improve maintainability.

      Now, diving into some small optimizations that may squeeze out a bit of performance:

      • The optimization that empty subchunks are directly set to 15 and light is then manually spread to all sides, instead of letting the lighting engine propagate the light from the top face, can be applied to all lightmaps in the pseudo-empty region upon initial lighting, not only to the topmost lightmap.
      • Currently, the code pushes light from the pseudo-empty region from the chunk that is being lighted to the pseudo-empty region of all neighbors. In particular, in practice the topmost lightmaps are often on the same y-level. This causes light to be spread to a lightmap that will later be overridden with a value of 15 anyway. While this is not drastically bad, it is easy to optimize. Simply extend the above optimization to skip propagations from lightmaps in the pseudo-empty region to the pseudo-empty region of (not yet lighted) neighbor chunks. This optimization will not interfere with our invariant above, as it says that the pseudo-empty region of not yet lighted chunks can be opaque.
        Also, note that we can safely skip propagations to the pseudo-empty region of lighted neighbors, as they already have a skylight value of 15. In conclusion this means that we can skip propagations from the pseudo-empty region to the pseudo-empty region of all neighbors.

      Summary

      • Both described bugs can simply be solved by initializing topmost skylight maps of already lighted chunks to a value of 15 and not doing anything else. This doesn't interfere with the initial lighting code.
      • Lightmap initialization not changing values allows to get rid off some code that is currently needed to compensate for these changes. Hence the proposed solution will reduce code complexity.
      • There might be some bug with the current initial lighting code, depending on whether or not there can be block changes for chunks between pre_light and light stage. You can probably judge better than me whether this is a problem or not
      • Restricting the current lightmap initialization code solely to initial lighting allows moving it to a more fitting place and allows for some easy optimizations.

       

      Hope this is helpful
      Best,
      PhiPro

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              PhiPro Philipp Provenzano
              Votes:
              45 Vote for this issue
              Watchers:
              20 Start watching this issue

                Dates

                Created:
                Updated:
                CHK: