Skip to content
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

After Damage Event #4051

Merged
merged 9 commits into from
Aug 26, 2024
Merged

Conversation

TheDeathlyCow
Copy link
Contributor

Adds a server-side only entity event after an entity is damaged. The event is fired after damage is applied, and includes both damage dealt and damage taken. However, damage taken is a bit weird. Its not the final amount of health reduction, but the damage the entity receives just armour / enchantments are applied and after shield, freeze_hurts_extra_types, and helmet damage reduction. The event is not fired if the damage killed the entity.

I'm not sure if the difference between damage dealt and damage taken is clear enough that they should be separate here. Maybe it would be best to only supply the damage taken? These two numbers are both supplied to the player_hurt_entity and entity_hurt_player advancement criteria, so I thought their inclusion here made sense. However, as this is a fairly easy change to make I thought I would just open this now to see what people think.

@modmuss50 modmuss50 added enhancement New feature or request small change labels Aug 23, 2024
* is applied, or after that damage was blocked by a shield.
*
* <p>Damage dealt is the damage initially applied to the entity. Damage taken is the amount of damage the entity
* actually took, after effects such as shields and extra freezing damage are applied. Damage taken does NOT include
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it should be renamed to baseDamageTaken or something else to make it clear its not the final amount of damage?

PR looks good otherwise 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, pushed change 👍

@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Aug 26, 2024
@modmuss50 modmuss50 merged commit 2122d82 into FabricMC:1.21.1 Aug 26, 2024
4 checks passed
modmuss50 pushed a commit that referenced this pull request Aug 26, 2024
* after damage event

* add after damage event to testmod

* remove amount > 0 check to capture shield blocking

* add javadoc

* dont fire event if killed

* clarify javadoc a bit more

* fix checkstyle issue

* fix other checkstyle issues lol

* rename damageDealt to baseDamageTaken

(cherry picked from commit 2122d82)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request merge me please Pull requests that are ready to merge small change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants