Fix direct palettes #232

frej4189 · 2023-08-24T12:45:30Z

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:

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

…smarine-chunk into fix-direct-palettes

frej4189 · 2023-08-24T13:50:50Z

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 · 2023-08-24T19:28:25Z

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 · 2023-08-24T19:53:54Z

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 · 2023-08-24T22:03:34Z

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 · 2023-08-25T02:55:56Z

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 · 2023-08-25T09:53:27Z

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

frej4189 · 2023-08-25T12:46:21Z

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

test/test.js

src/pc/1.18/ChunkColumn.js

extremeheat · 2023-08-25T18:08:08Z

Cc @nickelpro if you have the time to review

src/pc/common/PaletteBiome.js

nickelpro · 2023-08-25T20:21:28Z

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.

src/pc/common/BitArray.js

frej4189 · 2023-08-25T20:24:17Z

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 · 2023-08-25T20:31:18Z

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 · 2023-08-25T20:35:42Z

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.

nickelpro

Comments all resolved, I'm happy with this.

Holding off on merge for further field testing/rom's thumbs up

rom1504 · 2023-08-25T21:07:18Z

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 · 2023-08-25T21:08:26Z

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

frej4189 · 2023-08-25T21:17:53Z

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 · 2023-08-26T10:26:33Z

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 · 2023-08-26T10:48:18Z

test/test.js

@@ -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')) {


which are hypixel chunks not the same after dumping?

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

rom1504 · 2023-08-26T11:26:44Z

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

frej4189 · 2023-08-26T11:31:30Z

@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 · 2023-08-26T11:40:12Z

Running current implementation CI here frej4189#1

rom1504 · 2023-08-26T12:28:22Z

ok sounds good, thanks for the pr!

rom1504 · 2023-08-26T12:28:45Z

/makerelease

Frej Alexander Nielsen and others added 5 commits August 24, 2023 14:25

Fix direct palette parsing

f6129bb

Merge branch 'PrismarineJS:master' into fix-direct-palettes

3ea42c2

Fix mcData parsing

98bf26d

Merge branch 'fix-direct-palettes' of https://github.com/frej4189/pri…

d22ce83

…smarine-chunk into fix-direct-palettes

Fix chunk dumping for 1.18+

f33f6ca

Generalize readBuffer and writeBuffer methods in bitArray

0fc215e

Frej Alexander Nielsen added 2 commits August 25, 2023 15:59

Fix dumping/loading issues

e3315d5

Fix lint

bcdc427

lkwilson reviewed Aug 25, 2023

View reviewed changes

test/test.js Outdated Show resolved Hide resolved

Remove version filter from test

1b5e1f4

lkwilson reviewed Aug 25, 2023

View reviewed changes

src/pc/1.18/ChunkColumn.js Show resolved Hide resolved

Apply fixes to earlier versions

9dd89a9

nickelpro reviewed Aug 25, 2023

View reviewed changes

src/pc/common/PaletteBiome.js Show resolved Hide resolved

Update readOffset to skip entire long array

12c04dd

nickelpro reviewed Aug 25, 2023

View reviewed changes

src/pc/common/BitArray.js Show resolved Hide resolved

nickelpro approved these changes Aug 25, 2023

View reviewed changes

Frej Alexander Nielsen added 2 commits August 26, 2023 12:23

Add hypixel chunk dumps

3f7e4a9

Fix lint

3cce92c

rom1504 reviewed Aug 26, 2023

View reviewed changes

rom1504 merged commit 6422abc into PrismarineJS:master Aug 26, 2023
2 checks passed

rom1504bot mentioned this pull request Aug 26, 2023

Release 1.35.0 #234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix direct palettes #232

Fix direct palettes #232

frej4189 commented Aug 24, 2023

frej4189 commented Aug 24, 2023

extremeheat commented Aug 24, 2023

frej4189 commented Aug 24, 2023

rom1504 commented Aug 24, 2023 •

edited

Loading

frej4189 commented Aug 25, 2023

rom1504 commented Aug 25, 2023

frej4189 commented Aug 25, 2023

extremeheat commented Aug 25, 2023

nickelpro commented Aug 25, 2023

frej4189 commented Aug 25, 2023

nickelpro commented Aug 25, 2023

frej4189 commented Aug 25, 2023

nickelpro left a comment •

edited

Loading

rom1504 commented Aug 25, 2023

rom1504 commented Aug 25, 2023

frej4189 commented Aug 25, 2023

frej4189 commented Aug 26, 2023

rom1504 Aug 26, 2023

frej4189 Aug 26, 2023

rom1504 commented Aug 26, 2023

frej4189 commented Aug 26, 2023

frej4189 commented Aug 26, 2023

rom1504 commented Aug 26, 2023

rom1504 commented Aug 26, 2023

Fix direct palettes #232

Fix direct palettes #232

Conversation

frej4189 commented Aug 24, 2023

frej4189 commented Aug 24, 2023

extremeheat commented Aug 24, 2023

frej4189 commented Aug 24, 2023

rom1504 commented Aug 24, 2023 • edited Loading

frej4189 commented Aug 25, 2023

rom1504 commented Aug 25, 2023

frej4189 commented Aug 25, 2023

extremeheat commented Aug 25, 2023

nickelpro commented Aug 25, 2023

frej4189 commented Aug 25, 2023

nickelpro commented Aug 25, 2023

frej4189 commented Aug 25, 2023

nickelpro left a comment • edited Loading

Choose a reason for hiding this comment

rom1504 commented Aug 25, 2023

rom1504 commented Aug 25, 2023

frej4189 commented Aug 25, 2023

frej4189 commented Aug 26, 2023

rom1504 Aug 26, 2023

Choose a reason for hiding this comment

frej4189 Aug 26, 2023

Choose a reason for hiding this comment

rom1504 commented Aug 26, 2023

frej4189 commented Aug 26, 2023

frej4189 commented Aug 26, 2023

rom1504 commented Aug 26, 2023

rom1504 commented Aug 26, 2023

rom1504 commented Aug 24, 2023 •

edited

Loading

nickelpro left a comment •

edited

Loading