Fix direct palettes #232
Fix direct palettes #232
Conversation
|
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. |
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 |
|
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 |
|
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. |
|
If data in the dumps is invalid it means there is a bug in the vanilla Minecraft server implementation |
|
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 |
|
Cc @nickelpro if you have the time to review |
|
Other than the one comment, which might be nonsense because I'm not 100% clear on why that I think there's a bit of repetition here that could use refactoring, but the basic idea of only using I'm less certain about trusting servers about their long counts, but testing should reveal the safety of that. |
|
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 |
prisChunk being bad is the norm, we do try to improve it from time-to-time. Agree not a blocker.
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. |
|
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 |
|
maybe let's wait a few more days if anyone else wants to take a look (maybe @Karang ?) |
|
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. |
|
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. |
| @@ -376,7 +392,7 @@ versions.forEach((version) => describe(`Chunk implementation for minecraft ${ver | |||
| } | |||
| } | |||
|
|
|||
| if (!version.startsWith('1.8')) { | |||
| if (!version.startsWith('1.8') && !chunkDump.includes('hypixel')) { | |||
There was a problem hiding this comment.
which are hypixel chunks not the same after dumping?
There was a problem hiding this comment.
Because hypixel provides excess data which is dropped by pchunk (per this PR). See my comment here #232 (comment)
Since the test already iterates over all blocks and palettes in the chunk, it means the two chunks are still functionally the same if the test passes
|
@frej4189 if you run the tests with implementation from |
It does indeed. |
|
Running current implementation CI here frej4189#1 |
|
ok sounds good, thanks for the pr! |
|
/makerelease |
Closes #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:
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.