MinecraftForge/MinecraftForge

Allow enchantments to modify damage based on the entity hit

Open

#6,564 opened on Mar 14, 2020

View on GitHub
 (6 comments) (0 reactions) (0 assignees)Java (6,538 stars) (2,701 forks)batch import
1.181.19FeatureHelp Wanted

Description

Enchantments can only modify the dealt damage based on the entity's CreatureAttribute, not on the entity object itself. Therefore it's not possible to e.g. change the damage based some capability of a player. More information in the linked PR below.

I once made a PR https://github.com/MinecraftForge/MinecraftForge/pull/4052, which was eventually declined due to some thread safety concerns by Lex. However, the issue is still present. I don't assume Lex got around talking to grum about this, but I noticed that in PlayerEntity#attackTargetEntityWithCurrentItem the entity object is passed to a CriticalHitEvent just a few lines after the enchantment damage calculation, which means it is also available to modders.

I do not exactly know what's the thread safety issue here (as I don't see any difference from other parts of MC), but if there is a potential issue, then it is already exposed by said event. So why not add the enchantment thing as well.

If you agree, I would update the PR for 1.15 and make sure to add a comment that the entity should not be modified in that method.

Contributor guide

Tech stack
java
Domain
backend
Issue type
feature
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
4
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
1-2 days
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
stale
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
mostly clear
Prerequisites
JavaMinecraft moddingunderstanding of enchantment system
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
20
Research direction
The issue requests a way for enchantments to modify damage based on the entity hit, referencing a previous closed PR (#4052) that was declined due to thread safety concerns. The author notes that PlayerEntity#attackTargetEntityWithCurrentItem already passes the entity to CriticalHitEvent, suggesting a similar pattern could be used. To implement, review PR #4052, understand the thread safety issue, and propose a solution that uses the same entity reference as CriticalHitEvent but ensures thread safety, perhaps by documenting that the entity should not be modified.