mojira.dev
MC-80966

Lightcalculation of ChunkSelection faulty implemented resulting in client bugs (empty chunks don’t show light/render dark)

Resolved
Fixed
46

The code in ChunkSelection.java that reports if a specific part of a chunk needs to be delivered to a client or not is faulty:

I'm not sure what the Class is named in the obfuscation. In the partially deobfuscated code it's ChunkSelection.java and the method's name is public boolean a().

I'm guessing it's public boolean hasBlockData() in the original code

ChunkSelection.java

public boolean a(){ 
    return this.nonEmptyBlockCount == 0; 
}

This method is called when a Chunk is placed in the OutgoingQueue to a client. It checks if a ChunkSelection should be delivered or not.
The code does not consider the fact that - even if there is no block in a chunkselection - there may still be light-information for that selection.

This results in light-Bugs on the client side if a Chunk hast light-information in a chunkselection that does not contain any blocks.

Video content:

  • Connecting to a Singleplayer World (same in Multiplayer)

  • Two chunks have obvious light-Information missing

  • Teleporting does not fix the problem

  • Placing a Block inside that specific chunkselection - thus working around the bug - does not fix the bug initially as the information only get updated on ChunkSending.

  • Teleporting post the workaround does fix the bug

  • Removing the Block - recreating the empty chunkselection and recreating the bug does work post teleport

  • F3 shows we are exactly at the edge of a chunkSelection (63/64)

  • The client-side light-updates in the area around a player does only fix the problem temporarily

This can be fixed by changing the code:

ChunkSelection.java

public boolean a(){ 
   // return this.nonEmptyBlockCount == 0; 
     return false; //kade fix, bukkit/mc reports wrongly if a part of a chunk is empty or not.
}

Performance and Gameplay implications of this bugfix

  • Performance
    The performance impact is negligible. The problem occurs in <5% of chunks. Specifically: It occurs in every Chunk that has Blocks at the top of one ChunkSelection and light-information in the one above that but no blocks.

  • Bandwidth
    The Bandwidth increases a small amount for the <5% Chunks. But as all that is being sent is the light-map we are talking about a compressed data of only some bytes more.

  • Gameplay
    There are absolutely no side-effects of this fix. I have been running this code-change on my servers for at least 1 1/2 years now (concurrent playercount > 300).

Conclusion
In essence the Idea behind the "nonEmptyBlockCount == 0"-Check is good. But its neglecting the fact that there might be light-information present. As ChunkSelections without any lightinformation are null anyways there is no need for that check.

Linked issues

is duplicated by 45

MC-531 One-Block thick ceilings not casting shadows when chunk is unloaded and reloaded Resolved MC-911 Sunlight in big caves and under overhangs Resolved MC-1254 Phantom skylight - sunlight appears in impossible areas Resolved MC-2318 sl value is glitched in the End Resolved MC-2907 light levels in an enclosed space fluctuating between 0 and 15 with no light sources. bl=0 sl=15 rl=15. fully enclosed(3 deep) with dirt. Resolved
40 more links

relates to 2

Attachments

__1.png __2.png 2016-02-17_11.02.11.png 2016-02-19_09.38.18.png 2016-06-08_17.35.47.png 2016-06-08_17.40.48.png 2018-06-02_21.34.23.png 2018-06-13_19.18.30.png SG_LightBug1.png SG_LightBug2.png SG_LightBug3.png SG_LightBug4.png

Comments 23

Still present in 16w07b.

Affects 1.9-pre2 also! Just tried. Same thing, place a torch in a chunk and it doesn't go across the boundary

Tyler Bozinovski

Confirmed for 1.9pre4.

Confirmed 1.10; same seed same coordinates

Using MCP Names: ChunkSelection is ExtendedBlockStorage, and a is isEmpty. In 1.10 ExtendedBlockStorage is obfuscated as asw (you can find this in any version by searching for a String found in Chunk, and then finding the storage array member - the string I used was "Could not set level chunk heightmap, array length is ").

MCP names are way better for dealing with the base code than NMS, since it's got a lot more stuff renamed (and renamed sanely, and in separate packages... It's actually designed to be used by developers, unlike NMS where it's explicitly stated that things may be poorly named and direct use should be avoided. Also, even the non-renamed stuff is named uniquely, so there's only ever one non-renamed method named something (rather than there being two a methods, you'll see (for example) func_184161_a and func_184159_a)). You can also get the obfuscated names from the deobfuscated ones by looking at methods.csv or fields.csv and then using the SRG name found there in the joined.srg file (for class names you only need joined.srg). You can get MCP here.

That said: there is a performance cost to your fix, though it isn't as severe when network compression is factored in. You're still sending a chunk section of air, which is (due to the way the the chunk section is formatted) always going to use at least 4 bits per block (and would have used 16 in 1.8). That's 2048 bytes per section, uncompressed, though all the data is the same and it'll probably be pretty small when compressed.

Also, there are two types of empty chunk sections that can be found. The first is when isEmpty returns true, and the second is when the section itself is null (see Chunk.NULL_BLOCK_STORAGE). Your solution only covers the case where isEmpty is true, but NULL_BLOCK_STORAGE may also apply (though it looks like there may not be any cases where a section should be lit but is null, since they're created when lighting hits them; it's still a possible concern).

I would recommend instead that a protocol change is made to allow using 0 bits per block to indicate a section that has no blocks, but still has light. Those sections (and their light data) would still be sent, but no block data would need to be sent. (And the bits per block field already exists and currently won't ever be 0, so it wouldn't require adding any new data to the packet). Depending on whether NULL_BLOCK_STORAGE turns out to be an issue, it may make sense to remove the primary bit mask from the chunk data packet, but it may not be necessary if it can be reasonably confirmed that there is no light data in those sections. The 0 bits per block thing would be implementable by another IBlockStatePalette, which only ever has a length of 0 and always returns air. It would require client and server code changes, so a serverside mod couldn't do that without client modification, but overall it would be an improvement.

The isEmpty solution does work for servers in the meantime, but it's not ideal.

13 more comments

Here's what's happening in 1.13.2. The torch one is happening at a chunk boundary.

[media][media][media][media]

The issue was fixed in 18w43a (the first snapshot for 1.14), but is still present in 1.13.2 (it hadn't been fixed then, but since 18w43a was released 2 days after 1.13.2 it was never added).  I've added 1.13.2 to the affected versions for clarity.

Thanks for the info. I'm rather clumsy with snapshot names and didn't realize it was fixed for 1.14.

I really don't understand where I'm supposed to change that code or if im supposed to, having this issue in 1.14 prerelease 2. 

There's a different issue that produces similar effects, MC-142134. Check that issue for more information. (The bug here is a very specific issue regarding empty chunks, and has been fixed, but there's a different, more sporadic issue that still occurs and that is MC-142134).

Kademlia

Fry

Confirmed

chunksection

Minecraft 1.8.1, Minecraft 1.8.3, Minecraft 1.8.4, Minecraft 1.8.5, Minecraft 1.8.6, ..., Minecraft 18w06a, Minecraft 18w22c, Minecraft 1.13-pre1, Minecraft 1.13.1, Minecraft 1.13.2

Minecraft 18w43a

Retrieved