Fix direct palettes

[frej4189]

Closes https://github.com/PrismarineJS/prismarine-chunk/issues/164

There are a handful of problems with the implementation of direct palettes that would cause errors on some servers (most notably Hypixel).

This PR addresses these issues:

1. The BitArray readBuffer function always attempts to consume data enough to populate the entire storage. This will cause an error when the server provides a shorter (or longer) long array than needed.
2. The direct palette uses the server provided bits per value, instead of truncating to the size of the registry.

I generally believe these changes to be correct, but I have not tested any versions below 1.18. I'm putting up the PR now so others can test with mineflayer and so we can get reviews started.

[frej4189]

Breaks with lighting information and other similar things that uses BitArray for a wrapping its data.

Also CI won't pass most tests since it relies on dumps generated with the incorrect implementation.

[extremeheat]

Also CI won't pass most tests since it relies on dumps generated with the incorrect implementation.

What do you mean by "incorrect implementation"? The test data should just be desterilized chunk data off the network, no?

[frej4189]

Also CI won't pass most tests since it relies on dumps generated with the incorrect implementation.

What do you mean by "incorrect implementation"? The test data should just be desterilized chunk data off the network, no?

Dumps seem to be generated by pchunk itself

[rom1504]

They are not. Dumps are raw bytes generated using Minecraft-protocol with https://github.com/PrismarineJS/minecraft-chunk-dumper

The tests using them check internal consistency of pchunk between load and dump

So if they're not passing , something is broken

[frej4189]

I see.

I haven't fully implemented dumping so that could be the cause of this. I just saw invalid data in the dumps (namely direct palette with bit per block set to 16) but that may well be an issue with the dumping method if it compares the new dumps with the ones from the network.

[rom1504]

If data in the dumps is invalid it means there is a bug in the vanilla Minecraft server implementation

[frej4189]

Yeah it's all good. I'm just saying there's something wrong with my dump method. Not that there's something wrong with the dumps

[extremeheat]

Cc @nickelpro if you have the time to review

[nickelpro]

Other than the one comment, which might be nonsense because I'm not 100% clear on why that read was there to begin with, LGTM WRT the commit doing what it says it does.

I think there's a bit of repetition here that could use refactoring, but the basic idea of only using GLOBAL_BITS_PER_X when we know that it's big enough is correct and the current behavior should be considered a bug.

I'm less certain about trusting servers about their long counts, but testing should reveal the safety of that.

[frej4189]

I don't think there's anymore repetition than earlier so I don't think that's too much of an issue.

With regards to trusting servers long counts, that is also what the Vanilla client does. See https://github.com/extremeheat/extracted_minecraft_data/blob/20e005fafc4928179f144d3ff68413f0ca70808b/client/net/minecraft/network/FriendlyByteBuf.java#L402-L417

[nickelpro]

I don't think there's anymore repetition than earlier so I don't think that's too much of an issue.

prisChunk being bad is the norm, we do try to improve it from time-to-time. Agree not a blocker.

With regards to trusting servers long counts, that is also what the Vanilla client does.

I'm down for this in principle, I'm just skeptical of modded servers in general to follow vanilla behavior and this entire patch series is about modded server support. Vanilla works fine.

[frej4189]

I'm down for this in principle, I'm just skeptical of modded servers in general to follow vanilla behavior and this entire patch series is about modded server support. Vanilla works fine.

While I agree this technically is only a problem on modded servers, the current chunk implementation does in fact not mirror that of the Vanilla client. I am not jumping through hoops to support non-Vanilla servers, I am simply mirroring the Vanilla (client) implementation.

Also to add the current implementation actually does not work on Vanilla servers due to the GLOBAL_BITS_PER_BLOCK constant being incorrect for some versions. It just doesn't cause errors because we seldom fall back on that value.

[rom1504]

looks ok but yeah we really need to put more code in common between versions here or it's going to become very hard to maintain fast

[rom1504]

maybe let's wait a few more days if anyone else wants to take a look (maybe @Karang ?)

[frej4189]

Will add chunk dumps from Hypixel to test data aswell. I have a few that I tested locally but I haven't extracted it just the way the other dumps are generated so I will have to make some changes.

[frej4189]

Added hypixel dumps and made a few changes to the dump tests to account for non-Vanilla format (i.e. histogram will not look like a naturally generated world). I don't think anymore testing is needed, but if anyone has comments to the code itself, feel free.

[rom1504]

@frej4189 if you run the tests with implementation from main on hypixel chunks, does it fail? (eg does this PR fixes it)

[frej4189]

@frej4189 if you run the tests with implementation from main on hypixel chunks, does it fail? (eg does this PR fixes it)

It does indeed.

[frej4189]

Running current implementation CI here https://github.com/frej4189/prismarine-chunk/pull/1

[rom1504]

ok sounds good, thanks for the pr!

[rom1504]

/makerelease