-
-
Notifications
You must be signed in to change notification settings - Fork 125
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
Damage Pipeline Update #792
base: 1.20.x
Are you sure you want to change the base?
Conversation
|
- function-based modifiers - interface for damage container - javadocs
Updated with input from discussions on discord. Original post has been updated to reflect these changes as well. |
src/main/java/net/neoforged/neoforge/common/damagesource/DamageContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/entity/living/ArmorHurtEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/entity/living/DamageBlockEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/entity/living/DamageBlockEvent.java
Outdated
Show resolved
Hide resolved
Going to tentatively set this as targeting 1.21, due to the scope of other changes that are currently slated for 1.20.5. If we find ourselves in an accelerated timeline before the 20.5 stable release, we can revisit this decision. |
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 have time to finish all the files atm (I got about halfway through) but I'll submit this review set thus far. Lots of little changes that need to be made for sanity.
One thing I think needs to be looked at on a broad level is the event naming. The sequence doesn't have a coherent name set, and we have two clashing names that (at a glance) would appear to be the "initiating" event - EntityPreDamageEvent
and IncomingDamageEvent
. That might be better discussed on discord though.
|| this.invulnerable && !p_20122_.is(DamageTypeTags.BYPASSES_INVULNERABILITY) && !p_20122_.isCreativePlayer() | ||
|| p_20122_.is(DamageTypeTags.IS_FIRE) && this.fireImmune() | ||
|| p_20122_.is(DamageTypeTags.IS_FALL) && this.getType().is(EntityTypeTags.FALL_DAMAGE_IMMUNE); | ||
+ return net.neoforged.neoforge.common.CommonHooks.onEntityInvulnerablityCheck(this, p_20122_, isVanillaInvulnerable); |
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.
Attempting to fire the event here is risky, but firing it at the call sites is untenable. At the least, I don't think we can reasonably provide the isVanillaInvulerable
field since it is not necessarily correct - the common case of overrides to this method check additional conditions and then &&
them with super.isInvulnerableTo
.
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.
The only overrides where this occurs is:
- Warden: checks if digging animation is active and damage source bypasses immunity
- Ghast: checks entity invulnerability, same damage source check, and if the damage is not a deflected fireball
- Breeze: damage source is in the breeze immune tag or source is from a breeze (self included)
I am of the opinion that the immunity bypass tags are compatible with this feature mainly because they are configurable. A user or modder still has control over the tag contents to handle immunity if it matters for these 3 mobs. The other aspects relate to reasonable invulnerabilities (breeze not affecting itself, warden (de)spawning). The Ghast fireball is the only other one that might be worth patching. I agree that patching each of these mobs for the sake of uniformity would be untenable in the long run though.
I don't think we can reasonably provide the isVanillaInvulerable field since it is not necessarily correct
Perhaps rename? This way it's clearer that it isn't absolute in its scope.
This PR aims to create a more cohesive chain of events when entities are dealt damage.
For an in depth explanation of the need for this PR, please see This writeup by Tslat.
Overview
this PR adds two (2) new events, renames four (4), an override method for damage, and adds a protected field (
damageContainer
) toLivingEntity
to track damage throughout the damage sequence.DamageContainer
InterfaceThis new field in
LivingEntity
tracks everything that happens during the sequence and is passed to each event. This means that for a single attack, no detail about the sequence from a prior step is obscured or inaccessible. For example, in later events, the modified and original damage are referenceable. Additionally values such as shield blocked damage, armor reduction, enchant reduction, absorption reduction, and mob effect reduction are all accessible (provided they are set at that point in the sequence). Since this container is provided to each event, it is also where modifications take place so that they persist between events.When the sequence is complete, this object is nullified until a new attack is received. This is also a protected method to prevent access from the outside, since its sole purpose is to be a transient container for event-driven modifications happening internally.
The Sequence at a High Level
The "damage sequence" starts when the entity's
hurt
method is invoked and ends when damage is applied to the entity health. the sequence is as followshurt
EntityInvulnerabilityCheckEvent
(new)EntityPreDamageEvent
(formerlyLivingAttackEvent
)DamageBlockEvent
(formerlyShieldBlockEvent
)actuallyHurt
ArmorHurtEvent
(new)IncomingDamageEvent
(formerlyLivingDamageEvent
)DamageTakenEvent
(formerlyLivingHurtEvent
)Invulnerability Check
Currently the vanilla invulnerability check prevents modders from having any decision power on the receiving end of damage if vanilla decides the entity is invulnerable. The new event provides the following benefits:
EntityPreDamageEvent
(formerlyLivingAttackEvent
)Renamed to indicate this is the first stage in the damage sequence. This event is where most of the change should happen. Through
DamageContainer#addModifier
armor, enchant, mob effect, and absorption values are changed through the provided callbacks. Changing the damage in this event will have the most downstream effects.Shield changes and the new
DamageBlockEvent
(formerlyShieldBlockEvent
)The current
ShieldBlockEvent
is nested inside vanilla checks as to whether the entity is blocking. Like the invulnerability check, this was decision power taken from modders. The new event is now part of the condition, so the damage blocked, the damage to the shield, and whether the player is considered blocking are all handled by the event.ArmorHurtEvent
(new) and Reductions from Armor, Magic, and AbsorptionThere is currently no way to modify damage to armor durabililty except to modify the damage to the entity through two different events. This event gives explicit control over the armor durability damage portion. Furthermore, the modifiers for damage reduction from armor, enchantments, mob effects, and absorption are applied in the
DamageContainer
instance.IncomingDamageEvent
(formerlyLivingDamageEvent
)Renamed to indicate this is damage incoming to the entity and cannot be interrupted. The event is no longer cancellable for this reason. However, setting the damage to zero does still terminate the damage sequence. At this stage, all reductions have taken place and have been applied to the
newDamage
value. This is the last chance for users to change the final damage to the entity, but it will not revert and armor or shield durability losses.DamageTakenEvent
(formerlyLivingHurtEvent
)Renamed to indicate this controls the damage that will be applied to the entity. This event is not cancellable, and receives an immutable version of the DamageContainer. This event works more as a trigger for events that need to fire when an entity takes damage AND needs to know the amount of damage received.
added
ILivingEntityExtension#onDamageTaken
a default noop method that executes after the damage sequence has finished and allows custom entities a means to apply custom behavior during
actuallyHurt
without having to override the method.