yarn icon indicating copy to clipboard operation
yarn copied to clipboard

ArmorMaterial and ToolMaterial interface overlap

Open ghost opened this issue 4 years ago • 1 comments

The interfaces ArmorMaterial and ToolMaterial have overlapping method names meaning trying to implement both under one singular "material" instance at a time results in the inability to differentiate between the two. This is often desirable as even the vanilla materials have discrepancies between the armor and tool materials (the enchantability of iron armor is different than that of iron tools). Doing so would be a small quality of life improvement over needing a separate ArmorMaterial instance and ToolMaterial instance for every set of custom tools and armor.

The overlap is as follows:

  • Both share a method called getDurability however the one in ArmorMaterial accepts an EquipmentSlot whereas the one in ToolMaterial does not. Technically I guess these two don't clash but I can imagine it is a little bit confusing implementing both on the same class at the same time.
  • Both share a method called getEnchantability with the exact same signatures.
  • Both share a method called getRepairIngredient with the exact same signatures.

My recommendation is to rename these methods in their respective interfaces as follows:

  • getDurability -> getArmorDurability and getToolDurability respectively.
  • getEnchantability -> getArmorEnchantability and getToolEnchantability respectively.
  • getRepairIngredient -> getArmorRepairIngredient and getToolRepairIngredient respectively.

I understand that internally within each interface the addition of these clarifiers is redundant, but in actual practice I believe it would be beneficial to differentiate these methods.

I'm creating an issue here because I have literally no idea how to make a PR for yarn and no idea how to suggest mapping changes etc. If someone would like to give me a quick rundown, or if there's a document you can point me to explaining the process I'd be more than happy to go in and make a PR for this.

ghost avatar Jul 21 '21 04:07 ghost

@sfPlayer1 I recall forge had such issues before and they create extra mappings to fix that. Should we fix this through remapper adding custom bridges instead?

liach avatar Jul 21 '21 10:07 liach