Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Fix a few issues with ItemMeta

Open Lulu13022002 opened this issue 1 year ago • 4 comments

NPEs:

  • OminousBottleMeta#getAmplifier when ominous_bottle_amplifier data component is absent
  • TropicalFishBucketMeta when variant is null for almost all the color getter
  • AxolotlBucketMeta#getVariant when variant is null
  • ItemMeta#getAttributeModifiers(Attribute) when attribute modifiers is null
  • SpawnEggMeta#getSpawnedEntity when entityTag is null

Codec/item validation: This often kick the player holding the bad item instantly without saving the last data.

  • FoodComponent#addEffect with out of bound probability (outside 0-1)
  • the default food component created by Spigot (ItemMeta#getFood) is invalid since 'eat_seconds' cannot be equals to zero
  • ItemMeta#set(Max)Damage doesn't check the range
  • BundleMeta#addItem/setItems with empty item (amount=0)

ItemMeta bad equality: This breaks the contract of equals that say x.equals(y) == y.equals(x)

  • CraftMetaEntityTag (equals)
  • CraftMetaSpawnEgg (equals)
  • CraftItemMeta (hashCode) that handle the max damage as a simple boolean and not as an integer

Misc:

  • empty fireworks effect is cleared by the item meta conversion (firework_rocket[minecraft:fireworks={}])
  • charged projectile for CrossbowMeta should allow other items and not only arrow or fireworks especially since now the picked up item is custom
  • AxolotlBucketMeta#setVariant is not null but yet allow null to be handled as LUCY variant
  • ItemMeta#getAttributeModifiers(EquipmentSlot) create an empty attribute_modifiers component, but getter shouldn't affect the underlying data
  • mitigate crashes of https://github.com/PaperMC/Paper/issues/10677, pattern is lost however (need to check the event)
  • Book page limit is infinite however internal still limit to 100 pages for writable book
Known issues (unfixed): - No way to set some components: base_color (shield), intangible_projectile (for crossbow, in charged_projectiles component)
- The LeatherArmorMeta wipe default leather color and invalid color, also fallback to the default leather color even for wolf armor which doesn't make sense
- Shield with base_color component gets an empty banner_patterns components once moved into the inventory

Opened this as a draft since there are more stuff to check

Lulu13022002 avatar May 17 '24 20:05 Lulu13022002

having a skim, so far outside of what MM has pointed to, this generally looks fine; I'd need to double check that API def of that method, however

electronicboy avatar May 17 '24 22:05 electronicboy

having a skim, so far outside of what MM has pointed to, this generally looks fine; I'd need to double check that API def of that method, however

Yeah, we don't have to change the API, but then we have to change where that method is used in the implementation so that in hashCode and equals and serialization they aren't treated the same.

Machine-Maker avatar May 17 '24 22:05 Machine-Maker

Concern is more "what's empty" for me

electronicboy avatar May 17 '24 22:05 electronicboy

  • Shield with base_color component gets an empty banner_patterns components once moved into the inventory

I think this is fixed in https://github.com/PaperMC/Paper/pull/10731 but I would appreciate you testing this as well. That PR should fix a lot of issues that are on itemtypes that can hold BlockEntities.

Machine-Maker avatar May 20 '24 00:05 Machine-Maker

Moved to ready for review, further changes can be their own PR.

lynxplay avatar May 21 '24 22:05 lynxplay