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

ArmorMaterial and ToolMaterial interface overlap #2600

Open
ghost opened this issue Jul 21, 2021 · 1 comment · May be fixed by #2601
Open

ArmorMaterial and ToolMaterial interface overlap #2600

ghost opened this issue Jul 21, 2021 · 1 comment · May be fixed by #2601

Comments

@ghost
Copy link

ghost commented Jul 21, 2021

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 ghost linked a pull request Jul 21, 2021 that will close this issue
@liach
Copy link
Contributor

liach commented Jul 21, 2021

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant