-
Notifications
You must be signed in to change notification settings - Fork 257
Make Rich Soil use a HashMap for Mushroom Colonies #1089
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
base: 1.21
Are you sure you want to change the base?
Conversation
|
i suport this |
| public static HashMap<Item, MushroomColonyBlock> COLONIES = new HashMap<>(); | ||
| public static final int PLACING_LIGHT_LEVEL = 13; | ||
| public final Holder<Item> mushroomType; | ||
| public final Item mushroomType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a proper change, Holder should be used as addons could register mushroom colonies for their own mushrooms, where the item is registered after the blocks and thus not available in the block's constructor.
Also, the COLONIES field should not be exposed, use proper register and get with checks for duplicate registers, also since the Item is not available in the constructor, do not register to the COLONIES in the constructor, move it FMLCommonSetupEvent instead.
Since we are moving away from hardcoding, let's not restrict the value class to MushroomColonyBlock, and the key class could just use Block instead of Item since we are not doing the register in MushroomColonyBlock's constructor anyways.
.gitignore
Outdated
| *.iml | ||
| .idea | ||
|
|
||
| # vscode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what occurred in your workspace, but I'd rather leave the .gitignore unchanged. Can you please revert this edit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, done 👍️
| }; | ||
|
|
||
| public MushroomColonyBlock(Holder<Item> mushroomType, Properties properties) { | ||
| public MushroomColonyBlock(ItemLike mushroomType, Properties properties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's good to use ItemLike as the key, the Item, Block, DefferedItem and DefferedBlock all matches, but neither of them are equal to others by implementation.
Since you are using the Item to get value from the map, a modded colony which passed in a DeferredItem (or DefferedBlock) as the mushroomType parameter will simply fail when checked by the Rich Soil.
Please just revert this to use the Holder<Item> as the parameter type and key type, Holders are basically singleton and has proper equals and hashCode implementation on NeoForge, even on Fabric the Holders should still work totally fine as long as it's the Holder.Reference from registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively just move the registration away from the constructor as requested above (the Apr 7 one), which would eliminate the need of laziness of the key.
This allows addons to add their own Mushroom Colonies without needing to mixin
RichSoilBlock.This also changes the
MushroomColonyBlockto use anIteminstead of aHolder<Item>, as thebuiltInRegistryHoldermethod is deprecated and using anItemworks fine.