#Damage Events

3554 messages · Page 4 of 4 (latest)

quick kernel
#

internal annotation working wonders

#

maybe we shouldn't just be marking the entire container as internal -.-

sand copper
#

I do have it local. i'm making sure the change works as intended before committing that part

sand copper
# quick kernel

already removed in local. same as my comment above, getting gametests working before doing another commit

quick kernel
#

👍

sand copper
#

I'm thinking it's probably a good idea to add comments to my gametest to explain what it's doing, but none of the other tests have that. Are we avoiding the clutter of comments or was it just not done in the past and probably should be?

sand copper
#

added them for now. I figure it's trivial to remove if we decide we don't want them.

#

alright. PR changed from draft to open.

quick kernel
#

alright so

#
  • Helper methods ?
  • Removing clamp on IReductionModifier ?
  • Javadoc fixes ?
  • Logic fix on absorption ?
  • Remove internal annotation from Damage Container ?
#

looks like it's all covered

sand copper
#

yeah. The only change i realize i could go back and do it swap the use of persistent data to data attachments. I was just glad to get the test finally working i forgot to go back and do that.

quick kernel
#

looks like you still have the broken javadocs

#

and even added more

sand copper
#

that's how they are supposed to be per Maty.

quick kernel
#

they're supposed to be broken?

#

wild

#

I'd prefer a standard where they didn't look broken but whatever lol

sand copper
#

if i read correctly what people said above, it's just a display bug with intelliJ not an incorrect format

#

you are referring to the /**{@return words and more words}*/ syntax right?

sand copper
#

alright. gametest refactored to use attachments. That was delightfully easy to switch to

maiden dove
sand copper
#

Can someone pin the PR for quick reference?

tender haven
#

There ya go

sand copper
#

thanks

quick kernel
#

@sand copper
Since the PR is still open
Thoughts onchanging this parameter to be damageIn?

image.png
#

it's a misleading name currently

sand copper
#

it's not misleading, the value passed into that method is the reduction value from vanilla. the damage is changed after this method is returned and the outgoing value is applied to the damage

#

so like in the gametest, the reduction amount on 20 points of damage for wearing a diamond chestplate is 1.959991 That gets passed into this function, not the 20. we add 2 in our gametest, then the damage amount is reducted to 16.040009

sand copper
#

I don't want to seem pushy, but i also don't know the internal procedures. Do i need to request a review of this PR or will that happen automatically?

quick kernel
#

just poke it

quick kernel
#

that doesn't seem right

sand copper
#

not a full set. just the chestplate. that's before enchantments too.

quick kernel
#

ah

swift bluff
sand copper
#

@quick kernel i made the change you requested.

quick kernel
#

👍

quick kernel
#

@sand copper Added review
Something feels really wrong there

#

like critically wrong

#

I've re-read it a bunch of times and either the patch format is blinding me, or that'll outright break absorption

sand copper
#

alright, then let's look at unpatched vanilla and figure out how it handles absorption and then see if these changes violate any logical assumptions. so here's unpatched vanilla

image.png
#

and the current patch (soft wrap on to condense SS size)

image.png
#

I think the line that bothers you is the removal of the latter setAbsorption call after setHealth.

#

here's how that plays out with sample data

image.png
#

the latter call does absolutely nothing. The setAbsorption call clamps the value to zero and the max absorption amount, so even with negative values, the actual amount stored is zero.

#

So, either there is some other mechanism i don't see, or this is leftover code from when it mattered that hasn't been removed since it does nothing.

#

the only change i could make to the patch is removing the Math.min on the setAbsorption call since the clamp already does what that was intending to do. That should probably change.

sand copper
quick kernel
#

how does this do anything other than getting the sum of all reductions

#

I can't see how it does anything other than that

sand copper
#

that's exactly what it is supposed to do.

quick kernel
#

no it's not

austere patrol
#

why not mapToFloat thonkies

quick kernel
#

it's meant to get the amount of damage absorbed

#

that's why it's there

#

I.E amount of damage that absorption eliminated

sand copper
#

oh! i see what you mean. it should be capturing the absorption reduction only

#

🤔 why does vanilla only award stats for absorption below 3.4...? does vanilla not have absorption over that amount?

quick kernel
#

3.4E37F

#

that's 3 with 37 0's after it

#

lol

#

I.E. max float

#

basically if it's a positive float

sand copper
#

facepalm damn scientific notation

quick kernel
sand copper
#

yeah. so f now just gets set wiith this.damageContainers.peek().getReduction(Reduction.ABSORPTION)

quick kernel
#

has to be clamped

#

or does it thinkies

#

should we allow more absorption than has absorption hearts?

#

I guess so?

#

anyway

sand copper
#

good question. I gotta get off my train. we currently allow the reudction to exceed the actual absorption amount and record that but obviously that isn't what's applied to the entity

quick kernel
#

the other issue here is the application of absorption in relation to damage incoming

#

I.e. you shouldn't be able to absorb more damage than there was damage to absorb

#

which doesn't appear to be the case here at all currently

#

it just looks to take the absorb amount the player currently has, then uses it

#

you really need to be clamping the reduction of absorbtion to the newDamage at that stage of the chain

#

which is what vanilla does here

image.png
#

it then naturally uses that clamped value to feed back into the reduction of the absorption on the entity

#

which we don't, we just.. reduce it by the full amount?

quick kernel
#

and also because of that, f will never be properly clamped either

#

I'm heading off for the night, but there's definitely some issues here to fix
the rest of the Pr looked fine, even if I think the inline braced javadocs are stupid as fk

sand copper
#

it's also worth reminding that this line does three things. It starts with a vanilla value of absorption, applies any stored reduction functions, stores this final reduction value, and applies that final reduction value to the new damage. Which means we are letting users decide what the absorption value should be. There is no enforcement of whether the value matches the actual value, because we don't really care. We consume whatever value the modders have told us to consume. Vanilla is going to cap that at the actual value when updating it's own record of absorption, but the damage reduction is going to follow a modded pattern which states it doesn't have to conform to the actual value.

image.png
sand copper
quick kernel
#

what I'm saying is the initial value you're inserting is the full absorption value

#

regardless of damage

#

so if I have 5 absorption, and do 3 damage

#

you're just saying it's gonna absorb 5 damage, even though there's only 3 to absorb

#

which then reflects onto where you setabsorptionamount again, cus you're just setting it based on the full amount

sand copper
#

oh, i see what you mean! that would mean having more absorption than the damage received would always heal you

quick kernel
#

no it' mean any time the damage is less than your absorption, you'd consume all absorption regardless

#

the initial input needs to be clamped at the value of the newDamage at that stage

#

and if users want to increase it after that (in the event) they can

sand copper
#

yes. that i agree

quick kernel
#

thne you have to recalculate the amount of damage absorbed, and use that for f

sand copper
#

what are you recalcualting for f? if you pass in the lesser of damage or absorption into the setReduction call and nothing modifies that value, you still get all of the damage that was absorbed. and if it is modified by a mod, you are still getting what the mod intended to be absorbed.

quick kernel
#

if a mod modifies the reduction, then the amount of absorption will change, no?

sand copper
#

yes

quick kernel
#

so that difference needs to be accounted for when re-setting absorption, and granting the stat

sand copper
#

it is?

#

as an example....

#

okay as I wrote this out i saw the issue. ugh this is getting messy

quick kernel
#

lol

sand copper
#

okay, we may want to reinstate that clamping on reductions. if you want to heal the mob do that as its own effect in the event.

quick kernel
#

this isn't really related to healing

#

this is just about ensuring we consume absorption properly

#

clamp the absorption value in setReduction to the current newdamage is step 1

then step 2 is basically just copy what vanilla had for f1, but with the reduction instead of the absorptionamount

sand copper
quick kernel
#

yeah I don't see a problem here with that

#

that can remain as-is, we just need to ensure we're sanitising the actual absorption values used

#

which has no real impact on the damage

#

the issues I've brought up are entirely unrelated to damage lol

#

it's all about the absorption

sand copper
quick kernel
#

we do, because getReduction isn't clamped by damage or 0 for this

#

so could say I want 5000 damage absorbed

#

but if the attack is only 5 damage

#

then only 5 damage goes into the stat

sand copper
#

it is now.

quick kernel
#

ah
I'lll wait for PR update then

#

current PR does not

sand copper
#

yeah. running checks now. then i'll push

quick kernel
#

👍

#

I'll be back in 20

sand copper
#

pushed

quick kernel
#

yeah still same issue

#

there's nothing stopping people arbitrarily breaking the stat gain because they set an absorb value higher than is possible to absorb

#

there's essentially two ways to approach a fix, depending on what ideology we take

sand copper
#

How? I clamped the modifier and the reduction to the damage value and zero.

quick kernel
#

oh I didn't even see that set/modify reduction clamp
That definitely shouldn't be there

#

now we're clamping values arbitrarily
If we have two mods that modify the value, one reduces it by 5 and the other increases it by 5
and the damage is 3

then what'll happen now is the resultant damage will be 5, whereas before it would've been 3

#

this breaks proper stacking modifiers

#

the modifiers should be allowed to basically go to any value, since stacking means any value is valid

#

then we sanitise the value before using it

#

I can tell this damage thing has got your head in a spin lol

#

clamp needs to go, then we have to decide on one of two ideologies for the last fix:

#

Ideology 1:
Users should be able to modify the entity's remaining absorption after an attack by amounts that don't make sense to the damage - I.E. Reducing absorption hearts by 6, even if they're only taking 3 damage

Ideology 2:
Users shouldn't be able to modify the entity's remaining absorption value outside of what is possible for the damage taken - I.E. you can only reduce the absorption hearts by up to the amount of damage the event has

#

then once that's decided on if you want you can send me the post-PR actuallyHurt method and I can show you want I mean for solutions on it

sand copper
quick kernel
#

I mean regardless of what we do

#

we can only reduce absorption value by the amount of absorption they have

#

so that's not really affected

#

if we're taking 3 damage, have 4 absorption hearts, and tell the event we want the absorption reduction to be 6, then the result would be:
Damage: 0
Absorption Hearts Remaining: 0
Container reduction value for absorption: 6

For Ideology 1

#

Results for ideology 2:
Damage: 0
Absorption hearts remaining: 1
Container reduction value for absorption: 6

sand copper
#

I think one is what we want. but one more scenario to make sure i understand these correctly. how woudl it look with 20 damage, 4 hearts, increased to 6 by modifiers?

sand copper
quick kernel
#

20 dmg, 4 absorb, 6 modified reduction

Scenario 1:
Damage: 14
Absorption hearts remaining: 0
Container reduction value for absorption: 6

Scenario 2:
Damage: 14
Absorption hearts remaining: 0
Container reduction value for absorption: 6

#

it only has an effect where the damage is less than the reduction value & absorb

#

because the issue stems from whether you should be able to consume more hearts than the damage could possibly consume

#

there's problems with both ideologies, so it's a "pick one" scenario

#

Ideology 1:
Pros: Allows for people to directly modify the resultant absorption value on the entity even if it doesn't align with the amount of damage being absorbed
Cons: Means that increasing the amount of absorption will unnecessarily and possibly unintentionally consume additional hearts even though they're not doing anything
Food for thought: If you want to directly modify the absorption hearts, you could hook damage post and modify it there?

Ideology 2:
Pros: Means that users can safely increase the absorption value without worrying about unintentionally consuming an entity's absorption despite it not being used to absorb anything
Cons: Can't use the modifier to directly modify the residual absorption on an entity

