Minecraftforge: EnumEnchantmentType.ALL and EnumHelper.addEnchantmentType() don't work well together

[HenryLoenwind]

The EnumEnchantmentType.ALL will allow enchantments to go on items if any other EnumEnchantmentType allows it. However, this includes new types added with EnumHelper.addEnchantmentType(). This changes what items vanilla enchantments can go on as side effect.

I would recommend to change EnumEnchantmentType.ALL to only loop over the vanilla EnumEnchantmentTypes, not new ones added by mods to preserve the vanilla behavior. A way to add new EnumEnchantmentTypes to that list would be a bonus, as this would allow mods to change vanilla behavior intentionally (e.g. allowing those "ALL" enchantments to go on new tool classes like wrenches).

---

Use case: The Ender IO enchantment "Soulbound" should be able to be applied to every item players can have in their inventory. To make this possible without taking away control from items by ignoring the Item.canApplyAtEnchantingTable() callback, a new EnumEnchantmentType is needed. But adding one for "really all items" would also extend EnumEnchantmentType.ALL (and with it those vanilla enchantments that use it) to go on "really all items".

[KingLemming]

Such a change would actually break enchantments in a lot of other mods, because some of them currently rely on "ALL" to mean "enchantable by everything," as the code would suggest.

For example, CoFHCore's Soulbound actually uses EnchantmentType.ALL because I don't see a specific need for Porkchops to be made Soulbound, and the idea that if an item is enchantable by anything is good enough.

So what you're suggesting right now is just breaking things. I could possibly see doing VANILLA, ENCHANTABLE, and ACTUALLY_YES_EVERYTHING_INCLUDING_PORKCHOPS. But again, I personally think that last one is kind of stupid.

[HenryLoenwind]

Wrong way around. Me using enumhelper to add my own all type WITHOUT a forge change would have this unwanted effect.

Sent from my iPhone

On 23. May 2018, at 18:19, KingLemming [e-mail] wrote:

Warning: This message has had one or more attachments removed
Warning: (msg-16554-30.html).
Warning: Please read the "j-e-b-Attachment-Warning.txt" attachment(s) for more information.

Such a change would actually break enchantments in a lot of other mods.

For example, CoFHCore's Soulbound actually uses EnchantmentType.ALL because I don't see a specific need for Porkchops to be made Soulbound, and the idea that if an item is enchantable by anything is good enough.

So what you're suggesting right now is just breaking things. I could possibly see doing VANILLA, ENCHANTABLE, and ACTUALLY_YES_EVERYTHING_INCLUDING_PORKCHOPS. But again, I personally think that last one is kind of stupid.

--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
https://github.com/MinecraftForge/MinecraftForge/issues/4946#issuecomment-391408528

This is a message from the MailScanner E-Mail Virus Protection Service

The original e-mail message contained potentially dangerous content,
which has been removed for your safety.

The content is dangerous as it is often used to spread viruses or to gain
personal or confidential information from you, such as passwords or credit
card numbers.

Due to limitations placed on us by the Regulation of Investigatory Powers
Act 2000, we were unable to keep a copy of the original attachment.

The content filters found this:
MailScanner: Found a script in HTML message

--
Postmaster

MailScanner thanks transtec Computers for their support

[KingLemming]

Not exactly, no. You want ALL to ignore any EnchantmentType added by a mod. But some mods are relying on that behavior to figure out what is enchantable.

At the same time, you want to add a type which does apply to literally everything. So I get where you're coming from, because that will prevent your crazy type from breaking things.

However, that will also break the functionality of ALL where it will apply to EnchantTypes which are mod-added and do not otherwise apply to other vanilla enchant types. Such as the "Holding" enchants.

[HenryLoenwind]

So you don't want pork chops to be enchantable with COFH:Soulbound, but if someone adds an enchantment "extra nourishing" that can only be applied to food items, then you also want pork chops to be enchantable with COFH:Soulbound with no way for either you or the other mod to stop that? (AND have the vanilla "ALL" enchants apply top those, too)

[KingLemming]

Actually, thinking about it, my enchant will already not apply due to the minEnchantability check, because food isn't flagged as enchantable. I'm pretty sure all the Vanilla Enchants also have that.

[Alpvax]

Maybe there should be a flag to determine whether or not the EnchantType falls under the ALL group, then it would be entirely up to the mods adding the types.

[HenryLoenwind]

Alpvax, that was my original suggestion, just with 2 method calls instead of one that takes a boolean flag...

[Alpvax]

Oh, yes, I missed the final part of your original post

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time. If this issue is still relevant and should remain open, please reply with a short explanation (e.g. "I have checked the code and this issue is still relevant because ___." or "Here's a screenshot of this issue on the latest version"). Thank you for your contributions!

[MajorTuvok]

This seems to be still relevant to me - and maybe considered more closely for 1.13?

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time. If this issue is still relevant and should remain open, please reply with a short explanation (e.g. "I have checked the code and this issue is still relevant because ___." or "Here's a screenshot of this issue on the latest version"). Thank you for your contributions!

[stale[bot]]

This issue has been automatically closed because it has not had activity in a long time. Please feel free to reopen it or create a new issue.