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

Eliminated ArmorMaterial and ToolMaterial overlap #2601

Open
wants to merge 1 commit into
base: 1.17.1
Choose a base branch
from
Open

Eliminated ArmorMaterial and ToolMaterial overlap #2601

wants to merge 1 commit into from

Conversation

Vaerian
Copy link

@Vaerian Vaerian commented Jul 21, 2021

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.

@haykam821 haykam821 added refactor A PR that renames existing names. release A PR that targets a release version of Minecraft labels Jul 21, 2021
@liach
Copy link
Contributor

liach commented Jul 21, 2021

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
Copy link
Contributor

liach commented Aug 31, 2021

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

@modmuss50
Copy link
Member

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?

@Shnupbups
Copy link
Contributor

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.

@haykam821 haykam821 added outdated A PR that targets an outdated branch. impactful A change that is likely to affect some mods. Will require more reviews. labels Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impactful A change that is likely to affect some mods. Will require more reviews. outdated A PR that targets an outdated branch. refactor A PR that renames existing names. release A PR that targets a release version of Minecraft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ArmorMaterial and ToolMaterial interface overlap
5 participants