neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.16k stars 168 forks source link

Reimplement `LootingLevelEvent` #1112

Open Shadows-of-Fire opened 3 months ago

Shadows-of-Fire commented 3 months ago

As part of the data-driven Enchantment changes, the way that the original LootingLevelEvent worked was inoperable, and as such, the event was dropped.

However, there is an opportunity to implement a similar event within EnchantedCountIncreaseFunction#run, though it will have to be more broad as this function can be executed with an arbitrary enchantment, instead of being restricted only to Looting.

Additionally, we could have this new event cover both fortune and looting, by also firing it from ApplyBonusCount#run.

sciwhiz12 commented 2 months ago

Note that not only EnchantedCountIncreaseFunction#run would need to be patched for such an event, but also LootItemRandomChanceWithEnchantedBonusCondition#test. (For example, see the loot table for Zombies.)

Fuzss commented 2 months ago

EnchantmentHelper::processEquipmentDropChance also would need a patch specifically checking for looting. This is the method responsible for the chances for dropping e.g. armor worn by monsters.

SiverDX commented 1 month ago

Since in the first both cases EnchantmentHelper#getEnchantmentLevel is called and said method now has context to the entity (i.e. player), wouldn't it be possible to add an event there to modify any enchantment level?

The fortune one seems to call the getItemEnchantmentLevel(Holder<Enchantment> holder, ItemStack itemStack) one So maybe add the event there and potentially set an entity context in getEnchantmentLevel(Holder<Enchantment> holder, LivingEntity livingEntity) (which calls the itemstack one)

Meaning the event may or may not have an entity context, depending from where it's called

Fuzss commented 3 weeks ago

That might be worth looking into for a separate event. The entity in LootingLevelEvent is the attacked entity, not the attacker which is passed to getEnchantmentLevel(Holder<Enchantment> holder, LivingEntity livingEntity). Also the DamageSource context of LootingLevelEvent is important (at least I have used it).

Fuzss commented 3 weeks ago

So I just implemented a replacement for LootingLevelEvent in my own library renamed as ComputeEnchantedLootBonusEvent. I'd much appreciate if someone could just copy / adapt my code to NeoForge, I'm not really familiar with the contribution workflow unfortunately.

Here is the short documentation:

/**
 * Called just before a {@link LivingEntity} drops all its loot for determining the level of a loot bonus enchantment
 * such as {@link net.minecraft.world.item.enchantment.Enchantments#LOOTING} that should be applied to the drops.
 * <p>
 * Specifically the event allows for controlling the enchantment level when applying the:
 * <ul>
 *     <li>loot item function <code>minecraft:enchanted_count_increase</code></li>
 *     <li>loot item condition <code>minecraft:random_chance_with_enchanted_bonus</code></li>
 *     <li>enchantment effect component <code>minecraft:equipment_drops</code></li>
 * </ul>
 * <p>
 * This event is fired on the {@link NeoForge#EVENT_BUS}.
 */

Key differences from the old event are:

Implementing the event requires these three hooks: