FabricMC / yarn

Libre Minecraft mappings, free to use for everyone. No exceptions.
Creative Commons Zero v1.0 Universal
923 stars 380 forks source link

ArmorMaterial and ToolMaterial interface overlap #2600

Open ghost opened 3 years ago

ghost commented 3 years ago

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:

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

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.

liach commented 3 years ago

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