Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DataPalette index & deprecate unused globalPaletteBits #775

Merged
merged 3 commits into from
Dec 20, 2023

Conversation

davidmayr
Copy link
Contributor

This PR attempts to fix #774

From my basic testing, the block palettes still work as they did before without any issues and biome palettes do not throw any errors anymore. They also seem correct in game, but don't quote me on that. (Since my test world is very small with not that many different biomes (2 to be exact))

@AlexProgrammerDE
Copy link
Contributor

I've found this in vanilla code:
image

@davidmayr
Copy link
Contributor Author

I've found this in vanilla code:
image

That would result in a completly different index then before, right? I don't think that would work correctly unless other parts are rewritten aswell? Or am I wrong here?

@davidmayr
Copy link
Contributor Author

davidmayr commented Dec 19, 2023

But I can try if it works in a few hours

@Kas-tle
Copy link
Member

Kas-tle commented Dec 19, 2023

These are equivalent as sizeBits is effectively maxBitsPerEntry minus minBitsPerEntry, so the position of the encoded bytes is the same.

For example, a chunk section is CHUNK(4, 8, 4096), meaning a sizeBits of 4. Let's consider the position [5, 7, 9] in the chunk.

With the code in this PR, we would do:

( 7 << 8 ) | ( 9 << 4 ) | ( 5 )
11100000000b | 10010000b | 101b

11100000000
   10010000
        101
-----------
11110010101 = 1941

With the code from the Minecraft jar we would do:

( ( ( 7 << 4 ) | 9 ) << 4 ) | ( 5 )
( ( 1110000b | 1001b ) << 4 ) | ( 101b )

1110000
   1001
-------
1111001

( 1111001b << 4 ) | ( 101b )
11110010000b | 101b

11110010000
        101
-----------
11110010101 = 1941

@Kas-tle
Copy link
Member

Kas-tle commented Dec 19, 2023

Also since you're working in this area feel free to remove globalPaletteBits per #676

@davidmayr
Copy link
Contributor Author

Also since you're working in this area feel free to remove globalPaletteBits per #676

Do you want me to make breaking API changes, or should I keep the existing static createEmpty, createForChunk, createForBiome methods and deprecate them?

@Kas-tle
Copy link
Member

Kas-tle commented Dec 20, 2023

Yes it should have a deprecation notice and for now just redirect to the new method

@davidmayr
Copy link
Contributor Author

Yes it should have a deprecation notice and for now just redirect to the new method

And as for the @AllArgsConstructor ? Should I manually recreate a deprecated constructor with globalPaletteBits to not cause any breaking changes as well?

@Kas-tle
Copy link
Member

Kas-tle commented Dec 20, 2023

Correct. Basically the goal is just that we can easily remove it in the next breaking version.

@Kas-tle Kas-tle changed the title Fix datapalettes not working (biomes) Fix DataPalette index & deprecate unused globalPaletteBits Dec 20, 2023
@davidmayr
Copy link
Contributor Author

Done. Shouldn't involve any breaking changes now

@Kas-tle Kas-tle merged commit 809a0c5 into GeyserMC:master Dec 20, 2023
1 check passed
@Kas-tle
Copy link
Member

Kas-tle commented Dec 20, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataPalettes do not work correctly.
3 participants