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

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

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.

I do not understand why you wrote this.

• MCP Naming is not a concern here

• The network cost is already explained in the conclusion. Your elaboration adds nothing new.

• Your remark regarding a null-Section of a chunk is wrong. The code is built around identification of null-Sections.

• Your recommendation adds work on the server, the client and can have complicated new validation cases. For a problem of this magnitude this is absolutely over engineering. I have already stated that these changes are validated and working for over two years now. There is no need to complicate the issue buy adding unnecessary additional work.

• The whole idea behind the isEmpty-Check is faulty. It only hides information. Removing it is the ideal solution.

MCP Naming is not a concern here

True, that section started out as a response to "public boolean a()" and got way too long as I expanded it several times; it's really not that important.

Your remark regarding a null-Section of a chunk is wrong. The code is built around identification of null-Sections.

Indeed it is, sorry about that (I didn't have a chance to fully read it). I see that you mention that in the conclusion section, and I should have noticed that.

The network cost is already explained in the conclusion. Your elaboration adds nothing new.

It isn't fully explained. You say "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." I feel like that doesn't correctly explain what additional data is being sent (it's a 16x16x16 cube of air blocks). While you may know that, I still feel like it's important to mention.

Your recommendation adds work on the server, the client and can have complicated new validation cases. For a problem of this magnitude this is absolutely over engineering. I have already stated that these changes are validated and working for over two years now. There is no need to complicate the issue buy adding unnecessary additional work.

No, it doesn't overcomplicate it much. The server and client already decide how many bits per block to use (look at BlockStateContainer.func_186012_b). It would introduce some special considerations because right now BitArray doesn't handle a length of 0, but overall it would be better.

And while the data is compressed over the network, it still needs to be uncompressed clientside (and stored). 0 bits per block would eliminate the need to store the all-air sections in memory, both on the client and the server. (And there already is logic to resize the palette if the blocks in it change).

The whole idea behind the isEmpty-Check is faulty. It only hides information. Removing it is the ideal solution.

Yes, it is; that check should be removed. But that doesn't mean we can't keep the performance improvements it gives.

I understand what you are trying to say but this is still not very helpful to the ticket:

Your viewing the network/memory cost from the wrong side. By default all Sections are transferred (100% usage). Sometimes there are sections not transferred because of this faulty code (they should be anyways and everyone expects them to!). If you take typical values, lets say a viewdistance of 6 and and 5 sections each chunk. Thats 169 Chunks or 845 Sections. A missing section because of this bug is about 0,1%. This concern is not justified and even a recommendation to implement something to save 0,1% is a pretty hard sell.

It's a more significant issue when you consider artificial structures (for instance, large caves in custom maps), which is also where this issue is more annoying. [MC-911] (which was recently made a duplicate of this issue) shows more examples of that. I agree entirely that these sections should be sent.

Also, "By default all Sections are transferred (100% usage)." is incorrect, in that null sections aren't transferred. But "5 sections each chunk" does show that you already know that, so this entire paragraph is just me being pedantic.

I decided to do some more calculations and get concrete data.

Preparations

I modified SPacketChunkData to do some calculations on the chunks that are sent. You can see the modified version here (and the modifications themselves are visible on the revisions tab).

I'm comparing the 3 different possible behaviors:

• The current behavior, where empty chunk sections are completely skipped

• Your proposed behavior, where empty chunk sections are sent including their blocks

• My proposed behavior, where only the light data from empty chunk sections are sent

This is done both with the compressed and uncompressed packet data. I'm also printing the chunk sections and whether they are empty, null, or otherwise.

I'm making the assumption that empty sections follow the normal format except that bits per block, palette length, and data array length are all 0 (and the palette and data array are not sent).

Lit superflat preset

First and foremost, there's what I would consider the worst-case scenario: the superflat preset 3;minecraft:bedrock,2*minecraft:dirt,minecraft:glowstone;1;village. That preset has a layer of glowstone on the top layer (somewhat reasonable, though also unlikely), whose light travels into the second chunk section. That means that in every chunk, there is one filled section, one empty section, and 14 null sections, IE 40% of the data sent would be for an empty chunk. Here the improvement is noticeable.

Normal world, but lit

This is another situation where some empty sections might be found: a lit-up world. And yes, the torches do generate some empty sections... but not much.

Amplified world

Amplified worlds sometimes have overhangs that might be empty chunks. Let's see...

Caver's delight customized preset

This preset generates some areas that might be empty chunks, or at least I thought. In reality, it doesn't do much.

Island world superflat preset

What about this one?

Caves of chaos superflat preset

This is probably the only other preset where the difference might be noticeable.

Summary

Basically, you were right. This is a needless optimization, and even skipping empty sections in comparison to sending them doesn't help much. My suggested optimization really doesn't matter too much, since it effectively only reduces the amount of data sent in a section by a third, and there really isn't much data sent when compression is factored in (WOW! We could save a whole byte in the packet that takes 4500 bytes!)

Hi, I'm seeing a similar issue in 1.10.2 and 16w39a in one of my worlds, and trying to figure out if it's related or if I should file a new issue. Here are some screenshots: http://imgur.com/a/OOUVA. Could someone take a quick look and let me know?

@@unknown Press F3+G to view chunk borders. If the lighting glitch is right on the edge, then it is this bug; otherwise it might be something else. (Until this bug is fixed, you should be able to work around this bug by placing a piece of string or something else that's relatively hard to see but still isn't air within the 16x16x16 chunk section that has the lighting glitch).

@Pokechu22 Yep, that's why I included chunk borders in most of the pics. Anyway, thanks for confirming, won't file a dupe 🙂

Something has changed. From Minecraft 1.10 on, the problem shown in the pictures __1.png and __2.png from 27/Mar/2016 (on my server) is gone. The lighting there is now as it should be. This includes the current version Minecraft 1.11.2.

Confirmed for 1.13-pre1 see latest image

The fix is no longer valid for 1.13 - causes very subtle dark patches in the sky that can only be seen when you fly through them (or client mod)

https://hub.spigotmc.org/jira/browse/SPIGOT-4198

This bug is still present in 1.13.1

I'm getting something that looks like this bug in a 1.13.2 Java multiplayer server.

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

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).