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

Queued lightmaps are ignored for skylight propagation and initialization

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Open
    • Resolution: Unresolved
    • Affects Version/s: 1.16.1, 1.16.2 Pre-release 1, 1.16.2, 1.16.4, 20w48a, 1.17.1
    • Fix Version/s: None
    • Labels:
      None
    • Confirmation Status:
      Confirmed
    • Category:
      Lighting
    • Mojang Priority:
      Important

      Description

      While the following issue might sound rather theoretical and not really practically relevant, it has some rather bad interaction with my proposed fix for MC-170010. Fixing MC-170010 without fixing this issue first basically causes all underground subchunks to become fully bright.

       

      The first part of the issue comes down to the fact that upon initialization of skylight-maps the search for a lightmap above the specified position (which will be used to inherit its values in case no lightmap was queued up for the specified position directly) only takes into account lightmaps already added to the world but ignores lightmaps that are queued up to be added at some later time.

      net.minecraft.world.chunk.light.SkyLightStorage.java
      protected ChunkNibbleArray createSection(long sectionPos) {
        ChunkNibbleArray chunkNibbleArray = (ChunkNibbleArray)this.queuedSections.get(sectionPos);
        if (chunkNibbleArray != null) {
           return chunkNibbleArray;
        } else {
           long l = ChunkSectionPos.offset(sectionPos, Direction.UP);
           int i = ((SkyLightStorage.Data)this.storage).columnToTopSection.get(ChunkSectionPos.withZeroY(sectionPos));
           if (i != ((SkyLightStorage.Data)this.storage).minSectionY && ChunkSectionPos.unpackY(l) < i) {
              ChunkNibbleArray chunkNibbleArray2;
              while((chunkNibbleArray2 = this.getLightSection(l, true)) == null) {
                 l = ChunkSectionPos.offset(l, Direction.UP);
              }
      
              return new ChunkNibbleArray((new ColumnChunkNibbleArray(chunkNibbleArray2, 0)).asByteArray());
           } else {
              return new ChunkNibbleArray();
           }
        }
      }
      

       

      This would be fine if there are either no queued lightmaps at all, i.e., the chunk has not been generated yet, or if every lightmap that needs to be initialized does have queued up data. However, saving chunks strips uninitialized lightmaps, i.e., those that were created via the last path of the above code return new ChunkNibbleArray(); and never received any light change.
      This leads to lightmaps that need to be initialized without having queued data upon reloading a chunk. If this initialization does not happen top-to-bottom, then this search for a lightmap above to inherit from can return the wrong lightmap, causing lighting glitches.
      And indeed this initialization order is unspecified. Whenever a chunk is promoted to the light stage, it will initialize all non-initialized lightmaps of itself and its neighbors that are near a non-empty subchunk. This initialization seems to happen bottom-to-top, but this is rather implementation dependent. This will however not necessarily load all lightmaps of the chunk or its neighbors, since some lightmaps might be loaded through a non-empty subchunk of a chunk not yet in the lighting stage, hence making the initialization order undefined.

      This primary bottom-to-top initialization then causes the lightmap creation code to hit the last codepath return new ChunkNibbleArray(); for most underground subchunks, which causes them to become fully bright with my fix for MC-170010.

       

      Let's now give some steps to visualize this issue in Vanilla.

      • Create a redstone-ready world with generator settings 16*minecraft:sandstone;minecraft:desert
      • Set the render distance to 2
      • Run the following commands
        /setworldspawn 1000 16 1000
        /fill 0 48 0 47 48 47 minecraft:sandstone
        /fill 16 31 16 31 31 31 minecraft:sandstone
        /fill 16 48 16 31 48 31 minecraft:air
        /fill 16 15 16 31 15 31 minecraft:stone
        /fill 0 0 0 47 7 47 minecraft:air
        /fill 0 8 0 47 15 47 minecraft:air replace minecraft:sandstone
        
      • Fly to chunk (-14 1)
      • Fly back to chunk (1 1) and observe that subchunk (1 0 1) is fully bright

       

      The explanation here is that upon reloading chunk (1 1) when flying back, the lightmap at (1 2 1) is already loaded by the neighbors and is completely bright, but no lightmap below is loaded yet. When loading chunk (1 1) the lightmap at (1 0 1) gets initialized without queued data, as the lightmap was uninitialized before saving, and inherits its values from the already loaded lightmap at (1 2 1) instead of the correct (1 1 1) which is not yet added to the world.

       

      A similar situation can be created when dealing with block changes near the edge of the loaded world.

      • Create a redstone-ready world
      • Set the render distance to 10 (or anything > 3)
      • Run the following commands
        /setworldspawn 1000 56 1000
        /fill 48 128 0 63 128 47 minecraft:stone
        /fill 64 112 0 79 112 47 minecraft:stone
        
      • Set the render distance to 2
      • Reload the world
      • /setblock 40 88 24 minecraft:stone
      • Fly to subchunk (3 5 1) and observe that it is too bright


       

      The issue here is that after reloading the world, the lightmap at (3 6 1) is not loaded, as it gets loaded by chunk (4 1) which is too far away, so that upon placing the stone block the lightmap at (3 5 1) gets initialized and inherits its values from (3 7 1) instead.

      While this might sound similar to MC-170012, note that fixing MC-170012 will only move the issue by one chunk, but moving every step by 1 chunk in +x direction would still work as all block operations would still be in the accessible region. The fundamental issue of not taking into account queued up lightmaps is not solved by MC-170012.

       

      The second related part of the issue was already mentioned in the discussion of MC-170010 in regards to the vanilla data retainment mechanism. The issue mentioned there is that the skylight optimization simply pierces through such queued lightmaps that are not actually added to the world without changing their light values. When then later on readding the lightmap to the world these missing light changes will result in a lighting glitch. While the discussion of MC-170010 was mostly concerned with this effect for initial lighting, we can again create a similar situation when dealing with block changes near the edge of the loaded world. Again this scenario is unaffected by MC-170012.

      • Create a redstone-ready world
      • Set the render distance to 10 (or anything > 3)
      • Run the following commands
        /setworldspawn 1000 56 1000
        /fill 32 112 0 63 112 47 minecraft:stone
        /setblock 72 88 24 minecraft:stone
        
      • Set the render distance to 2
      • Reload the world
      • /fill 32 112 16 47 112 31 minecraft:air
      • Fly to subchunk (3 5 1) and observe that it is too dark


       

      Again the lightmap at (3 5 1) is not loaded after reloading the world. When clearing the stone platform at (2 7 1) the skylight optimization then pierces through (3 5 1) without changing its values and then later on the old unchanged lightmap is added.

       

      The solution to this problem sounds rather straightforward: Just take these queued lightmaps into account for lightmap creation and skylight optimization.

      There is however an issue. As mentioned above, the code currently (wrongly) creates most lightmaps unitialized, i.e., using the last codepath of createSection(...), and hence they take up no memory (or at least far less than the usual 2 kB for initialized lightmaps). After solving this bug, these lightmaps will then be created by inheriting the correct values from the lightmap above (which will be mostly 0). This will cause them to take up the full 2 kB of memory albeit still being mostly 0. One solution would then be to check for this case and leave the lightmaps unitiailized if this occurs.

       

      I propose to instead implement my suggested solution for MC-196725 which automatically contains a fix for the bug discussed here. This will decouple the lightmap handling from the current skylight-optimization distance tracking, and hence naturally places all lightmaps into a single datastructure rather than splitting them into two. Furthermore, this will solve this last concern about memory consumption in a more general way, as lightmaps will only be created when really needed, and will also clean up empty lightmaps more aggressively.

      Best,
      PhiPro

        Attachments

        1. 2020-07-31_12.06.48-1.png
          2020-07-31_12.06.48-1.png
          339 kB
        2. 2020-07-31_12.59.57.png
          2020-07-31_12.59.57.png
          250 kB
        3. 2020-07-31_13.00.34.png
          2020-07-31_13.00.34.png
          162 kB
        4. 2020-07-31_13.20.37.png
          2020-07-31_13.20.37.png
          366 kB
        5. 2020-07-31_13.21.05.png
          2020-07-31_13.21.05.png
          189 kB

          Issue Links

            Activity

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              PhiPro Philipp Provenzano
              Votes:
              14 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                CHK: