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

Small cleanup for skylight propagation code

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Resolved
    • Resolution: Fixed
    • Affects Version/s: 1.16.1, 1.16.2 Pre-release 1, 1.16.2
    • Fix Version/s: 20w45a
    • Labels:
      None
    • Confirmation Status:
      Plausible
    • Category:
      Lighting

      Description

      Just some small suggestions to improve readability and reduce complexity of the skylight propagation code.
      While not being bugs on their own, some of these vanilla code pieces cause issues with my other bugfixes, especially MC-170010 and MC-196725, so it is very convenient to clean these code pieces up before fixing the other bugs.
      I collect these cleanups here separately as I don't want to cluttter the other reports any further.

      First of all, quite a bit of complexity in ChunkSkyLightProvider.getPropagatedLevel(long src, long dst, int level) comes from the fact that the skylight optimization code calls it with non-adjacent src and dst positions. (Concretely, propagateLevel(long src, int level, boolean brighten) skips downwards propagations into empty sections and instead directly propagates its values to the next non-trivial section below and all neighbors on the way, using the original src position for the call to getPropagatedLevel(...), or rather to LevelPropagator.propagateLevel(...) which in turn calls getPropagatedLevel(...).)

      This three-way propagation then causes the following code inside getPropagatedLevel(...)

      net.minecraft.world.chunk.light.ChunkSkyLightProvider.java
      if (direction2 != null) {
        ...
      } else {
        voxelShape = this.getOpaqueShape(blockState2, sourceId, Direction.DOWN);
        if (VoxelShapes.unionCoversFullCube(voxelShape, VoxelShapes.empty())) {
      	 return 15;
        }
      
        int r = bl ? -1 : 0;
        Direction direction3 = Direction.fromVector(o, r, q);
        if (direction3 == null) {
      	 return 15;
        }
      
        VoxelShape voxelShape4 = this.getOpaqueShape(blockState, targetId, direction3.getOpposite());
        if (VoxelShapes.unionCoversFullCube(VoxelShapes.empty(), voxelShape4)) {
      	 return 15;
        }
      }
      

       

      I propose to change this convention and pass the block below src at the appropriate y-level to LevelPropagator.propagateLevel(...), i.e., the neighbor of dst that is below the original src, so that getPropagatedLevel(...) receives adjacent src and dst positions. This will completely eliminate all the code associated to the three-way propagation.
      This convention actually looks semantically more meaningful to me, but I guess that's not really a valid reason to change things

      Note that this convention is in fact slightly more optimal for ChunkSkyLightProvider.recalculateLevel(long pos, long neighbor, int neighborLevel) (which gets passed the contribution of one neighbor). Currently, in case this neighbor is not directly adjacent, when processing the contribution from the corresponding direction the code first has to search for the next non-empty section above, just to come to the conclusion that this contribution is already provided. With the proposed change the code could skip this neighbor directly.

      This change needs to be implemented both in propagateLevel(...) and recalculateLevel(...).

       

      Next, recalculateLevel(...) manually implements the light lookup for neighbors without an associated lightmap. This code already exists in getLight(...), which currently only handles the main-thread light lookup, using the uncached lightmaps.
      I propose to move this code to a common place, so both main thread and lighting engine can use it with the appropriate set of lightmaps. recalculateLevel(...) can then employ this instead of implementing everything on its own.
      This whole codeblock

      for(m = BlockPos.removeChunkSectionLocalY(m); !((SkyLightStorage)this.lightStorage).hasSection(n) && !((SkyLightStorage)this.lightStorage).isAtOrAboveTopmostSection(n); m = BlockPos.add(m, 0, 16, 0)) {
         n = ChunkSectionPos.offset(n, Direction.UP);
      }
      
      ChunkNibbleArray chunkNibbleArray4 = ((SkyLightStorage)this.lightStorage).getLightSection(n, true);
      if (m != excludedId) {
         int p;
         if (chunkNibbleArray4 != null) {
      	  p = this.getPropagatedLevel(m, id, this.getCurrentLevelFromSection(chunkNibbleArray4, m));
         } else {
      	  p = ((SkyLightStorage)this.lightStorage).isSectionEnabled(n) ? 0 : 15;
         }
      

      then simply becomes

      if (m != excludedId)
          p = this.getPropagatedLevel(m, id, this.getLight(m));
      

       

      Note that this uniform treatment also fixes the bug mentioned in this comment.

      I think one reason why this manual lookup is currently needed is due to the convention about src and dst position discussed above, so the code currently wants to know where the ligth data comes from. This is no longer the case with the convention proposed above.

       

      Related to the previous point is the fact that recalculateLevel(...) excludes contributions from above if there is no lightmap directly associated to the neighbor. Instead, it invokes getPropagatedLevel(...) with a dummy position Long.MAX_VALUE, which in turn contains some additional logic to handle this source skylight.
      I propose to simply include contributions from above in the uniform treatment discussed in the previous point and remove the logic handling this special value Long.MAX_VALUE from getPropagatedLevel(...), simplifying the code again.

      In my opinion it is actually more meaningful to think of skylight as always coming from above, with the source infinitely high up in the sky. Rather than the source sitting at the topmost lightmap which changes over time.

      Also note that the current code is actually slightly wrong. The source skylight contribution only is 15 for the topmost block of the topmost lightmap, otherwise it is 0. That means that a block with no lightmap directly above will not get any source skylight contribution, as soon as there is some lightmap above, even if the block is directly exposed to skylight, i.e., has no obstacle above. Since recalculateLevel(...) in this case excludes the contribution from above, the resulting value will actually be wrong. This issue doesn't really show up, though, since recaclculateLevel(...) is invoked in sufficiently rare situations that manage to spread the correct value through some other means.

      I also want to mention that this code causes some slight issue with my proposed solution for MC-196725, since the code handling this source skylight contribution in getPropagatedLevel(...) assumes that the topmost block of the topmost lightmap is air. This is true for the current vanilla code, but not for my proposed solution of MC-196725. The uniform treatment discussed above avoids this issue.

       

      Finally, some small cleanup slightly unrelated to the rest.
      ChunkLightProvider.resetLevel(long pos) is used to recalculate the light value at a position, e.g. after a block change. In case there is no associated lightmap at the specified position, ChunkSkyLightProvider.resetLevel(long pos) will delegate this to the next position above that does have an associated lightmap.
      This does not really make any sense to me. If there is no lightmap, simply skip the action, as is already done by ChunkLightProvider.resetLevel(...). If there was any need to recalculate the light level at the delegated position, it would have received a request on its own.
      One reason I could think of for this code to exist is to force the block above to re-emit its light, hiding some effects of MC-169913 or MC-170010. But that's just some wild speculation. In that case, this code can be removed after fixing the other bugs.

       

      Best,
      PhiPro

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              mgatland [Mojang] Matthew Gatland
              Reporter:
              PhiPro Philipp Provenzano
              Votes:
              6 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                CHK: