Sky-lightmaps not properly initialized
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.
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
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-170012has 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
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
Linked Issues
relates to10
MC-198987Stripping uninitialized lightmaps causes data lossConfirmedMC-199966Lightmaps for chunks before light stage get erased upon reloadingConfirmedMC-148580Server lighting still broken in 1.14 pre-4FixedMC-261857Using the "/setblock", "/fill", or "/clone" commands to create little amounts of blocks in completely isolated areas causes large client-side stuttersFixedMC-162253Lag spike when crossing certain chunk bordersFixedMC-169498Empty top subchunks don't update skylight in some casesFixedMC-170012Lightmaps are missing for initial skylightFixedMC-196542Small cleanup for skylight propagation codeFixedMC-196614Queued lightmaps are ignored for skylight propagation and initializationAwaiting ResponseMC-196725Lightmaps get lost upon unloading chunksAwaiting Response
Created Issue:
Sky-lightmaps not properly initialized
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 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.javaprotected 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.
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 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 ligth 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. 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 earlier: Don't change light values by creating lightmaps, but initialize them to the 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 additional light and darkness propagations in order to compensate for the changed values.
Concretely, this means to initialize topmost lightmaps of already lighted
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 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-? 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.
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, wheres 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 get scheduled for initialization inside
net.minecraft.world.chunk.light.SkyLightStorage.javaprotected 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); } }This basically pushes skylight to the topmost lightmap from above 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-? 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 independently 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 percisely 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 {{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.
Now, I hope that my suggested fix is now sufficiently convincing
Finally, let's discuss some simplifications and otimizations that are now possible:
Since the 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 previously topmost lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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 lightmaps (in fact, that doesn't even seem to be necessary with teh current coe).
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 to the above optimization to skip propagations from lightmaps in the pseudo-empty region to the pseudo-empty region of (not yet lighted) neighbor chunks.
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 rhat we can skip propagation 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 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 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 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
Attachments3
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 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.javaprotected 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.
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 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 ligth 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. 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 earlier: Don't change light values by creating lightmaps, but initialize them to the 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 additional light and darkness propagations in order to compensate for the changed values.
Concretely, this means to initialize topmost lightmaps of already lighted
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 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-? 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.
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, wheres 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 get scheduled for initialization inside
net.minecraft.world.chunk.light.SkyLightStorage.javaprotected 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); } }This basically pushes skylight to the topmost lightmap from above 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-? 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 independently 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 percisely 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 {{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.
Now, I hope that my suggested fix is now sufficiently convincing
Finally, let's discuss some simplifications and otimizations that are now possible:
Since the 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 previously topmost lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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 lightmaps (in fact, that doesn't even seem to be necessary with teh current coe).
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 to the above optimization to skip propagations from lightmaps in the pseudo-empty region to the pseudo-empty region of (not yet lighted) neighbor chunks.
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 rhat we can skip propagation 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 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 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 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,
PhiProThis 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.javaprotected 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 ligth 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. 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 earlier: Don't change light values by creating lightmaps, but initialize them to the 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 additional light and darkness propagations in order to compensate for the changed values.
Concretely, this means to initialize topmost lightmaps of already lighted
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 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-? 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.
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, wheres 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 get scheduled for initialization inside
net.minecraft.world.chunk.light.SkyLightStorage.javaprotected 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); } }This basically pushes skylight to the topmost lightmap from above 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-? 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 independently 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 percisely 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 {{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.
Now, I hope that my suggested fix is now sufficiently convincing
Finally, let's discuss some simplifications and otimizations that are now possible:
Since the 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 previously topmost lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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 lightmaps (in fact, that doesn't even seem to be necessary with teh current coe).
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 to the above optimization to skip propagations from lightmaps in the pseudo-empty region to the pseudo-empty region of (not yet lighted) neighbor chunks.
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 rhat we can skip propagation 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 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 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 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
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.javaprotected 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 ligth 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. 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 earlier: Don't change light values by creating lightmaps, but initialize them to the 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 additional light and darkness propagations in order to compensate forthechanged values.
Concretely, this means to initialize topmost lightmaps of already lighted
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 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-? 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.
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, wheres 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 get scheduled for initialization inside
net.minecraft.world.chunk.light.SkyLightStorage.javaprotected 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); } }Th
isbasically pushes skylight to the topmost lightmapfrom aboveand 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-? 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
entlyof 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 percisely 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 {{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.
Now,I hope that my suggested fix is now sufficiently convincingFinally, let's discuss some simplifications and otimizations that are now possible:
Since the 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 previouslytopmost lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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 lightmap
s(in fact, that doesn't even seem to be necessary with teh current coe).
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
tothe above optimization to skip propagations from lightmaps in the pseudo-empty region to the pseudo-empty region of (not yet lighted) neighbor chunks.
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 meansrhat we can skip propagation 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 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 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 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,
PhiProThis 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.javaprotected 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 ligth 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. 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
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-? 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.javaprotected 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-? 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 percisely 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 otimizations 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 lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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
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.javaprotected 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 ligth 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. 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
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-? 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.javaprotected 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-? 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 percisely 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 otimizations 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 lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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,
PhiProThis 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.javaprotected 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 ligth 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. 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
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-170012to 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.javaprotected 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-170012has 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 percisely 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 otimizations 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 lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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
relates to
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.javaprotected 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 ligth 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.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
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-170012to 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.javaprotected 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-170012has 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 percisely 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 otimizations 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 lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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,
PhiProThis 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.javaprotected 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 ligth 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 alsoMC-169913plays 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
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-170012to 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.javaprotected 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-170012has 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 percisely 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 otimizations 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 lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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
![[Mod] Michael Wobst](../../../data/avatars/users/37868.png){kind=small}
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.javaprotected 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 ligth 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 alsoMC-169913plays 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
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-170012to 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.javaprotected 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-170012has 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 p
ercisely 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 otimizations 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 lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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,
PhiProThis 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.javaprotected 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 alsoMC-169913plays 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
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-170012to 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.javaprotected 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-170012has 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 lightmapnet.minecraft.world.chunk.light.SkyLightStorage.javathis.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
![[Mojang] slicedlime](../../../data/assets/gravatar/slicedlime.jpeg){kind=small}
![[Mojang] Adrian Östergård](../../../data/assets/gravatar/nighter.jpeg){kind=small}
Important![[Helper] Johnibur](../../../data/avatars/users/28381.png){kind=small}
![[Mod] violine1101](../../../data/avatars/users/92858.png){kind=small}
relates to
relates to
Still in 1.16.1 and 20w29a
relates to
relates to
relates to
![[Mod] Jarl-Penguin](../../../data/avatars/users/92849.png){kind=small}
relates to
relates to
![[Mojang] Gegy](../../../data/avatars/users/10122.svg){kind=small}
[Mojang] Gegyrelates to
![[Bot] Piston](../../../data/avatars/users/31202.png){kind=small}
An analysis and solution is given in MC-170010.
This is simply a rendering issue: the bottom chunk is not marked for rerendering. Causing any block update in that chunk or reloadong the world will fix the issue.
The actual skylight as shown in F3 is correct.
Took me quite a while to fit all the puzzle pieces together, but I finally have a satisfying explanation for this bug with all its weird features.
Basically, it is caused by MC-170010 and my proposed fix for it will fix this issue as well.
Let me first explain what is causing the lagspikes. It is basically the same lagspike as experienced in the following issue:
- Create a new redstone-ready world
- /setblock 7 103 7 minecraft:glass
- Experience a clientside lagspike
This lagspike occurs whenever a block is placed high above the ground, where "ground" really means the topmost block of all neighbor chunks. As soon as there is only a single block above, the lag spike disappears. For example, if you /setblock 7 255 7 minecraft:glass then the above steps won't cause any lagspike.
The reason for this is MC-170010. As mentioned there, topmost skylight maps are not properly initialized to their previous values. Instead, they are created completely dark and then get relighted. Placing a block high above ground creates 27 new lightmaps. Due to the order of creating them bottom to top, all of them will be uninitialized and have to be relighted. So, although we place a glass block, which doesn't cause any light change at all, the client has to relight 27 subchunks at once due to this bug, which takes quite some time.
As soon as there is any block above, no (or fewer) lightmaps get created, hence avoiding the issue.
So, my proposed fix for this issue is my proposed fix for MC-170010.
While not really relevant for the solution, I think it is still interesting to explain why the bug doesn't occur in many situations and why it is related to spawnchunks and is also stateful. So, let's start by explaining how the previous lag spike relates to the one of this bug report.
The following easy setup is sufficient for triggering it:
- Create a new redstone-ready world
- Set the render distance to 2 (or 3)
- /setblock 7 103 7 minecraft:glass
- Move exactly 4 chunks in any cardinal direction, e.g. move to chunk (4 0)
- Move 1 chunk back, e.g. to (3 0), and experience a lag spike
When you move back from (4 0) to (3 0), the chunk (0 0) gets loaded on the client. This causes the lightmaps to be created around subchunk (0 6 0) and hence leads to the exact same issue as previously described. Namely, those lightmaps are created uninitialized and have to be relighted all at once, which costs too much time. (Actually, in this case not all of the 27 lightmaps are directly initialized, but only those in the already loaded chunks. Nevertheless, this seems to be enough to lag the client quite bad.)
Why does it happen only for certain chunk borders and not all of the time?
First of all, the issue relies on lightmaps being created upon loading chunks on the client. The lightmaps created for the newly loaded chunk are in fact initialized, namely to the data the client receives from the server. So those don't contribute. (Actually, the topmost lightmap of the newly loaded chunk is still thrown away and relighted in any case. But this single lightmap seems to have sufficiently small impact to not cause lag spikes all of the time. Nevertheless, this unnecessary relighting would be eliminated with my proposed solution as well.) So, a necessary condition for the issue is that the newly loaded chunk is much higher than its already loaded neighbors.
Nevertheless, even in this case, there are still a lot of awkward situations that avoid the bug:
- The bug doesn't occur if you move e.g. to (5 0) and then to (3 0)
- or if you reload the world at (4 0).
- Also, if you replace the block by air and then by glass again in the above steps, the issue doesn't occur either.
- If you move the spawnchunks far away from (0 0) the issue doesn't occur.
These strange stateful features of the bug were really giving me some headaches while hunting it down The block isn't loaded on the client while at (4 0), so replacing it by air and then glass again shouldn't have any effect on the client state, yet it turns out that it avoids the lag spike.
The easiest case is what happens if you reload the world at (4 0). In this case, when the server sends the data for chunk (1 0), it also sends the lightmaps for (1 5 0), (1 6 0) and (1 7 0). Similarly for (1 -1) and (1 1). Those lightmaps are not directly added to the world, since there is no nearby block yet. Instead they are kept queued. Once we move to (3 0), the block at (0 6 0) creates all the surrounding lightmaps. Now, in this case, the lightmaps for (1 -1), (1 0) and (1 1) are not uninitialized, but are initialized from the queued up data that the client received from the server. That is why the bug does not occur in this case.
If we now move back to (4 0), the block at (0 6 0) and hence all the surrounding lightmaps get unloaded. Now, the neighbor chunks do not anymore have any queued up data from the server, and the server won't resend the data unless we move even further away to (5 0). So, when now crossing the chunk border to (3 0) once again, the lightmaps for chunk (1 -1), (1 0) and (1 1) will now indeed be uninitialized and have to be relighted, causing the lagspike.
What happens if you replace the block by air and then glass again while at (4 0)?
When replacing the block by air, the server deletes all the lightmaps. When afterwards spawning the glass block again, the server encounters exactly the same issue of MC-170010 and has to relight the uninitialized lightmaps. This will be detected as ligth changes, although the final light values actually haven't changed. Now, some other piece of code comes into play.
public void flushUpdates(WorldChunk worldChunk) { if (this.blockUpdateCount != 0 || this.skyLightUpdateBits != 0 || this.blockLightUpdateBits != 0) { ... if (this.skyLightUpdateBits != 0 || this.blockLightUpdateBits != 0) { this.sendPacketToPlayersWatching(new LightUpdateS2CPacket(worldChunk.getPos(), this.lightingProvider, this.skyLightUpdateBits & ~this.lightSentWithBlocksBits, this.blockLightUpdateBits & ~this.lightSentWithBlocksBits), true);
What it does is to sync all light updates at the edge of the client's view area. Hence, the (wrongly) detected light updates cause the server to sync the lightmaps for (1 -1), (1 0), (1 1) to the client again, which basically puts us in the same starting point as in the previous case. Those synced lightmaps are used for initialization and hence the issue doesn't occur.
Finally, let us understand the role of spawn chunks. As it turns out, this is due to yet another effect, namely that the chunk tracking distance is 1 larger than the chunk loading distance. Concretely, this means the following:
- The chunk at (0 0) is unloaded from the client upon entering chunk (4 0) (the chunk tracking distance is capped at >= 3)
- Since the chunk loading distance by players is basically one less, i.e. 2, the chunk at (1 0) is already "unloaded" serverside at that time, but still loaded clientside. "Unloaded" means that it is demoted to a border chunk, so it is still loaded in some sense, but it is non-ticking and won't be sent to clients; however, it will be kept loaded on the client if already present.
- When moving to (3 0) the chunk at (1 0) becomes "loaded" again on the server, i.e. it is promoted to a ticking chunk.
- At this point, it will be resent to the client.
- The chunk at (0 0) will only be sent to the client upon moving to (2 0).
- However, as before, the client now has queued light maps, so the lag spike does not occur.
So the difference between spawn chunks and non-spawn chunks is that they are kept loaded on the server and hence shade this discrepancy between tracking and loading distance. This also means that the same rules apply to chunks loaded by any other means, e.g. other players.
Further remarks
As another suggestion, it might be useful to run the client lighting engine offthread as well, as is already done for the server. Of course, you want some way for the client thread to wait for light updates to finish before rendering, as even the slightest latency of light updates near the player usually feels very annoying. Such a way to wait for light updates to finish is needed anyway for MC-164281. Additionally, you probably also want to implement a fix for MC-177685 such that updates near the player can be prioritized and the client thread only has to wait for those, rather than all pending updates. Nevertheless, even without a fix for MC-177685, putting the clientside lighting engine offthread can help mitigating this issue further.
As a final remark, I really strongly recommend to implement my proposed solution for MC-170010 instead of putting more and more band-aid on it, as it makes it increasingly difficult to track down issues. This one already gave me a lot of headaches as it had so many strange fatures related to different code paths. (Admittedly, the challenge was also kindof fun )
Workaround for players
As a temporary workaround for player, you can place a ceiling of minecraft:barrier at y=255 above the whole map. As can be seen from the analysis, the issue arises from a height difference of neighbor chunks. Putting a ceiling above the world eliminates any height difference and reduces the issue to its minimal intensity of flat worlds. Putting the ceiling only above a small part of the world will simply push out the problematic chunk boundaries to the end of the ceiling, so for this workaround to work you need to cover all of the area you visit. While this is definitely not perfect, it should at least help in the case when you are moving in already generated chunks and not visiting new ones.
Best,
PhiPro
As a proof of concept and as a temporary workaround for players, I have implemented my proposed solution for MC-170010 as a fabric mod. As explained above, this also solves this performance issue. The code can be found at https://github.com/PhiPro95/mc-fixes/tree/mc-170010. Note that this requires the fabric mod loader.
Edit: Due to MC-196614 you also require https://github.com/PhiPro95/mc-fixes/tree/mc-196725 in order to avoid amplified versions of that bug.
A complete bundle of all required mods can be downloaded here
Also note that it is important to have this mod installed on the client in order to fix the lag spikes. (However, in order to fix MC-170010 itself, it is mainly important to have it installed on the server).
Best,
PhiPro
There is also a related issue for blocklight. As mentioned in the discussion of MC-170010 ProtoChunk.setBlockState(...) schedules light checks for chunks that have passed the features generation stage, suggesting that such block updates are indeed possible. Looking at the worldgen code also suggests that this is possible since the features generation stage has access to an 8 chunk radius.
The problem is now that such a block change could place a light source and the scheduled light check would then propagate the light too early. In contrast to the original report about skylight, lightmaps do exist for the neighbors, since they will be created due to the placed blocklight source (well, not quite due to the second issue below). However, the lighting engine treats chunks before the features stage as opaque, so the light propagation can get stuck at the chunk boundary nevertheless. So the end-result is similar to the original report, namely that light propagation can get stuck if carried out too early, i.e., before the neighbors are ready, altough the underlying cause is slightly different in this case.
Note that this version for blocklight cannot be as easily visualised by relighting the world using erase cached data as it relies on blocks being placed between the features and light generation stage.
The fix for this is again quite easy. Just let the lighting engine check if a chunk already has its initial lighting done, and otherwise ignore the luminance of a block for light calculations. This way, blocks don't emit light by themselves before the initial lighting and are properly turned on during initial lighting, similar to my proposed solution for initial skylight in MC-170010.
Note that you cannot simply remove the light checks that cause the early propagation. Initial lighting spreads light too all neighbors, which are only guaranteed to be in pre_light stage at that point. Any block changes thereafter need to schedule light checks as otherwise you can have missing or ghost-contributions to those neighbors. When later on those neighbors are Initially lighted they will not be completely relighted but only add their own blocklight-sources. So, you need any lighting information to stay consistent after the pre_light stage and hence you need these light checks.
Also note that you should not fix this bug by not treating chunks before the features stage as opaque. Leaving in the early propagation of blocklight would then cause light propagations into chunks before the features stage and hence the above discussion would also apply to those, so you would need to schedule light checks before the features stage as well. However, for performance reasons this is not desired.
There is another issue related to block changes after the features stage. ProtoChunk only schedules light checks upon block changes (for chunks after the features stage). but it does not handle lightmap creation and removal. Concretely, it misses the calls to LightingView.updateSectionStatus(BlockPos pos, boolean status) that are present in WorldChunk and World. In view of MC-169913 lightmap creation schould be scheduled before the block change and lightmap removal should be scheduled after the light checks.
Let me know if I should post these as separate bug reports.
Could this have anything to do with light updates / involve the light engine, lightmap somehow?
Just a very wild guess, but I recall such lag issues with moving and set blocks or structures since many years, and excluding that (also due to fairly more recent changes with rendering) might help a bit (again, no guarantee).
Essentially something in the direction of MC-170010, but potentially I'm completely off here.
Edit: I just saw you got a glass floor, and glass can lag like crazy.
I got first hand experience with that from a relatively recent MC Infinity client with a glass floor dimension, as well as years ago with the Cubehamster/Sethbling map that we playtested back then before its release.
It spawns moving flying machines, and there was also lots of (dyed) glass involved; we fixed the lag back then by adding a roof over the gaming field, and the resulting darkness/shadow by giving the players nightvision.
When you try to reproduce the lag / FPS drop, maybe it'd be good to test in a redstone-ready world, no glass; you can also test with a layer of solid blocks above the testing ground..
Edit 2: There have been issues in the past if 64 blocks or more changed in one chunk in the same tick or something like that; I recall such issues, see e.g. MC-123304 (and MC-108358 was with 32).
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(...)
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
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.
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
The lighting engine creates lightmaps for exactly those chunks that have any nearby (i.e., in a radius of 1 subchunk) non-air blocks and deletes them once the last nearby subchunk becomes empty. As a consequence, unloading a chunk, which marks all its subchunks as empty, will delete ligtmaps of still loaded neighbors if there are no other non-empty subchunks around to keep the lightmaps alive.
This can be visualized via the following steps:
- Create a redstone-ready world
- Set the render distance to 2
- Run the following commands
/setworldspawn 1000 56 1000 /fill 16 80 0 16 95 15 minecraft:stone /setblock 16 88 8 minecraft:sea_lantern
- Fly to chunk (-14 0)
- Fly back to chunk (0 0) and observe that the lightmap for subchunk (0 5 0) got erased
There is also a variation for skylight
- Create a redstone-ready world
- Set the render distance to 2
- Run the following commands
/setworldspawn 1000 56 1000 /fill 0 128 0 47 128 47 minecraft:stone /fill 32 80 16 32 127 31 minecraft:stone /fill 33 128 16 47 128 31 minecraft:air /fill 32 96 16 32 96 31 minecraft:air
- Fly to chunk (-13 0)
- Fly back to chunk (1 0) and observe that the lightmap for subchunk (1 5 1) and (1 6 1) got erased
The vanilla code contains some data retainment mechanism that puts lightmaps back to the queued lightmaps, rather then deleting them. However, this mechanism is disabled upon promoting a chunk to the light stage.
One possible solution for this issue would be to reenable this retainment mechanism once a neighbor chunk gets unloaded.
I propose an alternative to this data retainment mechanism. This will decouple the lightmap handling from the skylight-optimization distance tracking that currently controls the lightmaps. Hence this approach will naturally avoid the bug discussed here and make the vanilla data retainment mechanism obsolete.
Furthermore, this approach will naturally contain a complete fix for MC-196614 and provide a more aggressive cleanup for trivial lightmaps, compared to the current vanilla code.
- The main idea is to create lightmaps on demand, i.e., when the lighting engine wants to set a light value, and delete them once they become trivial in the appropriate sense. When there is no lightmap associated to a subchunk, then skylight is inherited from the next lightmap above or direct skylight if there is none, whereas blocklight is simply 0. In order to check when a lightmap is trivial we need to track some complexity measure that can be easily updated upon chaning light values and is 0 iff the lightmap is trivial, i.e., if it coincides with the values that would be produced with no lightmap present.
- For example, we can simply take the sum of all blocklight values and for skylight we can for each position in the subchunk take the absolute value of the difference between the light value at that position and the light value at the position above (where in case of the topmost layer the light value at the position above needs to be taken from the lightmap above or direct skylight) and then sum up all these values. Note that in case of skylight this complexity measure not only depends on the lightmap but also on its position in the world.
- One important point is now that creating and removing trivial lightmaps does not change any light values, making the lightmap handling completely transparent to the lighting propagator. This also means that newly created lightmaps will always have a complexity of 0. This does however require
MC-170010to be fixed first, as otherwise some lightmaps will not be properly initialized upon creation, causing a change of light values and hence messing around with the light propagator. This causes a bit of a cyclic dependency between the two bug reports, so they should ideally be fixed simultaneously.
- The skylight optimization is then applicable to subchunks that are not near any non-air blocks, as determined by the current distance tracking, and that don't have an associated lightmap. This second condition that having a non-trivial lightmap disables the skylight optimization basically takes care of the second part of
MC-196614. - Note that changing a light value at the bottom of a lightmap will cause subchunks below without an associated lightmap to automatically change their light value as well, since that is how missing lightmaps are handled in the light lookup. This means that before changing a light value at the bottom of a lightmap, we first have to find the next subchunk below for which skylight optimization is not applicable and create a lightmap for it, in case it does not already have one, fixing the old light value so that changes are then properly propagated through the usual skylight optimization.
- It is convenient to forcefully disable any light propagations to chunks before the pre_light stage (using terminology of
MC-170012), allowing to defer the complexity initialization until the promotion to the pre_light stage. When unloading chunks, propagations should then be forcefully disabled again, so that the unloaded region is very well controlled. Ideally, this manual mechanism should not be necessary as any violation of it is always accompanied by some lighting glitch. However, in view ofMC-164281it might be a good idea to take some precautions and avoid further bugs caused by screwed complexity trackings. - Furthermore, for the unloaded region lightmaps can be directly added to the world as nothing will be interacting with them, given that we have just forcefully disabled all such interactions. Lightmaps getting queued for already loaded chunks, i.e., on the client, might be better placed in some queue first, so they can be added at a more controlled point in time. Furthermore, when adding such a lightmap to an already loaded chunk its complexity tracking has to be reevaluated and if any value at the bottom changes, the necessary steps need to be taken for skylight, as explained above.
- Removal of trivial lightmaps should only be done once every update cycle or upon saving or something similar, in order to avoid rapidly removing and reallocating lightmaps
One concern that might come up is regarding the interaction between skylight optimization and unloaded chunks. More concretely, note that the last accessible chunk is in the full state but its neighbors are only guaranteed to be in pre_light state. Hence there can be light updates into chunks that are only in pre_light state and their neighbors are not guaranteed to be loaded at all. So the issue one might see here is that there are light updates into chunks that do not have complete information about their neighbors, so the skylight optimization might be applied in situations where it shouldn't. We will in the following sketch a proof that the extra condition that the skylight optimization is disabled for subchunks with an associated lightmap is in fact sufficient to ensure correct results. Note that some similar discussion could have been already placed under MC-170012 or MC-196614, but I think it fits here nicely as well. Of course one could simply avoid this whole discussion by just increasing the chunk loading distance by 1, so that light updates only take place in chunks that are in full state; however that would be rather inefficient given that this is actually not an issue.
For the following discussion we will assume MC-170012 to be fixed and use its terminology. We will assume from the chunk loading mechanism that whenever a block change in a chunk c1 could potentially cause a light update into chunk c2 (or some boundary touching it) then c2 should be loaded in pre_light stage. Note that any violation of this will result in a lighting glitch simply because light updates would get stuck at chunk boundaries, so this would be a completely unrelated issue on its own. More technically, we can state this condition as follows: If there occurs a block update in some chunk c1 and we are given two neighbors c and c2 of it (where we also count diagonal neighbors and also say that a chunk is a neighbor of itself) such that they are also neighbors to one another and such that c has its initial lighting done, i.e., was generated into the light stage but is not necessarily loaded currently, then c2 should be loaded in pre_light stage.
Note that this assumption might not be true on the client. However there are other mechanisms taking care of such effects for the client light syncing, so we don't want to worry about this here.
We want to show that the end result of our lighting model is correct, for which it suffices to show local correctness, i.e., each block has a light value that is precisely the maximum over all contributions of its neighbors, where the contributions are specified in the ideal lighting model where no skylight optimization takes place and all chunks are loaded. It is always true that subchunks for which no skylight optimization is applied are locally correct and also unloaded chunks retain their local correctness since by the assumption above no block directly adjacent to an unloaded chunk changes its light value. Hence it is enough to show local correctness for subchunks for which skylight optimization is applied.
So, for sake of concreteness assume that skylight optimization is applied to subchunk (0 0 0) for which we want to show local correctness. We may assume inductively that all subchunks at y-level >= 1 are locally-correct.
Consider some horizontal chunk border of it, e.g., the border to chunk (-1 0), consisting of the blocks (0 0 0)..(0 15 15). If no light value for a block adjacent to this border changed, i.e., for no block in the region (-1 -1 -1)..(1 16 16) then it retains its local correctness and there is nothing to check for this border. Otherwise, such a light change must come from a block change in a chunk from the region (-1 -1)..(0 1) and the skylight source propagated through this block change must lie in the same chunk region and furthermore this skylight source and the block change must lie in chunks that are neighbors of one another. Hence we can conclude by the assumption about chunk loading that some quadrant containing the border is loaded in pre_light stage, i.e., the quadrant (-1 -1)..(0 0) or (-1 0)..(0 1). By the specification for the skylight optimization we then know that the subchunks of this quadrant that lie in the region (-1 -1 -1)..(1 1 1) contain only air blocks. Similarly, we conclude that a corner-column of subchunk (0 0 0) either retains local correctness or the unique quadrant containing the column in its interior is loaded in pre_light stage and the intersection with (-1 -1 -1)..(1 1 1) consists purely of air blocks.
Consider now the region consisting of all such quadrants intersected with (-1 -1 -1)..(1 1 1). This region then has the following properties which allow to deduce quite a lot of information about the lighting model:
- It is star-shaped with respect to chunk (0 0)
- It contains only air blocks
- Its boundary can be decomposed into 3 parts:
- The top faces
- The faces that are one subchunk away from (0 0 0) (excluding the top faces)
- The faces touching (0 0 0)
- The faces and corner-columns of (0 0 0) that also belong to the boundary of this region retain local correctness, so there is nothing to check for those
We need to show local correctness for those blocks in (0 0 0) that are strictly inside this region, i.e., excluding those borders for which we already know it. We now consider two lighting models: The real model applying the skylight optimization according to our specification, where we replace propagations at y-levels >= 16 with ideal ones, as we already know local correctness in that area by induction. And the semi-ideal model that additionally treats subchunk (0 0 0) as non-optimizable, i.e., uses ideal propagations. We will show that both models give the same solution with boundary conditions given by the boundary of the region. This suffices since the solution to the semi-ideal model is locally correct on subchunk (0 0 0) by construction. In order to show this it is in turn sufficient to show that both solutions agree on subchunk (0 0 0) since we can then add it to the boundary conditions and both models are identical outside this subchunk, hence have the same solution.
For this we show that for any propagation path from the boundary of the constructed region into subchunk (0 0 0) in the real model there exists a corresponding propagation path from the boundary to the same block in the semi-ideal model that has at least the same contribution, and conversely for every propagation path in the semi-ideal model there is a corresponding path in the real model that has at least the same contribution.
Note that the distinction between changed and unchanged borders was necessary as the unchanged borders are now treated as sources in this model, hence proving local correctness in this model does not provide useful information about them (which is no problem as we already know that these are fine).
We consider the following cases according to the decomposition of the boundary as above:
- Both in the real and semi-ideal model there are no propagation paths from the faces that are one subchunks away from (0 0 0), excluding the top faces, so this case is easy
- Both in the real and semi-ideal model the only propagation paths starting from the top faces start with direct skylight access, i.e., with a skylight value of 15. An optimal propagation path in the real model first goes straight down until y=16, then propagates in the xz-plane and then propagates for the remaining y-level using the skylight optimization, hence not reducing the light value. An optimal path in the semi-ideal model only does part of its xz-movement at y=16 and does the rest at the final y-level, since subchunk (0 0 0) is treated as non-optimizable. This gives a 1:1 correspondence between optimal real and optimal semi-ideal paths giving the same light contributions, as desired.
- Finally consider propagation paths starting from a face touching (0 0 0). Both for real and semi-ideal propagation paths we can find an alternative one giving at least the same contribution that first travels along the boundary of the region, then travels on the xz-plane inside a single non-optimizable subchunk (including (0 0 0) in case of the semi-ideal model) and then possibly travels downwards through an optimizable subchunk using skylight optimization. To prove this consider such a path appended by a single-step propagation into a non-optimizable subchunk. Using that the region consists purely of air blocks, the propagations can then be reordered in such a way that the subchunk is crossed along the boundary of the region, so that this part can be absorbed into the first part of the path and the resulting path is again of the desired form.
Note that the boundary of the region is "locally correct" with respect to the real model, i.e., each light level on the boundary is at least the maximum of all real contributions from neighbors lying on the boundary, or in other words, the propagated value of a real path along the boundary is at most the given boundary value. This is true simply because the boundary values are chosen from the global solution of the real model. The same is also true for the semi-ideal model, since it only differs from the real model on subchunk (0 0 0) and for blocks of this subchunk that belong to the boundary of the region we know that they are locally correct in the ideal model and hence also in the semi-ideal model. Hence we can leave out this first part of the propagation paths travelling along the boundary as the new starting point will then still lie on the boundary and has at least the same contribution by the argument just given.
So we can restrict our attention to those optimal paths first travelling on the xz-plane of a single non-optimizable subchunk and then possibly travelling downwards through an optimizable subchunk using skylight optimization.
In the real model an optimal path now starts at y=16 (which is treated as a non-optimizable subchunk in both models) then travels on the xz-plane and then travels down into subchunk (0 0 0) using skylight optimization, so that the value doesn't change. An optimal semi-ideal path starts at the correct y-value and then travel on the xz-plane. The important observation is now that since subchunk (0 0 0) is optimizable, it does not have an associated lightmap and hence its light values are constant along columns (including y=16). Hence we can transform an optimal real path into an optimal semi-ideal path by starting at the correct y-value, which then has the same starting value and gives the same contribution. And also conversely an optimal semi-ideal path can be transformed into a real path by starting at y=16 with the same starting value and same contribution. This produces again a 1:1 correspondence between optimal real and semi-ideal propagation paths giving rise to the same contributions, as desired.
This finishes this rather lengthy proof.
Note that this argument requires a slightly inefficient specification for the skylight optimization, namely that non-air blocks also make the neighbor subchunks above non-optimizable. This is the one implemented by Vanilla. (This was used when transforming paths to first travel along the boundary and then only inside a single chunk. Without this assumptions the subchunks at y=-1 might not be empty and there would be more complicated optimal propagation paths).
In the absence of unloaded chunks this is not needed and it would be sufficient that a non-air block only makes neighbor subchunks that have less or equal y-value non-optimizable. In the presence of unloaded chunks the argument given above requires the slightly inefficient version and in fact one can (easily) construct examples showing that the more efficient specification would produce wrong results and is hence not sufficient.
So if one would like to implement this optimization, extra care needs to be taken in the presence of unloaded chunks and the optimization needs to be disabled in those cases or information about unloaded neighbors needs to be provided through other means.
Best,
PhiPro
![[Mod] ampolive](../../../data/avatars/users/18949.svg){kind=small}
Could be tangentially related to MC-170010 also? Looks a bit similar visually. I am not sure.
This is similar to MC-170010.
Caused by MC-170010, which has been resolved for Future Update.
It turns out that the interaction between initial lighting and skylight optimization introduces some subtleties I didn't really deal with; basically "Hence, all skylight updates will be properly propagated" isn't completely true due to the interaction between skylight optimization and the fact that some subchunks might be partially opaque to skylight.
I will update that discussion once I figured things out completely.
Nevertheless, this doesn't really affect the general theme of the solution: Initialize lightmaps to the exact values that would have been returned before adding it and only schedule additional propagations as needed to fullfill the desired invariant for initial lighting. Don't mix lightmap initialization with initial lighting code and compensate the change in values with a bunch of other checks. That will only complicate the code and has a high probability that some check is missing.
For the moment, I would suggest to simply keep that part of the initialization logic that spreads light to neighbors (in fact, independent of whether the subchunk is empty or not), but properly initialize lightmaps inside createLightArray(...) and remove those parts of the logic that reinitalize lightmaps and spread darkness.
In order to figure out the correct way to deal with the interaction between initial lighting and skylight optimization, it would be quite helpful to know whether the event of a block change inside a chunk between pre_ligth and light stage as described above is possible or not
After looking through the code some more, I feel like it is indeed possible that there are block changes inside chunks between the pre_light and light stage. In particular, ProtoChunk.setBlockState(...) contains the clause
suggesting the possibility of block changes in those stages.
For what it's worth, I could only come up with one simple implementation that trivially works correctly (which I will present below). Everything else I tried causes problems in one way or another. The basic problem is that the skylight optimization is only aplpicable away from any non-air blocks, but that the empty region behaves necessarily as if it were opaque. This makes the skylight optimization inapplicable near the empty region of any not yet lighted chunk, as we don't necessarily have enough lightmaps around. Hence many applications of it during initial lighting are in fact not correct; however, simply turning off this optimization would cause problems as well, as we have to deal with missing lightmaps around the empty region in some way.
I can also cook up examples that break the current vanilla code and produce persistent errors, as long as block changes in those chunks are not forbidden. I also suspect some more issues than I currently found...
So, in summary, I think it's not at all straightforward to come up with some solution (other than the one I present below) that works in all edge cases. The current solution definitely does not, although those situations don't arise in practice with high probability.
If block changes were forbidden, the situation would be much simpler. In that case, we wouldn't really care about what happens in the pseudo-empty region, as it will be completely filled with skylight later on, anyway. So in this case we could simply state in the invariant that the pseudo-empty region does whatever it wants (as long as light is correctly propagated to the outside, perhaps). But as soon as the pseudo-empty region is allowed to change and light can decrease, things are really easy to mess up. Keeping any invariant always seems to require some repair mechanism.
On another note, there is currently also a problem when removing the topmost lightmap of the pseudo-empty region: When removing such a lightmap, light values will be overridden with 0. The reason for this change in light values is that we model the empty region as opaque, but do not properly react to this change in opacity. The result from this is that we now produce ghost light, in case there was any outgoing light from this lightmap before its removal. If later on some skylight is decreased, the lighting engine won't be able to properly track this through the missing light map and will produce persistent errors. So, currently we also should counteract this situation by spreading darkness when removing such a lightmap. However, this is not needed in the solution below.
Also, there is currently some mechanism in the code that prevents lightmaps from being completely erased before the chunk finishes its initial lighting. Readding a previously removed lightmap will simply restore its old values. I'm not really sure what the idea behind this is. My guess is that it should prevent lightmaps from being deleted between loading the chunk from disk and registering its non-empty subchunks. The problem with this is that this mechanism is only turned off once the initial lighting is done. So, if we readd some lightmap during initial lighting, some stale values will be restored instead of creating and initializing a new one. It is again easy to cook up examples, where this stale lightvalues will cause persistent lighting errors. So, I would suggest here to disable this mechanism as soon as the non-empty subchunks have been registered and not only after initial lighting.
So, let's now discuss my proposed solution for initial skylight. It is conceptually quite easy and avoids all this discussion about inapplicable skylight optimization and actions required upon adding and removing lightmaps. Currently, the main problem is that the empty region behaves as if it was filled with stone, causing all kinds of problems around it. To overcome this issue, we can simply push up this misbehaving region high up into the sky.
we can then simply remove the forceloaded lightmap.
Basically, I want to model initial skylight simply as if there was a stone platform high up above the world and then use the normal lighting code. This will solve all these nasty issues.
Concretely, this means that we should forceload a lightmap in the void above the world in the pre_light stage together with all the regular lightmaps (as far as I can tell, lightmaps can currently be created 1 subchunk below and above the world bounds already) and make it receive all light contributions from all sides, but no direct skylight from above. This perfectly simulates the situation that there was a stone platform just above this lightmap. Preventing direct skylight contribution is already implemented, so there isn't really much to do here. The chunk now behaves as any other ordinary chunk, simply with a stone platform directly above it. Hence, as for any other chunk, there are no special actions required upon adding or removing lightmaps and skylight optimizations are now perfectly fine.
Initial lighting now takes the form of removing this imaginary stone platform. Concretely, this means that we should set all lightmaps in the pseudo-empty region to a value of 15 and spread light to everything adjacent to the pseudo-empty region, as is currently almost done (currently we only spread light to neighbors below the topmost lightmap, but we should in fact spread light for every y-level in the (pseudo)-empty region). Afterwards
While this approach might be slightly less optimal due to the additional lightmap, I don't think that the difference will be really noticeable. Given the fact that it avoids all kinds of light checks upon adding and removing lightmaps, it might even pay off performance-wise.
Basically, this is the same solution as already presented in the original report, with the only difference being this forceloaded lightmap high up in the sky that solves all the nasty issues. So this keeps all the benefits listed above, primarily it allows to simply solve the bug originally addressed in this report by simply initializing lightmaps with the previous values at the respective positions and not doing any other action upon lightmap addition or removal. It also moves all of the initial lighting code to one place, instead of being scattered around, making the coder easier to understand and maintain.
I would suggest to implement this solution, mainly for its simplicity and correctness.
It would be cool to hear your opinions on this. In particular, I would be interested in what your idea was for the current initial lighting code. Maybe we can come up with some fancy more optimal and actually correct solution together
Best,
PhiPro
While implementing this fix as a fabric mod, i noticed that
MC-170012should ideally be fixed first, as it makes the handling of the forced lightmaps, as mentioned in the previous comment, a bit easier. The problem is that upon initial lighting we need those forced lightmaps to be present for the neighbors, which is currently not so nice because ofMC-170012. Alternatively, instead of fixing that issue first, you can also explicitly force these lightmaps on the neighbors, but I think this will be the more cumbersome way to do things.As a reference, I have implemented my suggested solution as a fabric mod. The code can be found at https://github.com/PhiPro95/mc-fixes/tree/mc-170010. While doing so, I noticed a few things that are worth documenting here. Also note that the code really isn't that long or complicated, although the use of mixins vs. direct source control sometimes makes things look more complicated than they actually are.
Observe that this also solves
MC-162253as explained there.First, as already noted in the last comment, it is convenient to fix
MC-170012first. This is done in https://github.com/PhiPro95/mc-fixes/tree/mc-170012. In my implementation I introduced a new pre_light generation stage and mirrored the state of the current light stage, i.e., it requires chunks in the features stage within a margin of 1. This is due to the fact that ProtoChunk does not properly handle lightmaps, as noted in the code. So I didn't want to introduce problems by promoting chunks too early and hence mirrored the current light stage. Whether or not this margin can be removed or if the missing lightmap handling in ProtoChunk is in fact a bug requires some knowledge about the worldgen code that I don't have. So that decision is up to you.Edit: Looking a bit more into the worldgen code it indeed looks like the missing lightmap handling is a bug. Once the missing lightmap handling is added and the blockligth version of
MC-170012, as noted in the comment there, is fixed, this margin is no longer needed. This is done in the most recent version of my implementation.Second, it turns out that the current data retainment mechanism, as mentioned in the report and comment above, has some more far reaching problems than I thought. It not only overrides lightmaps in certain rare situations, as mentioned above, but it can in fact lead to loss of data in surprisingly common situations. I will link the bug report to that once it is created.
As a consequence, I would ignore this lightmap retainment code for the moment and simply leave it as it is. Simply write the code as if the issue with the lightmap retainment code was solved. This doesn't cause any regression compared to the current vanilla code, any potential problems already exist within the current code.
The retainment code can then be fixed later. In fact, it may be even convenient to solve this issue here first, so that newly created lightmaps are always trivial in the appropriate sense. This would at least slightly help for the solution I have in mind, but more on that in the report for the data retainment issue.
Third, it turns out that the current code hides some problems of
MC-169913in some rare cases. For example, consider the following situation:This will already trigger part 1 of
MC-169913, but this is hidden by the current lightmap initialization code. With my proposed fix, this is no longer the case and these steps will produce some dark spots on the ground. However, the current code only helps in some edge cases and it's easy to find situations where it doesn't help. For example, modify the steps above by placing a block above the platform, e.g., /setblock 7 255 7 minecraft:glass. This will suffice to trigger the issue with the current vanilla code as well. So, I would say that this little regression is a fair compromise, given that it solves the rather severe performance issue ofMC-162253.Nevertheless, I have also implemented a fix for this part of
MC-169913, which can be found at https://github.com/PhiPro95/mc-fixes/tree/mc-169913. While fixing the whole issue described there requires a bit of work, as the multithreading code needs to be rewritten, the first part of the issue, which is what we observe here and which is also the most common and severe part of the issue, can be fixed rather easily. As explained there, we must simply reorder operations on the main thread as follows:and the lighting engine must move the lightmap removal to the POST_UPDATE phase, whereas lightmap creation needs to stay in PRE_UPDATE. However, there is a small problem with this, namely that earlier removals of lightmaps can override later creations, as they are carried out in a later stage.
This is solved by my proposed solution for the complete problem by cancelling operations properly, but this requires the rewrite of the multithreading code. Fortunately, we can easily work around this issue by scheduling an additional lightmap creation into the POST_UPDATE phase. This will eliminate any overrides and make sure that the final configuration of lightmaps is now indeed correct. However, as noted in the bugreport, this will not eliminate the problem of lightmaps being removed and recreated, causing data loss. But this situation shouldn't be that common in practice.
In summary, this easy solution for part 1 of
MC-169913greatly improves the situation over the current vanilla code and eliminates any regression introduced by eliminating code that currently shadows these issues.I hope these inputs assist you in implementing a fix for the issue.
Best,
PhiPro
I found another small bug impacting initial skylighting. I post it here since the report already kindof derailed into a discussion about initial lighting. Let me know if I should open a new bug report.
ChunkSkyLightProvider.recalculateLevel(long id, long excludedId, int maxLevel) contains some logic to calculate contributions from neighbors that don't have an associated lightmap, namely it looks up the light value by inherting it from the next lightmap above, as usual.
The problem now occurs if there is no such lightmap, i.e., the neighbor block is directly exposed to skylight, in which case it uses p = ((SkyLightStorage)this.lightStorage).isLightEnabled( n) ? 0 : 15;.
However, 0 is the skylight value of the neighbor (note that light values are inverted inside the lighting engine) but p should contain the propagated value, as is the case when chunkNibbleArray4 != null. Hence this should not be 0 but 1, as this case only deals with horizontal propagations.
Or even better it should be this.getPropagatedLevel(m, id, ((SkyLightStorage)this.lightStorage).isLightEnabled( n) ? 0 : 15).
This issue basically only affects intial skylight, as it can only occur if the block in question is directly exposed to skylight, which means that it will be completely bright anyways. However, before initial lighting this conclusion does not hold true.
This bug then makes the lighting model depend on the empty region which cuases problems when the empty region changes, similar to issues discussed above.
This bug still present in 1.16.1 or not?
In 1.16.2
Can confirm in 1.17.1.