#

personally my pick is #2

#

since it protects innocent usage and you can still modify absorption with a later event

sand copper
#

🤔 I think it comes down to what's intuitive to users. I think if you say the absorption consumption is different than what was fed it, it should consume that amount. which means option 1 is still the better option.

quick kernel
#

cus if a user says they want the reduction to be 6, the question becomes - Do they want to consume 6 absorption even if the damage isn't 6? Or do they just want the maximum possible absorption to be 6, if there's enough damage to satisfy that

sand copper
#

so it's a matter of whether we want heart effectiveness to change instead of just heart consumption

quick kernel
#

heart effectiveness is untouched

#

it's purely down to heart consumption

#

should heart consumption be clamped to the amount of hearts actually consumed by the damage

#

or should be be based on the reduction value set

sand copper
#

i mean like if i'm reducing the value by 6 but only 3 hearts are being consumed the effectiveness of each heart is effectively doubled.

quick kernel
#

what if there's only 2 damage to begin with

#

should the user have 1 absorb heart remaining? or none

sand copper
#

so you're saying 2 damage in 3 hearts. if I want hearts to be twice as effective then in that case i'd expect only 1 heart consumed, but all damage reduced, which wouldn't work. Which as i write that i realize that heart effectiveness is not a viable approach for this API. we need hearts to equal damage (or at least we make that assumption). So then we need option 1 because if we have 3 abs and 2 damage and we double via modifier, then we have 0/0/4. likewise if we have 20 damage, 3 hearts and a doubling function then we end with 14/0/6. so we need to clamp the inbound value of the reduction function to the actual hearts. but we don't clamp the modifiers or their impact on the new damage. then hearts themselves are reduced up to the modifier amount (or stop at zero if higher than the hearts)

quick kernel
#

nah you're misinterpreting it

#

logically, doubling the reduction of absorption doubles the effectiveness of the hearts you have

#

but functionally, it just adds additional pseudo-hearts

#

thinking about it in terms of effectiveness doesn't work - as you've just realised

#

you have to think about it in terms of adding or removing hearts

sand copper
#

okay, so we store the amount of hearts expected to be consumed in f1 before we apply reductions. if I then pass in a reduction function of (c, r) -> 0 so that no matter the absorption, it will never reduce the damage. do we still consume hearts?

quick kernel
#

no in either ideology

sand copper
#

then we have a problem.

quick kernel
#

ironically, if you're concerned about the 0/0/4 scenario, then you really should be opting for ideology 2

sand copper
#

how do we store the pre-modified absorption and then callback to that value after modifications to decide if we are reducing hearts or not?

quick kernel
#

you don't need to store the pre-modified absorption at all

#

here, throw me the post-PR actuallyHurt method

sand copper
quick kernel
#

I'll show you

sand copper
# 
protected void actuallyHurt(DamageSource p_21240_, float p_21241_) {
        if (!this.isInvulnerableTo(p_21240_)) {
            this.damageContainers.peek().setReduction(net.neoforged.neoforge.common.damagesource.DamageContainer.Reduction.ARMOR, this.damageContainers.peek().getNewDamage() - this.getDamageAfterArmorAbsorb(p_21240_, this.damageContainers.peek().getNewDamage()));
            this.getDamageAfterMagicAbsorb(p_21240_, this.damageContainers.peek().getNewDamage());
            this.damageContainers.peek().setReduction(net.neoforged.neoforge.common.damagesource.DamageContainer.Reduction.ABSORPTION, Math.min(this.getAbsorptionAmount(), this.damageContainers.peek().getNewDamage()));
            this.setAbsorptionAmount(this.getAbsorptionAmount() - this.damageContainers.peek().getReduction(net.neoforged.neoforge.common.damagesource.DamageContainer.Reduction.ABSORPTION));
            float f1 = net.neoforged.neoforge.common.CommonHooks.onLivingDamagePre(this, this.damageContainers.peek());
            float f = this.damageContainers.peek().getReduction(net.neoforged.neoforge.common.damagesource.DamageContainer.Reduction.ABSORPTION);
            if (f > 0.0F && f < 3.4028235E37F && p_21240_.getEntity() instanceof ServerPlayer serverplayer) {
                serverplayer.awardStat(Stats.DAMAGE_DEALT_ABSORBED, Math.round(f * 10.0F));
            }

            if (f1 != 0.0F) {
                this.getCombatTracker().recordDamage(p_21240_, f1);
                this.setHealth(this.getHealth() - f1);
                this.gameEvent(GameEvent.ENTITY_DAMAGE);
                this.onDamageTaken(this.damageContainers.peek());
            }
            net.neoforged.neoforge.common.CommonHooks.onLivingDamagePost(this, this.damageContainers.peek());
        }
    }
#

you'll also need to factor in this behavior ```java
@ApiStatus.Internal
public void setReduction(Reduction reduction, float amount) {
float modifiedReduction = Mth.clamp(modifyReduction(reduction, amount), 0f, this.getNewDamage());
this.reductions.put(reduction, modifiedReduction);
this.newDamage -= modifiedReduction;
}

private float modifyReduction(Reduction type, float reduction) {
    for (var func : reductionFunctions.getOrDefault(type, List.of())) {
        reduction = func.modify(this, reduction);
    }
    return reduction;
}
quick kernel
#

how it was before your last push was fine and how it should be

sand copper
#

the clamps are part of the debate so change them as you see fit

#

both classes? or just the damage container changes?

quick kernel
#

just that one specific clamp in modifyReduction

#

needs to go, that's basically an essential

sand copper
#

yeah that one i agree needs to go based on both options

quick kernel
#

the one in setReduction I would argue needs to go for the same reason

#

if we allow stacking of modifiers, then any value is valid until used

#

although that is post-stack

#

yeah ok that one's fine

#

cus it's post-stack and leaving it unsanitised after that point poses a problem

#

uhhh

#

actually it reintroduces the original issue that pup had

#

lol

#

damage is fun

sand copper
#

oh right. the clamp needs to be on amount not the function

quick kernel
#

then the clamp is pointless

#

cus that value should be sanitised before coming in

sand copper
#

we can clamp it in either place, this was just less messy than doing it in the patch

quick kernel
#

true, but it hides sanitation

#

which makes for gross code legibility

sand copper
#

wait. we already have a clamp with the min function

quick kernel
#

yep

sand copper
#

that clamp is redundant

quick kernel
#

that's correct sanitation

#

also don't forget this setReduction thing applies to all modifiers

#

not just absorption lol

sand copper
#

foggy brain moment

quick kernel
sand copper
#

🤔 hmm. yeah i'm not sure which ideology is better. i get the implementations. reading the top comment though, i'm not sure which is better. In one method you can lose more hearts than you lose health. in the other you can lose more health than you lose hearts.

quick kernel
#

uh no that should never happen

#

did I miss something

#

#2 should be the same functionality as #1, with the only difference being the residual hearts is clamped to the damage it could've absorbed

sand copper
#

i was just going off your comments. let me check if that's possible based on the code

#

based on how your wrote #1 i could take 1 damage but still lose 4 hearts of abs

quick kernel
#

yeah, that's why I don't like ideology #1

#

because it's disconnected from reality

#

the only "benefit" it has realistically is that it's the most 'raw' interpretation of the event handling

#

which technically allows more flexibility for users, but I just see it harming people unintentionally

sand copper
#

oh yeah. two. i like two.

#

the reduction in the container would still be high, but the actual impact on absorption would be constrained. yeah that one makes sense

quick kernel
#

👍

#

then strip out the two clamps in set/modify reduction

#

and base your adjustment on my screenshot there

# 
// Ideology 2
    // Increasing the absorption heart reduction increases the amount of hearts consumed, but only up to the available damage
    protected void actuallyHurt(DamageSource p_21240_, float p_21241_) {
        if (!this.isInvulnerableTo(p_21240_)) {
            this.damageContainers.peek().setReduction(net.neoforged.neoforge.common.damagesource.DamageContainer.Reduction.ARMOR, this.damageContainers.peek().getNewDamage() - this.getDamageAfterArmorAbsorb(p_21240_, this.damageContainers.peek().getNewDamage()));
            this.getDamageAfterMagicAbsorb(p_21240_, this.damageContainers.peek().getNewDamage());
            // Cache pre-absorb damage here
            float damage = this.damageContainers.peek().getNewDamage();
            this.damageContainers.peek().setReduction(net.neoforged.neoforge.common.damagesource.DamageContainer.Reduction.ABSORPTION, Math.min(this.getAbsorptionAmount(), damage));
            // Cache absorb here, clamping to damage since you can't consume more absorb than damage dealt
            float absorbed = Math.min(damage, this.damageContainers.peek().getReduction(net.neoforged.neoforge.common.damagesource.DamageContainer.Reduction.ABSORPTION));
            // Clamp to not set negative absorb hearts here
            this.setAbsorptionAmount(Math.max(0, this.getAbsorptionAmount() - absorbed));
            float f1 = net.neoforged.neoforge.common.CommonHooks.onLivingDamagePre(this, this.damageContainers.peek());
            // Use ONLY absorbed value here, since you can't have absorbed more damage than existed
            float f = absorbed;
            if (f > 0.0F && f < 3.4028235E37F && p_21240_.getEntity() instanceof ServerPlayer serverplayer) {
                serverplayer.awardStat(Stats.DAMAGE_DEALT_ABSORBED, Math.round(f * 10.0F));
            }

            if (f1 != 0.0F) {
                this.getCombatTracker().recordDamage(p_21240_, f1);
                this.setHealth(this.getHealth() - f1);
                this.gameEvent(GameEvent.ENTITY_DAMAGE);
                this.onDamageTaken(this.damageContainers.peek());
            }
            net.neoforged.neoforge.common.CommonHooks.onLivingDamagePost(this, this.damageContainers.peek());
        }
    }
sand copper
#

alright. changes made. commits pushed

#

okay, i love how fast things process now.

quick kernel
#

reviewed and approved

#

although it still has my conversation as unresolved

#

and I.. do not know how to make that go away

#

I'll just delete the comment

#

there we go

#

@echo kiln all you

echo kiln
quick kernel
echo kiln
#

I will try to get a few minutes to take a look at it sometime in the next few days if no one beats me to it

neon anvil
#

before the PR gets merged, please investigate armor durability draining. It's draining way too fast in neo compared to vanilla and unbreaking isnt helping according to a player

#

May be related to the damage pipeline

quick kernel
#

@sand copper

neon anvil
sand copper
#

TIL you could commit directly to my PR

swift bluff
#

there's a checkbox for allowing maintainers to directly commit to your PR, which is ticked by default

sand copper
#

ah. i guess i misunderstood what that box meant. not a bad box to have for sure though

swift bluff
#

the reason it's there is because a maintainer pushing to your branch is literally someone pushing on your fork's branch -- it'll run any actions on your repo as configured, which includes secrets passed to the actions, so there's the potential of maintainers accessing your repo secrets

#

practically nobody runs Actions (and stores secrets) on their forks though

sand copper
#

GH has no chill with this description lol.

image.png
neon anvil
#

@echo kiln You alright if I just merge the PR or did you want to investigate it closer

quick kernel
#

I would appreciate one more set of eyes briefly skimming the logic of actuallyhurt post-pr before merging

echo kiln
neon anvil
quick kernel
#

👍

#

I mean I did review it

#

so it's not like it's not looked at

sand copper
#

woot!

neon anvil
#

I may had forgotten to test unbreaking

sand copper
#

I just updated the PR body to include the fixes for armor

sweet briar
#

Is it better to have the LivingDamageEvent.Pre fired before absorption calculation?

#

Player treats absorption as part of health, and having damage event fired after absorption calculation will make amplifications in the event bypass absorption, and make reductions in the event cause more consumptions of absorption than expected.

quick kernel
#

you sholdn't be using .Pre to modify absorption

#

that's what the absorption stage modifier in incomingdamageevent is for

#

you should almost never be using .pre to modify damage at all

sweet briar
#

It's a protective equipment

#

so It makes sense to modify it in .Pre instead of Incoming

#

Or do you think I should do it as part of reductions?

lucid harbor
#

If its something that is "like" absorption I would think you would apply it in .Pre, by reducing the damage from Pre and applying it against your thing

#

Since Pre is post-mitigation "about to be applied" damage

sweet briar
#

It's something reducing damage multiplicatively

#

for example, -10% damage

#

For example, player takes 10 damage, with this, they take 9 damage.

#

But if player has 10 absorption, currently it will consume all 10 absorption

lucid harbor
#

For that I would apply it in Incoming

#

since you would want it applied pre-mitigation

sweet briar
#

if it's part of incoming, it will be calculated before armor, enchantment, and magic

lucid harbor
#

That should be fine for your purposes

#

especially if its just a multiplicative reduction

sweet briar
#

then everyone just use event priorities again?

#

if that's the case, should setNewDamage in LivingDamageEvent.Pre be removed?

lucid harbor
#

No, because some may need to consume post-mitigation damage

#

i.e. if you were implementing an absorption-like effect

#

But for this feature I would expect you use a HIGH priority LivingIncomingDamageEvent listener

sweet briar
#

you mean low

lucid harbor
#

or at least a normal prio

sweet briar
#

because high is for weapons

lucid harbor
#

I don't think there was an established convention for the priorities

#

at least I don't see one in the javadocs

sweet briar
#

Weapons for sure should be calculated before armors

lucid harbor
#

Yeah, normal prio seems fine then

sweet briar
#

I thought separation of offensive modification and defensive modification is part of the purpose of this refactor

lucid harbor
#

That only really ended up happening for multiple damage reductions

#

I will be observing the current form over the lifecycle of 1.21 and see how it pans out

quick kernel
#

ehh?

#

if you want to modify outgoing damage, you use setamount in incoming

#

if you want to modify incoming damage, you either use setamount, or add a reduction modifier

#

reduction modifiers run at each stage, not "before" anything

#

including absorption

sweet briar
#

Currently damage modification priorities are:

  • Incoming
  • Armor
  • Magic
  • Absorption
  • Damage.Pre

My proposed changes:

  • Incoming
  • Armor
  • Magic
  • Damage.Pre
  • Absorption
quick kernel
#

why

#

absorption already has its own modifier

sweet briar
#

yes

lucid harbor
#

You can unwind absorption in .Pre if you want to apply it against something else iirc

sweet briar
#

I just want custom multiplicative modifications be applied before absorption, as it makes more sense to players

lucid harbor
#

If absorption was after the event you lose the ability to care about it at all

quick kernel
lucid harbor
#

Only armor is even sensitive to the actual incoming damage value like that (since it's nonlinear)

sweet briar
quick kernel
#

then use a modifier on absorption and set a listener priority?

lucid harbor
#

enchantments / potions are multiplicative so the application timing is irrelevant

sweet briar
#

I know

#

there are nonlinear modifiers as well

quick kernel
#

this is back to the whole 'we need a whole convoluted system for every applicaiton'

sweet briar
#

That was me

quick kernel
#

which we definitively voted down cus it got ridiculous trying to fit everyone's niche

#

I'm still not sure why you can't just do an absorption modifier here

#

that sounds like it'd work fine

sweet briar
#

I have several nonlinear defensive curios that I expect to be applied after magic and before absorption

lucid harbor
#

also I am not sure how this javadoc line got through screm <li>only the {@link LivingIncomingDamageEvent EntityPreDamageEvent}

quick kernel
sweet briar
#

modifier modifies reduction or damage?

#

I thought it modifies reduction

#

let me check again

lucid harbor
#

the reduction function modifies the effectiveness of that reduction

#

it takes a bit to wrap your head around

sweet briar
#

yeah so it modifies reduction

#

It's for things like "bypass 50% of protection"

lucid harbor
#

It is, however, difficult to link damage to absorption and remaining HP

sweet briar
#

My original proposal is to have a modifier for damage.

lucid harbor
#

since absorption isn't a typical damage reduction, but more like a "post-mitigation damage consumer"

sweet briar
#

yes, that's why I want Damage.Pre to be called before that

#

but the more important thing is that, should offensive and defensive effects be separated by events or by priorities?

lucid harbor
#

I don't think they should be separated by events necessarily

#

since that would cause us to just fire two events at once

#

Priorities are a fine system for that goal

sweet briar
#

but then the majority of modders would just use normal priority

#

for both offensive and defensive

lucid harbor
#

That is (imo) a documentation issue

quick kernel
#

the reduction is a modifier for the damage though

#

modifying the reduction modifies the damage

lucid harbor
#

it does but you wouldn't be able to do the intended effect here

sweet briar
#

it would be a mult_base if you do that though

lucid harbor
#

which is reduce the incoming damage by 10%, and then have absorption be computed appropriately

sweet briar
#

mult_base works for offensive modifiers, but it's terrible for defensive modifiers

#

mult_base < mult_total for offensive, but mult_base > mult_total for defensive

#

I can find ways around my problem, but I'm more concerned about how other mods would implement their modifiers

#

It would be chaotic if the system goes back to the 1.20 style where modifiers from all mods are mixed together

lucid harbor
#

Solidifying the conventions for priority usage early will be important

sweet briar
#

give a warning when someone use normal?

#

or crash in dev env when someone use normal

quick kernel
#

what

lucid harbor
#

That wouldn't be necessary

sweet briar
#

the worst part is when someone write offensive an defensive modifiers in the same event listener

#

that should be prevented

quick kernel
#

the problem here is you're trying to make a perfect world out of something that'll never be perfect

#

I can agree with wanting direct-modifiers after each stage or whatever, but the question then gets raised of when it becomes just too much to cater for in what's meant to be a generified event system

lucid harbor
#

The absorption thing should probably be addressed though

quick kernel
#

yeah I'm still not seeing the problem with absorption

lucid harbor
#

Mod X wants to apply a post-mitigation damage reduction modifier Y
This modifier should reduce the incoming damage (after armor / magic), and then the reduced damage should be applied to the player (which would first apply to absorption)

quick kernel
#

oh
treating absorption ideologically as a function of health rather than a modifier itself

lucid harbor
#

In the current system, the damage will have already been applied to absorption, meaning the absorption takes A (unreduced) damage, and the player takes (D - A) * Y

#

Yeah

#

in reality the function should be D = D * Y and the player's absorption should be hit with the post-reduction value

sweet briar
#

Yeah that would be good

quick kernel
#

what if we just add one more callback to damagecontainer

#

that's still stage-based

#

but instead of being a reduction, it gives you the newDamage post-reduction, and expects you to return a new modifieddamage

#

I'm not a fan of it basically just being a setAmount for every stage ideologically, but I think it's realistically the only way we'd be able to cater to the more niche audience without it being a battle every time

lucid harbor
#

Well, there is an alternative

#

which is basically to gather a stack of attribute-modifier like things for each stage and apply them

#

honestly I thought that was what we were going to end up with when this started, but we didn't arrive there

sweet briar
#

That's what I proposed as well

lucid harbor
#

Hence my comment about observing the current system over the lifespan of 1.21

quick kernel
#

that was discussed and denied

#

we had a full blown conversation about that that already ended

sweet briar
#

I know. Because I proposed too many operations

quick kernel
#

yes but that's the real problem lol

#

it's never enough

#

there's always "just add X"

#

because it's such an intricate system

lucid harbor
#

Well you don't actually need operators

#

you just need a stack of functions

#

right now we only have a stack of functions on the reductions

#

which is a bit weird

sweet briar
#

yeah

quick kernel
#

the reduction thing I did think was a bit odd personally

#

seemed like an arbitrary reduction of capability from just outright unaryoperator'ing the resultant damage

sweet briar
#

the reduction modifier is designed for weapons that can penetrate protections. Not for defensive gears to reduce damage

lucid harbor
#

Do open a github issue about the absorption thing though

#

It makes sense to move it, and if people would like to interact with it they have access to the entity and can do so

sweet briar
#

ok

quick kernel
#

I think it makes sense ideologically to consider absorption a function of health

#

rather than a modifying factor

#

so yeah

#

and I'd agree with a stack of operating BiFunctions on the damage for each stage
honestly I'm not entirely sure i see the point in the reductionfunctions if operating functions are gonna be present

#

I think the original intention was to prevent people just doing the functional equivalent of .setAmount on any stage

sweet briar
#

Also, regarding this line, should it be wrapped by if(!source.is(DamageTypeTags.NO_IMPACT))?

Screen_Shot_2024-07-09_at_10.14.51.png
quick kernel
sweet briar
#

hence NO_IMPACT tag has no effect on non-player

quick kernel
#

yep

lucid harbor
#

that is something that would have to be raised as a mojira bug first

quick kernel
#

^

sweet briar
#

anyway it might not be a neoforge thing to do

#

ok

lucid harbor
#

if a Mojira bug is opened, and marked as confirmed, we can wrap it accordingly

quick kernel
#

that being said

#

it looks intentional to me

#

just stupidly designed

#

cus they use no impact for the hurtmark

sweet briar
#

wait, they have NO_KNOCKBACK in 1.21

#

it's not there in 1.20

quick kernel
#

yep

#

because they overhauled explosions

sweet briar
#

ah

quick kernel
#

in 1.20.1 they basically only used it for explosion mitigation

#

then when breeze crap came in they had to redo how they handled explosions a bunch

sweet briar
#

makes sense

#

another mixin I can delete

quick kernel
#

I still have one damage mixin somewhere..

sweet briar
#

what about allowing crit on sweep

quick kernel
#

on wait no I commented it out

sweet briar
#

I have a dangerous local ref mixin on that

quick kernel
#

yeah this'd be the same thing you had lol

sweet briar
#

Currently crit will disable sweep. It's probably an unintended consequence. Should it be changed?

quick kernel
#

is that a neoforge issue? or vanilla?

proper shadow
#

im confident that sweep and crits are intentionally disjoint, you can only sweep while grounded in vanilla

#

(personally though, I do loathe having to choose between aoe or what I have normalized as my "normal damage" via jump critting lol)

quick kernel
#

Makes sense to me tbh

#

Its a decent way of varying the limited combat options

proper shadow
#

objectively its not intrinsically poor, I just hate it myself

sullen nebula
#

if you never sweep why not simply be an axe main? 😛

proper shadow
#

bhop atkspd

#

you need actually 1.67, but 1.6 is close enough that you won't feel it too badly with low latency (or singleplayer)

sand copper
# quick kernel I think it makes sense ideologically to consider absorption a function of health

I think another detail that adds to this, is unlike other reductions, nothing is consumed in the process of applying those reductions. Absorption is though, which makes it resource-like, like health. And before someone says armor durability, it's not the same. a reduction in health/absorption is a complete reduction in the maximum that can be reduced/taken, whereas the armor is always equally effevtive with durability only mattering when the item breaks as to whether there is or is not a reduction.

sweet briar
#

maybe modify crit event a little bit to allow mods to determine whether to disable sweep

quick kernel
#

that'll be a separate PR

sweet briar
#

yes

maiden dove
#

@quick kernel Sorry to bother, but is there a document or something that summarizes the damage events? I'd like to update the /docs damageevents command to explain the new system.

quick kernel
#

sent you a ping

eternal krakenBOT
#
The Damage Event Sequence

Made by @quick kernel
The series of events for an attack goes in this order:

  1. LivingAttackEvent - Use this if you want to catch any time an entity hits another, regardless of damage or whether it will be successful or not
  2. LivingHurtEvent - Use this if you want to handle an attack that is likely to hit, but before any potions, armour, or enchantments take effect
  3. LivingDamageEvent - Use this if you want to take action as an entity is definitely being damaged, with the final damage amount being available.
  4. LivingDeathEvent - Use this event if you want to handle an entity's death for any reason, with the exception of drops.
  5. Global Loot Modifiers - Use these if you want to handle any drops from a dying entity.
quick kernel
#

they haven't done it yet

#

that's the old one (which I also made lol)

sand copper
#

i was curious. i didn't know it existed until now

sand copper
#

soo the PR was closed?

neon anvil
#

spino's yeah they closed it. @sand copper did you want to make a PR to do the change you wanted? or should I give it a stab? (After I read up the previous convos about the change)

quick kernel
#

which change?

neon anvil
#

I am not familiar with the damage pipeline so I would have to catch up to speed

quick kernel
#

ah

#

yeah these PRs really need to be discussed and reviewed by the active people here first

#

that PR would've caused issues and I didn't even know it existed lol

neon anvil
#

so if I am reading this right and convo before, the change is to take this

image.png
#

and make it this

image.png
#

feed the pre damage to the absorption code. Get the absorbed damage amount. And pass the diff between pre damage and absorbed damage to get the remaining damage to continue on

neon anvil
#

gametests screm

#

Wait, is this livingDamageSequenceEvents not consistent every run???

#

wtf

lucid harbor
#

if that test is doing an == on a float it's mcfucked™️

neon anvil
#

formatter.format(player.getHealth()).equals("16.333334")

#

that's how the test checks

lucid harbor
#

what

neon anvil
#

but the test seems to be runnign every server tick for GameTestServer? it keeps firing when breakpoint at the assertions

#

livingDamageSequenceEvents in LivingEntityEventTests

lucid harbor
#

which assertions

#

the ones inside the thenWaitUntil block on 362-369?

#

this is a pretty terrible check though ngl

#

because it does the format and checks against a string literal, but the displayed "expected vs actual" do not match what the check is looking for

neon anvil
#

helper.assertTrue(formatter.format(player.getHealth()).equals("16.333334"), "player health expected 16.333334, actually " + player.getHealth());

#

That's the line I am failing on consistently in my pr

#

Wait, why are... the last few checks are using the wrong attachment data for the error reporting...

neon anvil
#

ok i think i fixed game test

sand copper
sand copper
#

can we get that pinned also?

neon anvil
neon anvil
#

is there any documentation anywhere that will need updating if we make this change?

sand copper
#

The java doc on Pre, and maybe the sequence in DamageContainer.

sand copper
sand copper
gusty moss
maiden dove
sand copper
eternal krakenBOT
maiden dove
#

Ah that

#

I... honestly don't know? You tell me:

#

/docs damageevents

#

Ugh, I despise commands, especially on mobile

#

I'll get back to you in a bit, when I'm back at my computer

gusty moss
#

damage events aren't promoted to slash

#

!damageevents

eternal krakenBOT
# gusty moss !damageevents

Script failed execution due to an exception: **SyntaxError: script.js:6:180 Expected comma but found private_ident
2. LivingIncomingDamageEvent - Use this event to modify the incoming damage, cancel the attack, or apply specific damage modifiers (E.G. armour, enchantment, mob effect) via #addModifier
^
script.js:18:11 Expected ; but found :
'title': {
^
script.js:19:13 Expected ; but found :
'value': The Damage Event Sequence,
^
script.js:20:5 Expected an operand but found ,
},
^
**

gusty moss
#

harold the command isn't even valid smh

eternal krakenBOT
#
Information about trick nr. 79

Script:

const description = 'The damage event sequence'
function execute()
{
  replyEmbed(
  {
    'description': `Unsure how to use NeoForge's damage API?

Here's how it works:
1. **EntityInvulnerabilityCheckEvent** - Called for all entities (even non-living). Use this to apply additional or revoke vanilla invulnerabilities. Do not use for chance-based or resource-consuming effects
2. **LivingIncomingDamageEvent** - Use this event to modify the incoming damage, cancel the attack, or apply specific damage modifiers (E.G. armour, enchantment, mob effect)  via `#addModifier`
3. **LivingShieldBlockEvent** - Use this event to change or add item-based blocking mechanics (such as custom shields). Non-item based blocking should be done in `LivingIncomingDamageEvent`
4. **LivingDamageEvent.Pre** - Use this event to apply final modifications to the final damage. ONLY USE THIS IF YOU CANNOT USE `LivingIncomingDamageEvent`
5. **LivingIncomingDamageEvent.Post** - Use this to react to an attack, regardless of whether it did actual damage or not
6. **LivingEntity#onDamageTaken** - Override this method instead of using `LivingDamageEvent.Post` if you have a custom entity that reacts to taking damage

Want more info? See the detailed writeup done for the PR: <https://gist.github.com/Tslat/0ef11e57036f7fe253268b5a64aa455e>
_Made by @quick kernel_`,
    'title': {
      'value': `The Damage Event Sequence`,
    },
    'color': 536870911,
  }
  );
}
Names

damageevents

Owner

@gusty moss (561254664750891009)

Stats

Uses: 6 total: 2 prefix, 4 slash

gusty moss
#

someone forgot to escape the `

maiden dove
#

ah, I see

eternal krakenBOT
#
The Damage Event Sequence

Unsure how to use NeoForge's damage API?

Here's how it works:

  1. EntityInvulnerabilityCheckEvent - Called for all entities (even non-living). Use this to apply additional or revoke vanilla invulnerabilities. Do not use for chance-based or resource-consuming effects
  2. LivingIncomingDamageEvent - Use this event to modify the incoming damage, cancel the attack, or apply specific damage modifiers (E.G. armour, enchantment, mob effect) via #addModifier
  3. LivingShieldBlockEvent - Use this event to change or add item-based blocking mechanics (such as custom shields). Non-item based blocking should be done in LivingIncomingDamageEvent
  4. LivingDamageEvent.Pre - Use this event to apply final modifications to the final damage. ONLY USE THIS IF YOU CANNOT USE LivingIncomingDamageEvent
  5. LivingIncomingDamageEvent.Post - Use this to react to an attack, regardless of whether it did actual damage or not
  6. LivingEntity#onDamageTaken - Override this method instead of using LivingDamageEvent.Post if you have a custom entity that reacts to taking damage

Want more info? See the detailed writeup done for the PR: https://gist.github.com/Tslat/0ef11e57036f7fe253268b5a64aa455e
Made by @quick kernel

maiden dove
#

@sand copper Would you say it needs updating, or is it fine as is?

quick kernel
#

it's still correct

neon anvil
quick kernel
#

not a concern

sweet briar
#

Question: should there be an event for changing damage source? If so, how should it be implemented? I think it has been discussed some while ago

lucid harbor
#

The proposal that I have in my head for this is implementing a mutable set of tags on DamageSource which can be adjusted in-flight

#

initially set to the tags from the damage type

quick kernel
#

it has definitely come up multiple times yeah

#

I kinda wanna keep it separate to this PR/topic though

#

better suited as its own self-contained brainstorm/topic

sand copper
#

yeah. that discussion is more closely aligned with the idea of multi-source damage which we talked about addressing later once these changes settled in

neon anvil
sand copper
#

done. still need an approval from someone with write access though

neon anvil
#

yeah ik. I just wanted your eyes on it as I had a review request from you lol

#

now to wait till other maintainers are online

sand copper
#

gotcha

lucid harbor
#

@sand copper going to dump some more thoughts in here because I realize that trying to manage the concept of "being killed in the middle of the stack" is an absolute nightmare.

#

Moving Post to the bottom where I have it fixes the issue I'm triggering, but does not prevent the exact same class of issue from being triggered in Pre. There's also ILivingEntityExtension#onDamageTaken which should probably go down to where Post now is (based on its own javadocs).

#

For managing this error state in Pre, I think we have a couple options...

  1. assert !this.dead after the event returns. Completely forbade killing the entity in Pre and require people defer it to Post, or let vanilla handle killing the entity when the stack returns.
#

This seems okay, because IMO something that kills the entity (i.e. dealing new damage by re-invoking hurtServer) should not happen in Pre and should be relegated upwards to the cancellable LivingIncomingDamageEvent or Post

#
  1. Instrument handling in the middle of hurtServer to handle the case where actuallyHurt completes and the entity comes out this.dead. Vanilla does not handle this case. We can make it happen but it requires some potentially iffy patches and careful logic. Not sure if there's a usecase that needs this compared to (1).
sand copper
#

I think this is where @quick kernel had notionally eluded to the expansion of the pipeline to include multi-damage events, but we kept that out of scope for the first pass. This whole scenario feels like we are teasing at the multi-damage-source framework again. Which feels like something where we either obliterate the Stack implementation in favor of an internalized handler of multiple damage sources, or we keep the Stack implementation and continue where you've started towards a safely-recursive pipeline.

lucid harbor
#

I think I would be in favor of (1) and simply requiring that people who want to deal multiple damages do it either "always before" or "always after" the real damage dealing phase (where Pre gets fired from)

#

There's some more nuance to getting things correct though, mostly little semantic things that I doubt anyone cares too greatly about

#

i.e. we currently don't propagate back the damage value to lastHurt and it always gets the non-Pre-modified damage since actuallyHurt doesn't communicate the damage upward

#

We also have getPostAttackInvulnerabilityTicks operate in an invalid state if we enter into the "dealing damage during iframe timer since the new attack was bigger"

#

For that I need to add this.damageContainers.peek().setPostAttackInvulnerabilityTicks(this.invulnerableTime); before actuallyHurt and then this.invulnerableTime = this.damageContainers.peek().getPostAttackInvulnerabilityTicks(); afterwards

lucid harbor
#

ohnoes wait a minute

# 
            this.damageContainers.push(new net.neoforged.neoforge.common.damagesource.DamageContainer(source, damage));
            if (net.neoforged.neoforge.common.CommonHooks.onEntityIncomingDamage(this, this.damageContainers.peek())) return false;
#

does this just leak the container forever?

sand copper
#

that's the line in my comment i mentioned in my first response

lucid harbor
#

Ah, yes it is

#

I started looking at this thing in depth and there are quite a few weirdnesses

#

Some I'm inclined to just throw assertions at for now

#

i.e. I currently have updated

    public static boolean onEntityIncomingDamage(LivingEntity entity, DamageContainer container) {
        Preconditions.checkArgument(!entity.isDeadOrDying(), "The LivingIncomingDamageEvent cannot be fired with a dead entity.");
        var event = NeoForge.EVENT_BUS.post(new LivingIncomingDamageEvent(entity, container));        
        return event.isCanceled() || entity.isDeadOrDying();
    }
lucid harbor