You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
The text was updated successfully, but these errors were encountered:
ghost
linked a pull request
Jul 21, 2021
that will
close
this issue
@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?
The interfaces
ArmorMaterial
andToolMaterial
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 separateArmorMaterial
instance andToolMaterial
instance for every set of custom tools and armor.The overlap is as follows:
getDurability
however the one inArmorMaterial
accepts anEquipmentSlot
whereas the one inToolMaterial
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.getEnchantability
with the exact same signatures.getRepairIngredient
with the exact same signatures.My recommendation is to rename these methods in their respective interfaces as follows:
getDurability
->getArmorDurability
andgetToolDurability
respectively.getEnchantability
->getArmorEnchantability
andgetToolEnchantability
respectively.getRepairIngredient
->getArmorRepairIngredient
andgetToolRepairIngredient
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.
The text was updated successfully, but these errors were encountered: