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

Add back unnecessary int to CarpetPayload for backwards compatibility #1837

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

altrisi
Copy link
Collaborator

@altrisi altrisi commented Oct 31, 2023

This adds back the unnecessary command int to CarpetPayload, to allow it to handle packets from servers running on older versions that are allowing newer clients with something like ViaVersion.

That int was removed in commit cd57269.

This also adds checks that command is always DATA, moves CarpetPayload to its own class and adds a constructor without command to it.

Completely untested at the moment.

@Fallen-Breath
Copy link
Contributor

Fallen-Breath commented Nov 20, 2023

Reviewed the changes and looks good to me

Also made a quick test for the client <-> server commucation in dev environment, all of version log, rule sync and scarpet rendering work fine

cross-mc-version test is not done tho, but I think it's ready to go

@ifacify
Copy link

ifacify commented Nov 29, 2023

Thank you for your work. Unfortunately upon further investigation there should be no easy way of retaining backwards compatibility given the nature of nameless root nbt introduced in minecraft 1.20.2 update. This completely breaks root nbt compound cross version compatibility and there is no decent way around that. So we may as well take this opportunity to modernize carpet custom payload and keep it stable again in the newer versions.

@ifacify
Copy link

ifacify commented Nov 29, 2023

Also a carpet custom payload packet translator across 1.20.2+ and 1.20.2- can be made without too much effort.

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.

3 participants