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

Update Forge registries with new ID limits #5531

Open
wants to merge 2 commits into
base: 1.13.x
from

Conversation

Projects
None yet
6 participants
@bs2609
Copy link
Contributor

commented Feb 19, 2019

This PR updates GameData to account for most ID limits being removed as of 1.13.2, as well as removing some commented out code for dealing with block metadata that is no longer relevant.

In particular, there is no longer an ID limit on blocks, items, and biomes now.

As unlimited IDs are the norm, max IDs are only specified where they apply, and the registry simply defaults to Integer.MAX_VALUE otherwise.

For reference, the original max IDs imposed here come from c44ed8f.
This is no longer required due to the initial availability map being size-limited since b203468 - the integer overflow can be dealt with simply by performing the addition after capping the value here.

Note that as BitSets can only be sized as a multiple of the word size used, an odd size is not meaningful here - this does not change the logic at all.

Double-checking would of course be appreciated, the vanilla protocol is well documented here: https://wiki.vg/Protocol.

@LexManos

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Registry information is still sent over the wire. you need to track down where and justify that it has been made "unlimited". As nothing is unlimited, most likely they are serialized as a varint somewhere. that needs to be documented. there has also been times where mojang attempted to remove limits but missed a packet or two. so, verify.

@williewillus

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Checking things that had limits:

  • Items: varint (PacketBuffer)
  • Blocks/Blockstates: varint (everywhere)
  • Biomes: int (AnvilChunkLoader)
  • Potion: Still constrained to byte (255) by SPacketEntityEffect
  • SoundEvent: varint
  • PotionType: not serialized to integers at all
  • enchantment: saved as registry name in stacks now, can be int. However, the enchanting gui still uses the container methods to sync the hover tooltip hints, which truncate silently to short. If we can accept that breaking with too many enchantments we could use int, otherwise the old limit of short remains.
  • entitytype: varint (SPacketSpawnMob, FMLPlayMessages)
  • tileentitytype: not serialized to integers at all
  • villager professions: int (put in the vanilla EntityDataManager)

so the interesting thing is why the old code had limits for things that used varint. It should be completely fine to use up to int max for this, since vanilla's readVarInt allows you to have 5 bytes, which with 7 significant bits per byte comes out to 35 bits, definitely enough to hold signed int max.

@LexManos

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

@bs2609 This PR can not be merged until all the max values and usecases are documented in the code.
You delete all those documentation lines. There was a reason they are constants and not inline in the one usage.

As for why we had limits before, vanilla changes. The limits were from before vanilla moved a lot of things to varint.

@LexManos LexManos assigned tterrag1098 and unassigned LexManos Apr 16, 2019

@mezz

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Please update this to resolve conflicts.

@Hypsellis

This comment has been minimized.

Copy link

commented Jun 9, 2019

Is there no way to extend the potions ids as for the enchantments ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.