search

TerraformersMC / Campanion

A camping companion mod that adds items and utilities to improve your life away from home
GNU Lesser General Public License v3.0
81 stars 31 forks source link

Loyalty enchantment is still applicable to Spears #43

Closed ejektaflex closed 4 years ago

ejektaflex commented 4 years ago

I just enchanted a Spear with Loyalty III. It seems to have no effect. After checking the source for SpearEntity, there seems to be no tracking of the loyalty enchant like there is in TridentEntity. For the time being, this means that Loyalty enchantments do not work on Spears (even though Spears can accept Loyalty enchantments).

Prospector commented 4 years ago

What version are you on? Loyalty should not be possible to get on spears, although I may have fixed it on 1.16 but not 1.15

ejektaflex commented 4 years ago

This is on 1.16. I just realized what the problem is. My mod is adding random enchantments to items, and checks to see if it is a valid enchantment by calling Enchantment::isAcceptableItem on the Loyalty enchantment. It passes true, I assume because the Loyalty enchantment has the enchantment target EnchantmentTarget.TRIDENT. That target defines isAcceptableItem() == true if the item is a TridentItem, and a spear is indeed a TridentItem.

So therefore, if you extend TridentItem, Loyalty will always be a valid enchantment.

Solution 1 that I can see: Add a Mixin to EnchantmentTarget.TRIDENT that ensures that the item is not a SpearItem

Solution 2: Just change it so that Loyalty works on spears? I was honestly more excited for that, haha. I'd love to see Loyalty working on spears if possible. 😄

Wyn-Price commented 4 years ago

Loyalty on spears doesn't really make sense. We already have a mixin for enchantments (https://github.com/TerraformersMC/Campanion/blob/1.16/src/main/java/com/terraformersmc/campanion/mixin/MixinEnchantment.java) but what I assume has happened is the loyalty enchantment predicate changed from 1.15 -to 1.16, with before just checking to see if the item is the trident item.

ejektaflex commented 4 years ago

What makes sense about Loyalty on a trident and not a spear? I'm not quite following that logic.

Wyn-Price commented 4 years ago

Pretty much everything in this mod is meant for early game or a nomadic lifestyle. Having unbalanced, easy to make, items ruins that. If we were to add loyalty then they would essentially just be an early game craftable trident. We intentionally removed the loyalty code from the spear entity, and I'm highly doubtful we'll add it back

ejektaflex commented 4 years ago

Ah yes, that's true. I suppose that if a spear has Loyalty, there no reason to ever have a trident. I do see your point!

Any any case, this is still a bug so I assume that yeah, the predicate has changed. A fix would be excellent :^)