yarn icon indicating copy to clipboard operation
yarn copied to clipboard

Eliminated ArmorMaterial and ToolMaterial overlap

Open ghost opened this issue 4 years ago • 4 comments

Closes #2600

Further information about why can be found in the linked issue.

There was discussion with @haykam821 on Discord about it not being great practice to change mappings solely to eliminate overlap and I would generally agree with that, however it seems like this is a fairly reasonable implementation choice that could become standard for developers, and the only alternative I can think of involves writing a separate class with helper methods to convert to the two interfaces depending on what's explicitly necessary and I think that's a little bit overkill for developers to implement for something so simple. That being said if it is decided that this PR shouldn't be merged for that reason, then maybe it makes sense to have a helper class like I described somewhere in fapi to aid in creating large volumes of armor and tool sets.

ghost avatar Jul 21 '21 04:07 ghost

Hmm, I think we come back to the old inheritance vs holding debate, like on whether we should carry the armor/tool material for an item through creating a subclass implementing both or a class containing getters for both.

My suggestion is that you for now can create a pojo or builder that takes some parameters to maximize the shared data and the built object has getters for the armor and tool materials.

liach avatar Jul 21 '21 10:07 liach

@modmuss50 How do you think of these names? I personally think if you do create a class for definition of materials, you should make them hold two fields of ArmorMaterial and ToolMaterial types than having it implement both interfaces.

liach avatar Aug 31 '21 22:08 liach

It wouldnt be my choice to make them one class, but I can see why people would. Removing the clash is a good idea, but something for 1.18 imo? Not sure what you guys think?

modmuss50 avatar Aug 31 '21 22:08 modmuss50

Probably too late in the 1.18 release cycle for a refactor of this magnitude now, let alone in 1.17, so if we are to truly consider this it'd likely be for 1.19 snapshots.

Regardless, this could be a good change. Personally I'm fine with the idea of having one class that serves as both a ToolMaterial and ArmorMaterial, though it's probably best practice to keep them separate in mods. Having the option to combine them could be useful for some people though.

Shnupbups avatar Nov 23 '21 16:11 Shnupbups