#Damage Events

1 messages · Page 3 of 1

quick kernel
#

then the first one is arguably less functional

#

so you'd be asking to get rid of LivingIncomingDamageEvent

#

and keep the shield event

#

which is then gonna confuse the shit out of people

#

unless you then rename the shield event, which will then again confuse people looking to do shield stuff

proper shadow
#

Is this a reason or a useful side effect? Ironically, inner classes would also make it convenient for searching

quick kernel
#

yeah so here's the problem

#

and the reason I wanted the names changed

#

the average user, who uses these events

#

do not search or look through event heirarchy

#

those are your common idiot (AkA newbie dev)

#

then you also have the people who may or may not have experience and are just looking for an event to use

#

these events are some of the most widely used events in the entire API

#

and requiring users to 'just know' you're meant to look at the heirarchy and whatever to work out which one to use

#

will not happen

#

full stop

#

it just won't

#

the event name itself has to be its own descriptor

#

and we know thta to be true because we've seen this circumstance happen so goddamn often already

proper shadow
#

So you are basing the PR off of the idea that we should not inconvenience low information users (who could gain information by reading the event file)
If it is so prevalent, I suppose its worth shelving that whole issue just to assuage the collective annoyance brought out by that

quick kernel
#

low information users & those who are just looking for something to use

#

this may come as a shock to you

#

but not everyone looks at the entire event heirarchy every time they want ot use an event

proper shadow
#

I don't appreciate the tone

quick kernel
#

neither do i

#

curb your sarcasm and I'll curb mine

proper shadow
#

this is me looking at the issue from what I am assuming is your point of view

quick kernel
#

I am frustrated by the seeming complete lack of awareness of the already existing issue that we're trying to resolve

proper shadow
#

I assumed the continued annoyance and made the conclusion that I would, in that mindspace, agree with you

sand copper
quick kernel
#

I mean the perceived higher bar to entry is one of the most common arguments people use to push people to fabric

proper shadow
#

I think you are misinterpreting my words, but I have also seen you use this tone for the preceding conversation

quick kernel
#

it's very clearly beneficial

proper shadow
sand copper
#

how about this. what if we create clear definitions about what each event's purpose is then if we agree upon that definition we can use that as a basis for a name?

quick kernel
#

that's the problem

#

shadow's argument seems less oriented about what the event is used for

#

and more about how the API reflects its position in the chain

#

I fundamentally disagree with that approach

sand copper
#

then we hybridize those perspectives in our definition. we may end up with a chimeric name, but it will ideally meet all needs

quick kernel
#

I don't believe it will, but I'm open to the attempt

#

I can edit this if you think I need to

#

I think at the very least changing to this would be a good improvement over the current names
#1198800008225501285 message

proper shadow
quick kernel
#

this is about the entire thing

#

the shield event is just one small almost entirely unrelated tangent shadows went on

proper shadow
#

I think specifically shadow is right about the shield event

quick kernel
#

shadow has a point about it

#

but it opens up other issues if you go with his solution

#

as I explained above

#

the shield event currently does more than the incoming event

#

so you'd have to get rid of the incoming event

#

if you do that, then people who just want to cancel the event are going to be confused trying to figure out wtf they're meant to do

#

even in searching cus it'll just look like they're using a shield or something

#

if you then change the name to account for that, then people looking to do stuff with shields will be lost

sand copper
#

Event 1 (LivingEntity#Hurt after the invulnerability checks)

The point at which a vulnerable living entity is provided a damage type and value from the attacker's successful attack wherein listeners can modify this damage value.

quick kernel
#

I don't disagree that they have similar usages, but I don't think similar means the same, and having an event specifically used for recording incoming attacks or outright cancelling them (as opposed to blocking them, which is functionally distinct)

#

is worthwhile

proper shadow
#

Why would the incoming event need to be removed if we agree to limit the scope that the vanilla shield event is used for? Is this something I have to review the code to understand? OR is this an implication that follows from how Shadow talked about the issue?

quick kernel
#

wait why are we arbitrarily limiting an event?

#

that seems incredibly counterproductive

proper shadow
#

arbitrarily? I am reducing it from general damage blocking to purely just modification of vanilla's built in shield blocking.

quick kernel
#

that's already what it's for

#

it just also allows you to dynamically modify the amount of damage a shield blocks

#

which is a heavily requested feature

#

given that vanilla shields are incredibly limited in scope

#

that's important because the amount of damage a shield blocks also usually correlates with the damage a shield takes (durability)

#

so you can't just push damage reduction to a later event

#

and we shouldn't be encouraging people to modify damage in a prior event

proper shadow
#

Specifically, I interpreted the following as this being considered part of the shield blocking event's scope and thought that it was tangential to it.

quick kernel
#

it's a side effect of it

#

the main goal is to allow for non-vanilla shields that can do different things

#

but as a side effect it also allows for people to do non-"shield" stuff too

#

which you can't really limit without defeating the benefits of the event in the first place

#

and I don't see why we should tbh

sand copper
#

having the shield and predamage as separate events creates a separation of purpose. predamage is for cancelling, and changing the initial value. block event assumes that first modification is handled and that the damage coming in is intended to be used as is. You can imagine if there were 5 listeners to predamage that were a mix of damage amplifiers and shield-based damage nullifiers, you'd have conflicts of priority. but if the shields wait for the shield event, then it's clear that your damage modification is contextually in the correct place.

#

i used shield for Block because i had mana shield as my mental example

quick kernel
#

I don't even think of mana shields tbh as a primary

#

like right now vanilla shields are broken OP
I know modders that want to be able to do modded shields that aren't just 'they hit back' or whatever

#

so being able to modify the actual blocking values and stuff is very valuable

sand copper
#

they don't have to be. i was just thinking of something that the old event wouldn't capture because of the vanilla isBlocking checks

proper shadow
#

I don't disagree with that. Hypothetically, if something was to be shield-like, it should borrow vanilla shield blocking and use the shield event to extend the functionality

quick kernel
#

that's.. what this PR does

#

that's the whole point of that event

#

lol

#

that's quite literally what the change to the event was all about
but the other event is still there to just be a 'I need to cancel this attack before it happens'

proper shadow
#

I believe that the way I communicate is definitely confusing the topic here, sorry

quick kernel
#

or if for some very specific reason you needed to modify the initial damage input, but is discouraged as a general thing

proper shadow
#

I was following up with my own realization of why you held your current stance on the event

quick kernel
#

language is weird

sand copper
#

tan estrano

quick kernel
#

anyway shadows' thing was about changing the name(s)

#

but his focus is more on the events as a collective chain

#

which I can understand but fundamentally disagree with

#

because users don't care if it's in a chain

#

he also had that thing on the PR about carrying invulnerability or something into the invuln event

#

which I'm not entirely sure I understand

proper shadow
#

The sequence is not the most important aspect of the events, I can agree on that part

sand copper
proper shadow
#

What are the important things about Pre that we may infer from the sequence, but are not readily apparent to lower information api users?

#

that could help with naming it

sand copper
#

i'm for Tslat's suggestion

EntityInvulnerabilityCheckEvent
LivingIncomingDamageEvent
LivingBlockDamageEvent
LivingModifyDamageEvent
LivingDamageTakenEvent

quick kernel
#

that's.. as broad as you can get

#

it is also "pre" damage, but that infers a correlative ordering, which is what caused this in the first place

#

but you can't really narrow it down to more than that, cus then it limits its potential scope

lucid harbor
#

I have returned; let me go read all of this...

quick kernel
sand copper
#

what if we used the above names. but still nested them in the parent class as sub-classes so that the API has a sense of sequence, but the names themselves are user-friendly?

quick kernel
#

I'm not really bothered how the events are laid out internally tbh, my main focus is on fixing the problem they already have

#

which can only be fixed with the names themselves

#

so if that's an acceptable solution then I'm fine with that

sand copper
#

😅 anyone know how to sub-class refactor in intellij?

quick kernel
#

just wait

#

better to decide on a solution before implementing every possible one lol

sand copper
#

oh for sure. i just remember not finding the feature when i unsubclassed the internal damage container and know these ahve a lot of references i'd like not to update manually

quick kernel
#

I think a rename might be the best approach

proper shadow
#

I think my problem with IncomingDamage is that its meaning relies on knowing the name of the other events, especially DamageTaken
DamagePotential? ugh idk

quick kernel
#

an outright removal feels like an unnecessary loss of valuable information when it'd only be affected by some edge cases

sand copper
quick kernel
#

and a mod could override and account for that if they so wanted

quick kernel
quick kernel
#

actually

#

no

#

I still don't agree

proper shadow
#

its not clear that IncomingDamage can be damage that never happens unless you also know that DamageTaken exists

quick kernel
#

because even if an overriding caller is handling this

#

it'll only get to this call if the parent call fails

#

so the vanillainvuln value will still be correct and relevant

#

it'll just not be overriding, but that can only be fixed by handling all call sites

#

which is not a great idea obviously lol

sand copper
#

and only happens in 3 places, which aren't wholesale upsets of the design

lucid harbor
#

Tslat: so you'd be asking to get rid of LivingIncomingDamageEvent
[1:34 PM]Tslat: and keep the shield event
I would say keep both and restrict the shield event to just shields. Currently blocking something with the "block" event will transiently ripple into calling shield-specific code paths, even if a shield is not present.
You could rectify this, but then you need a complex patch and extra flags to say if the damage was blocked by a shield or not.

Tslat: the event name itself has to be its own descriptor
This isn't necessarily correct, but the names have to not be ambiguous at face value (which the current pre-PR names are). The ambiguity makes it harder to recall what the purpose of the event is between the similar names.

there's an arguable benefit to the project to lower arbitrary bars
Agreed

how about this. what if we create clear definitions about what each event's purpose is then if we agree upon that definition we can use that as a basis for a name?
This is a reasonable approach to determining the name surface, though I think it was tabled earlier.

having the shield and predamage as separate events creates a separation of purpose. predamage is for cancelling, and changing the initial value. block event assumes that first modification is handled and that the damage coming in is intended to be used as is.
It's difficult to define that an event firing right after another event really separates anything. Though refer to the earlier blob about side effects of expanding the shield event.

quick kernel
#

I would say keep both and restrict the shield event to just shields. Currently blocking something with the "block" event will transiently ripple into calling shield-specific code paths, even if a shield is not present.
You could rectify this, but then you need a complex patch and extra flags to say if the damage was blocked by a shield or not.
I too love arbitrarily reducing functionality because you don't like the name of a class

lucid harbor
#

The shield thing isn't about the name

quick kernel
#

add a boolean to the event to skip the blockwithshield call if you want

#

I do'nt see why we should fuck over everyone who wants to use this event properly because we didn't want to add a boolean

lucid harbor
#

They can just use the earlier event

#

it fires one line above it

quick kernel
#

why

#

like why arbitrarily force that

#

whats' the benefit

lucid harbor
#

Because I don't have to maintain a bunch of patches to the vanilla shield logic t.t

quick kernel
#

it's all in one place

#

except for the one external patch to wrap the vanilla handling

#

at which point your argument is we should just not have the event

#

because it's work

proper shadow
# proper shadow its not clear that IncomingDamage can be damage that never happens unless you al...

For example, if the player enters a rage mode after taking damage, and I search for an event to do this and find and use IncomingDamage, then I can enter a rage mode even after I block damage by some means. I would have to see all the damage events side by side to understand the extra meaning attached to incoming.
I think calling it DamagePotential (or similar) would clue users into the idea that the damage from this event may not even happen.

quick kernel
#

to denote that it was before anything happens

#

but that caused a problem for shadows

lucid harbor
#

Because we conventionally use Pre only to describe a linked series of events

quick kernel
#

I understand

#

I'm just explaining

#

I'd rather get rid of pre than change damagetaken to post

proper shadow
lucid harbor
#

This is what I would setup for the first two

  1. LivingAttackedEvent as the first one. Fires where pre does now with all the control pre has now. Name conveys a target has been attacked by a damage source.
  2. ShieldBlockEvent (revert the changes and reduce the scope back down). Other non-shield damage reductions can happen in the former event.
quick kernel
#

god no

#

attacked is past-tense

#

that'll screw so many people

lucid harbor
#

They've been attacked; the reason people got confused with LivingAttackEvent was that it sounds outgoing (i.e. when a mob attacks someone else on the attacker side)

#

but its on the receiver side

quick kernel
#

might wanna put a marker in Neo that we're not allowed to work on shields again in the future then

#

if we can't have it now, mightaswell blackmark the whole section for future work

proper shadow
#

Shadow made a good point about how the recipient of the damage might be confusing from just reading the event name

#

Incoming solved that in a way that my just-proposed alternative didn't

#

IncomingPotentialDamage 😭

lucid harbor
#

Incoming is also fine as a name for the first thing

quick kernel
#

wait so what's the problem with the current proposed name then? LivingIncomingDamageEvent

lucid harbor
#

Its fine, it's just not what i would pick given the context

#

For the final two events

#

which seem more contentious

quick kernel
#

so either LivingIncomingDamageEvent or LivingIncomingAttackEvent

lucid harbor
#

^

quick kernel
#

the reason I didn't want Attack anymore

#

is because damage doesn't have to do with attacking anything

#

they could've just walked into a cactus

#

Damage is more accurate

proper shadow
lucid harbor
# lucid harbor For the *final two events*

we would want to go with LivingDamageEvent.Pre/Post. But to do in a "sensible" fashion requires removing Damage from the other ones for sanity (hence a push for Attack)

Alternatively it can just be LivingHurtEvent.Pre/Post, which is fine, given the call site is atomic in actuallyHurt

quick kernel
#

no thanks

lucid harbor
#

I realize tslat hates the word post

quick kernel
#

I hate Pre/Post

lucid harbor
#

But that is the pattern we use for this project

quick kernel
#

I'll literally harbour anger at you over it if it goes through

#

it is an absolute decimation of the first point of this PR

lucid harbor
#

Consistency across the project is more important than any individual's feelings towards the paradigm

quick kernel
#

the consistency implied here is entirely arbitrary

sand copper
# lucid harbor > Tslat: so you'd be asking to get rid of LivingIncomingDamageEvent > [1:34 PM]T...

I would say keep both and restrict the shield event to just shields. Currently blocking something with the "block" event will transiently ripple into calling shield-specific code paths, even if a shield is not present.
You could rectify this, but then you need a complex patch and extra flags to say if the damage was blocked by a shield or not.
Nothing in vanilla logic necessitates the item in hand be a shield. it actually exits the whole logic if the item doesn't have a shield block action type.

having the block event as in the PR now with the expanded scope adds a place for extended functionality (and in a place suited for it). LivingDamageBlockingEvent? so that it's more active about being an active action?

quick kernel
#

strip out the superclass and it's not even relevant anymore

#

any individual's feelings towards the paradigm

#

mate I have bot commands in both major community modding discords

#

because of how goddamn often this shit caused problems

#

it's not a "me" thing

#

it's a "people already get this wrong and I want to fix this" thing

lucid harbor
#

That's unrelated to naming new things pre or post

proper shadow
quick kernel
#

incorrect

#

it is wholly related to the naming of the event

sand copper
#

Shadow what do you think about this suggestion? #1198800008225501285 message

lucid harbor
#

The final two events are fired in tandem in actuallyHurt in accordance with the precident for Pre/Post events. One always fires before another, and only the former is mutable

maiden dove
quick kernel
#

they are

sand copper
#

i'm for moving DamageTakenEvent to inside the condition. that makes sense.

lucid harbor
quick kernel
#

shadows is trying to be reductive about my opinion

#

implying it's a "me" thing

#

it is not

sand copper
#

firing that event when damage is zero doesn't make sense.

lucid harbor
#

Tslat has argued the processes are not linked, though. I believe he's right on that action for the former and the latter pairs

#

Which is why pre as the first and post as the fourth don't work

maiden dove
lucid harbor
#

The first two are even only tangentially related due to how you want to separate the purpose there

maiden dove
#

the way you are reacting to it is not contributing anything

lucid harbor
#

The latter two are explicitly related

quick kernel
#

my response was entirely valid and relevant

maiden dove
lucid harbor
#

#3 fires with mutable context of the post-reduction damage before application, and #4 fires after application of damage

sand copper
#

what if we IncomingDamageEvent and DamageTakenEvent were changed to LivingDamageTakenEvent.Pre/Post ? this conveys one is before the damage is taken and one is after in a way that is consistent with the rest of the API

maiden dove
#

we are all working on the same project, and we're all doing this in our free time

lucid harbor
#

Its a tad verbose, but it works well enough

sand copper
#

this is Java lol. verbosity is king

lucid harbor
#

I say that because I think you can get the info without Taken, but it doesn't really matter

quick kernel
maiden dove
#

I'd like you to remember that, because I feel like you're not really considering Shadows' points here

quick kernel
#

I'm sorry if I'm mildly invested

maiden dove
#

You are not wrong, but Shadows isn't either.

quick kernel
#

I'm considering shadow's points just fine

sand copper
quick kernel
#

I'm not saying shadows is wrong
I'm saying he's fundamentally misunderstanding the problem we currently have and what this PR aims to resolve (among other things)

quick kernel
lucid harbor
maiden dove
#

I understand that

#

but you're overly lashing out

quick kernel
#

it implies that the damage is already taken one way or another

#

and that it's not mutable

lucid harbor
#

"Pre" events are generally accepted to have mutable control over the action that is about to occur

maiden dove
#

Shadows isn't trying to sabotage this, he's just voicing his concerns

quick kernel
#

sigh

maiden dove
#

I'd like you to recognize that, thank you

sand copper
quick kernel
#

I recognize that, thanks

#

I'm allowed to disagree with him

lucid harbor
neon anvil
lucid harbor
#

Same for #2 though the names are functionally controlling. I was going to do a brief assessment over the side effects of considering a blocking action is a shield

sand copper
quick kernel
#

if the event is unchanged

#

then it has to be the second of the two suggestions

sand copper
#

okay. i have to pack up to get off the train. please don't write novels in my absense lol

quick kernel
#

no deal, I'm already in talks with my publisher

maiden dove
lucid harbor
#

So... shields:

We have this hunk

if (!net.neoforged.neoforge.common.CommonHooks.onEntityPreDamage(this, this.damageContainer)) return false;
// ... local variable decls
net.neoforged.neoforge.event.entity.living.DamageBlockEvent ev;
if (p_21017_ > 0.0F && (ev = net.neoforged.neoforge.common.CommonHooks.onDamageBlock(this, this.damageContainer, this.isDamageSourceBlocked(p_21016_))).getBlocked()) {
    this.damageContainer.setBlockedDamage(ev);
    if(ev.shieldDamage() > 0) {
        this.hurtCurrentlyUsedShield(ev.shieldDamage());
    }
    f1 = ev.getBlockedDamage();
    p_21017_ = ev.getDamageContainer().getNewDamage();
    if (!p_21016_.is(DamageTypeTags.IS_PROJECTILE) && p_21016_.getDirectEntity() instanceof LivingEntity livingentity) {
        this.blockUsingShield(livingentity);
    }

    flag = p_21017_ <= 0;
}
quick kernel
#

bot comments?

#

where'd I do that

lucid harbor
#

I think ihh misread one of your statements

maiden dove
#

nvm me

quick kernel
#

both*

#

actually

#

it is bot

#

lol

#

literally meaning bot

#

gnome/k9/etc

maiden dove
#

But yeah. End of the day, I'd ask you to embrace the fact that Shadows is trying to improve this API just as much as you are, even if it might not be that obvious to you. Thank you.

lucid harbor
quick kernel
#

and again, I'm allowed to disagree lol

quick kernel
#

you can modify it if needed but it's meant to be discouraged

#

in favour of using the modifydamage one

lucid harbor
#

If you want to consume pre-mitigation damage you would likely use #1

quick kernel
#

yeah

lucid harbor
#

but with a lower priority

#

The existance of priorities imo makes expansion of the shield event not super necessary

#

We may want a hook that permits controlling the "shield area" for shield items

quick kernel
#

I disagree

#

having this here allows for far more control over shield usage

#

both vanilla and modded

#

making it reductive means you have to reimplement the whole algorithm yourself then potentially cancel/handle the other event too, etc

lucid harbor
#

What whole algorithm? Calling isDamageSourceBlocked?

quick kernel
#

yes

lucid harbor
#

it's a public method, you could call it from a handler for event #1

quick kernel
#

right but if I want to change how that works for a custom shield

#

I have to reimplement the whole thing just to change it slightly

#

as opposed to it just being present

lucid harbor
#

That's something that should be in a hook for a shield item (maybe on IItemExtension)

quick kernel
#

right

#

except you might have a non-item based shield

#

such as a mana barrier or something

proper shadow
#

I don't have codebase available atm, do shield blocks trigger iframes?

lucid harbor
#

Then we might want a better check in LivingEntity#isBlocking

#

Actually is that even called anywhere else?

quick kernel
lucid harbor
#

nope... just for some player head rotation code LUL

proper shadow
#

ah, so my following thought could be relevant

quick kernel
#

which tends to be the case

#

such as players having a mana barrier...

proper shadow
lucid harbor
#

A mana barrier being a global (i.e. from all directions) "shield-like" blocking option?

quick kernel
#

that's the point lol

#

if you want to block some of the damage, but not cancel it

#

then you use damageblocked

#

if you want to modify it for other purposes, there's the other event

proper shadow
#

I want to cancel all side effects of the damage aside from the ones that come from shield blocking

quick kernel
#

I.E. some form of custom "blocking" functionality that doesn't have to be an item

lucid harbor
#

In this case I would still think you would want it to be named Shield... to convey that the "blocking" action triggers shield side effects.

I think general pre-mitigation reduction should still be in that first event, and shield-esque effects in #2

quick kernel
#

which is why I proposed having that extra boolean

#

to skip out on the blockwithshield call

#

if the shield isn't item based or for any other reason

#

instead of hampering the entire event, allowing more configurability through a basic boolean seems ideal

lucid harbor
#

i actually need to delve into what that does

quick kernel
#

it just does knockback

#

and does shield disabling for players

#

axe hit, etc.

#

I think vanilla has some overrides for like ravager doing stuns and stuff

#

but imo that's all stuff that should be configurable as part of the event

#

so if anything that just pushes me further into wanting a boolean

lucid harbor
#

Well, right, if you have a non-shield-like block, just do damage reduction in #1

quick kernel
#

maybe "isPhysicalBlock" or something?

#

it's a separation of responsibilities lol

#

in general, modification of damage in #1 should be avoided and discouraged

#

otherwise it muddies things

lucid harbor
#

Not really

#

its the same as modifying it in #2 :p

quick kernel
#

functionally yes, but ideologically no

lucid harbor
#

without shield side effects

proper shadow
# quick kernel maybe "isPhysicalBlock" or something?

not sure.. my imagined use case was dodging projectiles (with a cooldown) but also having iframes for erring on the player's side on the actual gameplay value of the dodge (so a burst of arrows doesn't just strip you of the cooldown and also hurt you)

quick kernel
#

eh you can do that either way

#

cancel in #1 then set iframes manually

#

part of the problem is that not cancelling the attack also does the hurtTime

#

so you get that red overlay and stuff

proper shadow
#

I agree
I guess more specifically, is this kind of thing common enough to address in any direct way with some helper?

quick kernel
#

I mean that's kinda what #1 was intended for

#

dynamic attack cancellation

#

dodging, etc.

lucid harbor
#

The separation of uses (early cancel vs pre-mitigation reduction) for #1 is the job of event priorities

quick kernel
#

right but what if your barrier only works after shield block

proper shadow
#

oh specifically specifically cancelling damage and all side effects aside from iframes
but now I have no idea why I was asking that lol

quick kernel
#

like it's a skin-layer thing, or you just want the physical shield to take priority

lucid harbor
#

Well then you kinda want it to be after #2 entirely

#

to account for other mod's shields

quick kernel
#

right except your lastHurtamount gets screwed

#

as does your helmet damage

#

and your hurt marking, etc.

#

all the crap that happens after shield but not inside actuallyhurt

proper shadow
#

the other shields was partly why I didn't want to do #1, but past #2 I think there are some side effects

quick kernel
#

weird design btw vanilla, wtf you doin

quick kernel
#

so that you can avoid both situations

#

weird

proper shadow
#

yeah! I mentioned this use case for that reason

proper shadow
quick kernel
#

part of the issue here is vanilla's bizarre decision to decouple hurt and actuallyhurt, but put half of hurt's stuff past actuallyhurt with no backreference to it lol

#

I perfectly understand and agree with your thoughts on it shadows, but I just feel like there's enough of a niche there to warrant keeping the event expanded as-is

#

custom shield/blocking handling is pretty common

#

and relegating it to before the shield block is handled is.. fkin weird lol

lucid harbor
#

In terms of "time" its at the same point

proper shadow
quick kernel
#

like if we're already handling shield blocking stuff

#

why not keep the shield blocking stuff as.. part of that

#

as opposed to have some shield stuff here, some there

#

that just feels wrong

lucid harbor
#

Its due to what vanilla has defined as a shield blocking action

If you want all that behavior, event #2 is fine

quick kernel
#

right but we're not vanilla, we expand on vanilla, including shields

#

and shield-like handling

#

maybe rephrase

lucid harbor
#

Trying to mangle the logic and the patches to support shield-like and non-shield damage reduction at once isn't worth the complexity when you could just reduce in #1

quick kernel
#

what's your actual issue with the shield thing at present

#

is it the extra wrapping outside of hurt?

lucid harbor
#

The issue at current is the suggestion that all pre-mitigation go through that event, when it triggers side effects specific to using a shield

quick kernel
#

...no

#

only shield-like mitigation

lucid harbor
#

Well then we could keep it with Shield in the name, no?

#

To say "this is shield and shield adjacent handling"

quick kernel
#

I mean I'd prefer not to, but I'm ok with it if that's your gripe

#

just to encourage people to stick to "shield-like" stuff I presume?

#

I'm ok with that

lucid harbor
#

Yeah

quick kernel
#

👍

lucid harbor
#

And then back to #1

#

IncomingDamage or IncomingAttack

quick kernel
#

damage is my 100% preference

#

purely because damage incoming doesn't mean attack

#

environmental, etc.

lucid harbor
#

Attack is... maybe more "unique" w.r.t. the final two but I guess you wouldn't be "attacked" by a cactus

quick kernel
#

exactly

#

you're not getting attacked when you're drowning

#

or suffocating

neon anvil
#

The cactus jumped me

quick kernel
#

I get what you mean about naming it attack

#

but yeah just feels too far removed from what's correct

lucid harbor
#

call it LivingHurtEvent, no confusion there omegaLUL

quick kernel
#

(╯°□°)╯︵ ┻━┻

#

I get ptsd from that name

#

so tired lol

lucid harbor
#

tbh without the history it would be correct

#

its called from hurt()

quick kernel
#

they haven't been hurt yet

#

so.. technically not lol

#

hurt is both present and past tense which is ick

lucid harbor
#

LivingMightBeHurtInTheFutureEvent™️

neon anvil
quick kernel
#

I approve

quick kernel
lucid harbor
#

But yeah the only value in "Attack" is clear disambiguation from the final two

neon anvil
#

DamagingStartup

lucid harbor
#

how valuable that is... idk

quick kernel
#

actively damaging when you consider damage isn't necessarily an attack lol

lucid harbor
#

and the word "Incoming" may solve the issue

quick kernel
#

what about
LivingDamageEvent.Modify
LivingDamageEvent.?

#

I couldn't think of the post equivalent lol

lucid harbor
#

I think we should just go with the pre/post there

quick kernel
#

-.-

lucid harbor
#

Its well defined what those terms mean

#

users will be familiar with them

#

"happens before the thing with mutability"
"happens after the thing without mutability"

quick kernel
#

maybe from an API perspective lol
I can tell you that modders in the public don't implicitly understand that

sand copper
#

LivingDamageSourceAndAmountReceivedAndAboutToBeProcessedEvent

quick kernel
#

if you don't have a specifically notable name

#

people will default back to IncomingDamageEvent

#

cus it feels more relevantly named

#

and we're back to where we were

lucid harbor
#

#1 LivingIncomingDamageEvent
#2 LivingDamageBlockEvent
#3 LivingDamageTakenEvent.Pre
#4 LivingDamageTakenEvent.Post

#

(could redo #1 as LivingDamageIncomingEvent if we're being pedantic)

quick kernel
#

I still don't like pre/post and never will lol

#

we're gonna have another LivingHurtEvent situation and I'm gonna be annoyed eternally

maiden dove
#

I honestly hate the Incoming word here because that might just as well be the case at any other section of the chain

quick kernel
#

yeah it's an odd one

#

got a better idea?

maiden dove
#

I'm not sure we will be able to avoid the confusion here, just because of how convoluted the entire system is to begin with

quick kernel
#

avoid? maybe

#

but we can definitely mitigate it with properly named 3/4

lucid harbor
#

LivingAttackedEvent returns™️

#

Maybe you can't be attacked by a cactus in a pure english grammatical sense but eh

quick kernel
#

what about drowning

#

it no make sense

lucid harbor
#

Attacked by the ocean™️

maiden dove
#

LivingHurtEvent then, due to the call chain starting at hurt

#

(yes I know this is bad)

quick kernel
lucid harbor
#

It would be better if actuallyHurt wasn't called that

proper shadow
lucid harbor
#

But it is mojank

maiden dove
#

I think LivingHurtEvent would make the most sense honestly, but I see why you wouldn't wanna do that due to, uh, historical reasons

quick kernel
#

I think historical reasons are important here tbh

#

people will just fix their import

#

and not change their historical usage

maiden dove
#

yeah

#

that's the problem

quick kernel
#

I'm ok with that being a major problem

#

lol

neon anvil
#

LivingPainEvent

quick kernel
#

I don't really agree with LivingHurtevent being correct either tbh

#

from an API perspective yes, but not from a usage perspective

#

which is the issue we currently have

maiden dove
#

ok so can someone give me a TL;DR on the current naming proposal

#

*proposal(s)

sand copper
#

1. LivingOuchyEvent 2. LivingHowBoutNoEvent 3. LivingOhYouMeantItEvent 4. LivingThisIsMyLifeNowEvent

#
  1. LivingIncomingDamageEvent
  2. ???
  3. LivingDamageTakenEvent.Pre
  4. LivingDamageTakenEvent.Post
lucid harbor
#

#1198800008225501285 message

quick kernel
#

Shadows:
#1 LivingIncomingDamageEvent
#2 LivingShieldBlockEvent
#3 LivingDamageTakenEvent.Pre
#4 LivingDamageTakenEvent.Post

My issues with this - it adds ambiguity to #3/#4 that will cause users to just default to #1, and we're back to LivingHurtEvent all over again

Mine:
#1 LivingIncomingDamageEvent
#2 LivingShieldBlockEvent
#3 LivingModifyDamageEvent
#4 LivingDamageTakenEvent

Shadows issues with this - it doesn't conform to NeoForge's current api standard of doing pre->post for sequential events

#

I'm happy with a middleground of
#3 LivingDamageEvent.Modify
#4 LivingDamageEvent.?

#

my main goal is to avoid LivingHurtEvent v2

#

which I strongly feel pre/post will result in

maiden dove
#

LivingDamageEvent.Subclass1 and .Subclass2 is bad for anything that isn't Pre/Post

quick kernel
#

I don't disagree

#

just trying to meet in the middle

maiden dove
#

however pre/post is bad here too because we have more than 2 events in this chain

#

so I think pre/post is used rather arbitrarily here

quick kernel
#

shadows' logic is that 1/2 are functionally separate from 3/4

#

I'm not entirely sure I agree

#

but that's the logic anyway

maiden dove
#

that being said, I dislike the name LivingModifyDamageEvent

lucid harbor
#

3/4 are called atomically in order from actuallyHurt

#

i.e. for 4 to ever occur 3 must occur

quick kernel
#

well that's not currently true tbf

maiden dove
#

this popped up more as a joke in my head, but LivingActuallyHurtEvent.Pre and Post?

quick kernel
#

but calt was looking t ochange that afaik

lucid harbor
#

It is called actuallyHurt after all...

quick kernel
#

but Pre/post is my main issue

#

it's not self-descriptive enough to drag users away from IncomingDamageEvent

maiden dove
#

I think doing LivingDamageEvent.Pre/Post is bad because the name LivingDamageEvent is too generic

lucid harbor
#

It was LivingDamageTakenEvent.pre/post

quick kernel
#

even worse imo lol

maiden dove
#

LivingDamageTakenEvent.Pre makes no sense

#

past tense and Pre in the same event name is confusing

quick kernel
#

it makes sense from an API perspective, but historically this has been about a usage problem, not an API problem

#

and it'll continue being a problem to perpetuity unless we fix it here

maiden dove
#

I was thinking LivingApplyDamageEvent.Pre/Post, but that doesn't really solve the generic name problem

maiden dove
quick kernel
#

my issue is

#

I'm not sure what middleground there is

#

if Pre/Post is theo nly option for child event classes

maiden dove
#

not just yours, don't worry

#

Pre/Post is really only an option because it is successfully (and a lot more intuitively) used by other events

sand copper
# maiden dove past tense and Pre in the same event name is confusing

I think the past tense helps to convey that damage is going to be taken. sure you can set the value to zero, but all other damage effects have happened. The pre lets you know that you are in an early stage of this process and therefore have some ability to change the later state with post being the end marker

quick kernel
#

the fact that we have to think so hard for an alternative name to something that currently describes its usage almost perfectly feels like it should be a hint lol

maiden dove
#

the damage event sequence is everything but intuitive, so pre/post isn't really descriptive unless the wrapping event name is

quick kernel
#

yep

proper shadow
#

damage confirmed

quick kernel
#

if it was just those events on their own

#

I'd be fine with it

#

but it's not

maiden dove
quick kernel
#

tbf that wasn't my original name lol

#

it got changedo nly today cus of shadow wanting to change Pre

maiden dove
#

because quite frankly, DamageModifyEvent could be just about anywhere in the event sequence for all I (an unknowing modder) know

quick kernel
#

yeah but the sequence doesn't matter for damage

#

it's about the usage

#

you know that you use that event for modifying damage

#

it's immediately apparent that's what it's for

sand copper
quick kernel
#

wait am confuse

#

we changed the event positioning because of the damagecontainer thing didn't we

maiden dove
sand copper
quick kernel
#

do we have a footprint of what actuallyhurt looks like now

#

I've lost track

sand copper
#

one sec

maiden dove
#

(except for DamageTakenEvent which is obviously called afterwards and has its separate set of valid usecases)

quick kernel
#

nah sec you might be onto something

#

I forgot we changed the event position

sand copper
#

with arrow to where i think we should move Post to

image.png
quick kernel
#

ok yeah that makes much less sense now

#

although I still think new users are gonna fall to that

#

which is ok

maiden dove
#

I don't really see the issue with having a bit of an entry barrier on this

lucid harbor
#

Post should be at the same point as onDamageTaken yeah

quick kernel
#

fuck it I'm fine with .Pre and .Post there then

lucid harbor
quick kernel
#

I totally forgot we changed the ordering

neon anvil
#

LivingDamageAboutToApply
LivingDamageApplied

lucid harbor
#

You're getting applied

quick kernel
#

uhhh

#

actually

#

if we change to .pre and .post

maiden dove
#

Applied Energistics reference? squirr /s

quick kernel
#

shouldn't .post be moved to under onDamageTaken

#

as opposed to before recordDamage

lucid harbor
#

Thats what I suggested

quick kernel
#

ah ok

#

I'm cool then

#

LivingIncomingDamageEvent
LivingShieldBlockEvent
LivingDamageEvent.Pre
LivingDamageEvent.Post

Is our current standing point?

#

sample size of 1

#

lol

lucid harbor
#

qq: did the old events even have good javadocs? The case of many "really old" events is they didn't have any at all

sand copper
#

I think that move might also make our patch blob smaller

neon anvil
#

Can the javadoc for all of them mention all the others

quick kernel
#

^ definitely should, and their intended usage on each

quick kernel
maiden dove
#

can somebody also please make a docs PR to this

sand copper
maiden dove
#

(or @quick kernel, update the gist and I'll slightly rewrite it and make it part of the docs)

neon anvil
maiden dove
#

I just want to have a reference point to work with

quick kernel
#

alright I'll update the gist and pingeth

#

win

#

think we're gold

lucid harbor
#

v good

maiden dove
#

very nice

sand copper
#

I actually disagree with all of this /jkjkjk

lucid harbor
#

Tbh I just read the javadoc on LivingHurt and man is that thing kinda useless

quick kernel
#

so @sand copper
Need to move damagetaken to below onDamageTaken call

  • change damagetaken to LivingDamageEvent.Post
  • change incomingdamage to LivingDamageEvent.Pre
  • Change PreDamageEvent to LivingIncomingDamageEvent
  • Change DamageBlockEvent to LivingShieldBlockEvent
#

anything I missed?

lucid harbor
#

Its overtechnical

maiden dove
#

LivingShieldBlockEvent is called DamageBlockEvent in the PR desc, is that on the PR desc or the actual code

quick kernel
#

updated

#

thanks

#

@lucid harbor

#

name change aside

#

should we add that boolean to skipping blockwithshield?

#

that still feels useful

#

heavy shield might not want to knock back when hit or be stunned, etc.

#

or might be a braced shield that can't be disabled by axe, etc.

lucid harbor
#

Is it the blocking player's knockback or the attacker?

maiden dove
#

yeah, what exactly is that for

lucid harbor
#

I feel like maybe that's a "don't do it yet and see if anyone needs it"

#

Since its non-breaking to add later

quick kernel
#

defender gets knocked back

#

players have their shield disabled

#

vanilla mobs do additional effects

lucid harbor
#

Yeah lets leave it for now

quick kernel
#

feels worthwhile adding a skip to me while we're here

#

given that it's one boolean

neon anvil
#

so if I want my honey crystal shield to take say 1/6th of its durability off when blocking fire damage, which event would I use? The block damage one?

quick kernel
#

shieldblock yep

maiden dove
#

under what condition would that boolean be true

quick kernel
#

under whatever condition a user would want it to be

neon anvil
#

and will the PR now make shield damaging now apply cooldowns to modded shields? iirc it was hardcoded to vanilla shield

quick kernel
#

etc.

maiden dove
lucid harbor
#

always

quick kernel
#

vanilla never would? what do you mean

lucid harbor
#

Vanilla always calls blockWithShield if a block occurs

maiden dove
#

I'm not sure what the use case of this would be

quick kernel
#

#1198800008225501285 message

quick kernel
#

would be a separate PR I think

#

it's not even called from hurt

maiden dove
#

what is the semantic meaning of blockWithShield?

quick kernel
#

it's called from Mob#doHurtTarget lol

lucid harbor
#

I say hold off on the extra flag for blockWithShield for now

quick kernel
#

any particular reason?

#

what's the downside

maiden dove
#

does it mean "this damage is being blocked because the player is using a shield-like item", as opposed to "this damage is blocked because of an armor bonus or the like"

quick kernel
#

to an opt-in single boolean

quick kernel
maiden dove
#

I'm talking about the proposed blockWithShield boolean

quick kernel
#

you might be missing some context

#

basically when a shield block is successfully made, the game reduces the damage & damages the shield (durability), thne calls blockUsingShield on the target entity

sand copper
#

FYI, i'm not following the shield discussion, so please ping me with a summary once you all decide what we are going to do

quick kernel
#

that method is then used for things like the target mob being knocked back fro mthe impact

#

or the attacker disabling the shield with an axe

#

the proposal is to add a single boolean to the shieldblockevent to allow users to skip that specific call

#

for things such as un-disableable shields, shields that are considered too heavy to knockback, etc.

#

cus there's no other place to do so

maiden dove
#

so that they can do only the necessary logic themselves?

quick kernel
#

or no logic at all

#

cus you can't prevent the logic currently without mixin without this boolean

maiden dove
#

yeah if that's what they want yeah

quick kernel
#

if you block with a shield, you get all those effects, no exception

maiden dove
#

or only do axe logic, but no knockback logic or sum

lucid harbor
# quick kernel what's the downside

Its patch complexity on our side and some added mental complexity to the user to think about when they need to raise the flag or not, as opposed to it just being implicit

quick kernel
#

but modded shields may not want that

quick kernel
#

so it's really only if they want ti nteract with it

#

and it's not really patch complexity cus we're already wrapping the crap out of that subsection

maiden dove
#

so this would default to true?

quick kernel
#

uh it depends on the name

#

it'd default to do vanilla

#

I.E. no change

maiden dove
#

hm, then what to name it

quick kernel
#

and you could flick it to do the opposite (skip the vanilla functionality)

maiden dove
#

callVanillaPostBlockBehavior feels clunky, but gets the point across fairly well I believe

quick kernel
#

a single boolean conditional would hardly make a difference lol

maiden dove
#

yeah I'm all in for adding that boolean, just need to name it correctly

maiden dove
quick kernel
#

it should default to true

#

so the name should be inverse

#

skipVanillaShieldBlockHandling etc

#

unless it's a setter

#

the name for this sucks lol

maiden dove
#

because if we default to vanilla, then vanilla shield block behavior should run

quick kernel
#

oh I was thinking event method caller

#

yeah makes sense

maiden dove
#

(also I dislike shieldBlockHandling, I think shieldBlockPostBehavior is more exact)

#

the term "handle" is among the worst to use in a call chain because literally any piece of code can be "handling" something

sand copper
#

tweaked

image.png
maiden dove
# sand copper tweaked

DamageSequenceEvent is not, and should not be, directly invoked.
This disclaimer feels unnecessary in this state, abstract event classes throw anyway when you try to attach event listeners. Maybe do something like "DamageSequenceEvent provides a DamageContainer to its subclasses. Subscribe to the correct subclass of DamageSequenceEvent for your usecase." instead.

sand copper
#

i like that.

#

oh yeah. reading the rest with that feels way more organic and informative

sand copper
#

wt actual fuck. I can literally ctrl+click the symbol and get to the value. is this some weird load order thing with tests?

image.png
sand copper
#

Alright. changes pushed. Tests passed. Let's merge this.

quick kernel
#

invuln thing?

lucid harbor
#

right

#

So my problem with that event

#

Is the way that vanilla overrides isInvulnerableTo

#

ex: breeze

    @Override
    public boolean isInvulnerableTo(DamageSource p_312691_) {
        return p_312691_.is(DamageTypeTags.BREEZE_IMMUNE_TO) || p_312691_.getEntity() instanceof Breeze || super.isInvulnerableTo(p_312691_);
    }
#

Regardless of the event, the breeze is always invulnerable to BREEZE_IMMUNE_TO damage types, or to other breezes

#

but there isn't really anything we can do about it

quick kernel
#

yeah that's more of an issue with the event

#

than the captured boolean

#

which isn't really fixable unless we wanna patch everywhere

#

which I'm pretty sure we don't

#

so we just kinda went "ohwell, best we can do"

lucid harbor
#

Yeah

quick kernel
#

the event is still worth having

lucid harbor
#

it might be worth a warning at the class level though

sand copper
#

Like in the event java doc?

lucid harbor
#

Yeah

#

"This event may be unable to change the invulnerable status of some entities that override isInvulnerableTo against certain damage sources"

quick kernel
#

ehh

#

"This event may be skipped by some entities that override isInvulnerableTo and should not be considered a completely reliable indicator of existing invulnerability or incoming attacks"

#

maybe?

sand copper
#

i already pushed Shadows' wording >.<

#

If the wording needs to be different to merge, i'll check it out in the morning. It's like 2 hours past my bed time. good night everyone.

quick kernel
#

shrug
Less critical

lucid harbor
#

I do wonder if the damage container on the living entity should be a Stack of damage containers instead of a single object

#

Mostly because if someone calls hurt during the sequence from an event, it will not create a new container, but will null out the old container after it returns, and then crash at some indeterminate point later in the cycle

quick kernel
#

Queue or stack wouldnt hurt I guess

#

I could see that happening

sand copper
#

Makes sense. Though i need some help walking through this scenario.

Context: Player, LocalPlayer, and RemotePlayer have their own invocations of CommonHooks#onPlayerIncomingDamage (calls #1) which necessitates the instantiation of a DamageContainer before the one instantiated in LivingEntity#hurt. The current logic does a null check on the container field to prevent recreation of an existing container from an override method.

Problem: Since there is a possibility of infinitely recursive calls to hurt within damage listeners, we cannot assume the presence of a damage container is the instance for that level of recursion.

Solution Ideas:

  1. LocalPlayer and RemotePlayer don't use the event output in the return and still return false and true respectively. What is the purpose of the event firing here that isn't available from the invocation in Player or LivingEntity?
  2. as with the above, Player also has its own invocation, but does cancel out early if the event is cancelled. However, the only logic in Player that precedes super.hurt is the damage scaling. Do we need this event invocation here?
    a. note: the current PR stores the scaled damage in the damage container, but we could just pass it to super.hurt since we don't track the pre-scaled damage
  3. I propose removing CommonHooks#onPlayerIncomingDamage and relying on CommonHooks#onLivingIncomingDamage from LivingEntity#hurt, since they fire the same event anyways.

If we make that change, we can use a Stack and each invocation of LivingEntity#hurt would create a new one that it would also subsequently drop off at the end making for the LIFO behavior we want.

#

~~after writing this, this conversation sounds familiar.~~nvm I was thinking about the Player#actuallyHurt override conversation from a while back.

quick kernel
#

do the extra calls even need a damagecontainer?

#

LocalPlayer/RemotePlayer/Player#hurt

#

they seem like they're just meant to be standalone catches for extra stuff

sand copper
# quick kernel do the extra calls even need a damagecontainer?

If we create a special event that doesn't take a damage container and has no impact on subsequent logic, then sure. But these are copies of LivingIncomingDamageEvent that fire before the super.hurt gets called. On player this is a double firing of the event. On the local and remote sub-classes, there is no super, so it's the only invocation. I want to know why those events exist in the first place if listeners are using the same event as a regular livingentity event. There's nothing in the event that specifies it's for players only that would make it cleaner than an instanceof check. Seems out-dated to me

quick kernel
#

think they were there for client-side parity in overriding classes

#

or something

sand copper
#

I guess the local and remote events make sense. We can pass in a damage container that isn't stored in the entity for the sake of the event. They don't call super so it's kind of an isolated posting.

#

The one in Player though seems ripe for removal

#

Then we could use the Stack<DamageContainer> exclusively in LivingEntity#hurt without losing track of each entry's parity with events.

quick kernel
#

could just give it a dummy instance of the container tbh

#

I'm not sure the container is even relevant there

sand copper
#

that's what I was thinking. just instantiate one in the parameter line.

gusty moss
#

I'll push this to 1.21, we need to do a 1.20.6 stable

lucid harbor
#

I don't think it even makes sense to fire the events from LocalPlayer or RemotePlayer

#

damage is a serverside concept

#

firing it on the client in some niche cases just makes it annoying to reason about

sand copper
# lucid harbor I don't think it even makes sense to fire the events from LocalPlayer or RemoteP...

that's what I was thinking. I'll remove those patches, which is two less blobs on the patch count.

What about the one in Player though. vanilla only cancels if the damage is zero otherwise calls super, which would result in the same event being fired by LivingEntity. both Player and LivingEntity are both-sided. The only thing that happens between the two event firing that changes state is the damage modification on difficulty. I don't see how having the event fire before that adds value for modders, but it does create an imensely complicated issue for the goals of this PR.

lucid harbor
#

Its just the mechanism of application. The alternative would be i.e. applying an attack damage modifier to every zombie in the world based on difficulty

#

but you wouldn't change how the event operates in that case

sand copper
#

neat. that event hook was the only patch in RemotePlayer so that's one whole less file

#

we are up and running with Stacks for the damage container. i guess i'll just sit on this until we port to 1.21 then

quick kernel
#

I've lost track of where we're up to RE: the shield event, and also the potential associated boolean

lucid harbor
#

My decision was to just leave it as-is currently in the PR

quick kernel
#

ok
PR needs its summary event names changed btw @sand copper

#

also

#

should we add helper methods to LivingIncomingDamageEvent for adding modifiers to the container?

#

to better indicate that:

  1. That's even an option now
  2. That event is the appropriate time/place to do so
#

I'll add some stuff to the PR

#

cus I just noticed a a new problem too

quick kernel
#

submitted review
and I've updated the attached writeup

sand copper
#

@quick kernel getLast and peek do the same thing. The same is true for add and push. the only difference is the former are inherited methods from List whereas the latter are native to Stack. it's a nominal change, so i'm refactoring now.

quick kernel
#

does add not bypass this modCount incrementor?

#

peek also throws a more relevant exception if the stack happens to be empty

#

and doesn't instantiate a separate view and an iterator just to retrieve the element

#

peek just does a direct array lookup

sand copper
#

yeah, i'm looking at that now.

#

While you're here, what are these helper methods you have in mind?

#

for the first event

quick kernel
#

so you know how you currently have to get the container

#

to add a modifier

sand copper
#

yes. so just add a setReduction method that grabs the contianer for the user?

quick kernel
#

addReduction*

#

👍

sand copper
#

oh, right because we are passing functions to the method. wow i forgot we did that.

quick kernel
#

I left it as plural form of helper methods

#

because there's room there if we wanted to add overloads for each of the reduction types

#

for ease of use

#

but ultimately that's optional

sand copper
#

kinda going backwards from the code reduction Shadow asked me to do in the container, but arguably a better place to have that kind of repetition

quick kernel
#

I agree but personally I think it serves a couple rather important benefits

#

if DRY is important here, just the one addReduction is fine

sand copper
#

Changes addressed.

quick kernel
#

this isn't right still

#

that's only true for pre

#

it's also not just health

sand copper
#

what else is affected that you think needs to be in that javadoc?

#

absorption i guess?

image.png
# 
/**
 * LivingDamageEvent captures an entity's loss of health. At this stage in 
 * the damage sequence, all reduction effects have been applied.
 * <br>
 * {@link Pre} allows for modification of the damage value before it is applied
 * to the entity's health.
 * <br>
 * {@link Post} contains an immutable representation of the entire damage sequence
 * and allows for reference to the values accrued at each step.
 * <br>
 * For more information on the damage sequence
 * 
 * @see DamageSequenceEvent
 */
``` how's that?
quick kernel
#

👍

#

think we should be all set then for 1.21

sand copper
#

word. it's a shame we are just a day too short for the 1.20.6 stable, but with 1.21 this week, i get why maty held off.

quick kernel
#

any chance this is going to get in early in the 1.21 neoforge cycle giving how breaking it is?

gusty moss
#

if he rebases, I'll take a look at it tomorrow / on Saturday™️

sand copper
#

fixing the patches is always annoying. what do these symbols mean @@ -1127,23 +_,30 @@

lucid harbor
#

its the hunk location of the conflict

#

the advise for rebasing patches is just to discard the entire thing and redo it

restive ridge
#

yes please

lucid harbor
#

You can't "fix" them

restive ridge
#

don't try to fix them by hand

sand copper
#

got it.

sand copper
#

@gusty moss PR Rebased and tasks passed.

quick kernel
eternal krakenBOT
#
tslat

[Reference to](#1198800008225501285 message) #1198800008225501285 [➤ ](#1198800008225501285 message)ok
PR needs its summary event names changed btw @sand copper

sand copper
sand copper
#

@gusty moss for the Mutable Damage Container. Would renaming InternalDamageContainer satisfy your comment or did you have something else in mind?

#

right now it's
DamageContainer interface
InternalDamageContainer mutable implementation
ResultDamageContainer immutable implementation

gusty moss
#

i suggest DamageContainer and MutableDamageContainer extends DamageContainer

sand copper
#

so no interface, just an immutable class and a mutable sub-class?

#

currently ResultDamageContainer doesn't have any set methods because it's a record. the two that we do have throw errors to inform the developer that they've tried to invoke a set method during an immutable event

gusty moss
#

i want the containers to be typesafe when they're mutable in mutable events and immutable in immutable events

#

an immutable class with a mutable sub-class doesn't make sense

#

that'd mean the fields in the immutable would have to be mutable

sand copper
#

currently the events do handle mutability. the only event in which the container is immutable is the last one LivingDamageEvent.Post which takes the internally mutable container as a parameter but stores it as an immutable container within the event so that listeners have an immutable container in that event without impacting the actual mutable container on the entity

gusty moss
#

yes you're missing the point

#

although hmm

#

no yeah

#

the event shouldn't expose an object with set methods if those set methods throw

#

if we're making this as clear as possible for modders, it should also be using types safely

sand copper
#

what about this. what if we have only one Mutable DamageContainer object and we simply add the final values of what would be the immutable version as fields in the one and only event that is meant to be immutable. that eliminates the set methods and the type references

gusty moss
#

sure

sand copper
#

okay nevermind, the damage events all extend the sequence event which takes the container. so i'd have to pass in a null or otherwise immutable container anyways or break the Pre/Post format completely just to have an event that doesn't take a container

#

So then i guess instead of throwing, the set methods should just do nothing instead?

gusty moss
#

no no

#

that's even more confusing

#

it shouldn't have a set method if it can't set

#

that's why i suggested 2 interfaces

#

since in events with mutable you can just override in the event to return a mutable container

sand copper
gusty moss
# 
class DamageEvent {
  DamageContainer getContainer();
}

class LivingBlahBlahPre {
  @Override MutableDamageContainer getContainer() { return (MutableDamageContainer) super.getContainer(); }
}
sand copper
#

okay let me rephrase. in what scenario would someone be implementing this override?

gusty moss
#

it's not someone implementing it?

#

it's the event subclasses

#

since you use a superclass with a damagecontainer

sand copper
#

so you're worried about people extending DamageSequenceEvent with unsafe overrides? wouldn't that still be the case if someone overrode the sub-classes in the same way?

#

Are we talking about people posting events instances or listener implementations?

gusty moss
#

uhm no? i'm worried about LivingDamageEvent.Post extending damagesequenceevent

#

which currently directly provides access to a damage container with set methods

sand copper
#

Then we need to yeet the DamageSequenceEvent so that each of the current sub-classes takes the appropriate container explicitly and only extends LivingEvent

#

At that point the damage containers wouldn't need any inheritence at all because each event would be using it's own object with the first three events using the same type of object

#

So we end up with ```java
record ImmutableDamageContainer(...){}
class MutableDamageContainer{}

class LivingIncomingDamageEvent extends LivingEvent {
MutableDamageContainer container;
public LivingIncomingDamageEvent(LivingEntity entity, MutableDamageContainer container) {super(entity); this.container = container;}
}
//repeat for LivingShieldBlock and LivingDamageEvent(.Pre)
class Post extends LivingDamageEvent {
ImmutableDamageContainer container;
public Post(LivingEntity entity, ImmutableDamageContainer container) {super(entity); this.container = container;}
}

#

alternatively the Post method doesn't need to have a container. as with one of my previous suggestions, it could just have final fields for all of the container properties which provide an immutable representation of the values. It would mean one less class in the codebase

#

Then we would just have one DamageContainer that was inherently mutable without needing an immutable counterpart

gusty moss
sand copper
#

sounds good. i'll make those changes now

sand copper
#

Alright. changes made. Tests passed. lmk if there's anything else that needs updated. I believe all the javadocs are still correct, but I may have missed some details

sand copper
#

did a final cleanse of the javadocs and cleaned up some formatting the the IDE had thrown in there arbitrarily.

sand copper
#

javadoce formaatting addressed.

sand copper
#

@neon anvil comments addressed. ping me if you want to discuss more.

neon anvil
#

Shadow is the residential ASM subject matter expert if you want him to write one lol

#

The entity hurt thing… my code is multiloader so adjusting it to do your suggestion will be… painful for me

#

So just note that multiloader people may bypass some of your events

sand copper
#

in that case, i think the suggestion about using super.hurt at the end might be the better approach for your use case

neon anvil
#

Do any of the lower level events allow modders to change the damage? Like lower than hurt

quick kernel
sand copper
#

There's also LivingShieldBlockEvent which also fires in hurt and will accept changes to the damage.

lucid harbor
#

Otherwise it would be a good use of it

sand copper
#

That being the case, can we resolve that comment?

neon anvil
#

Ya

#

Could leave a todo to mention to re-evaluate with asm once the method is added to NG

lucid harbor
#

what happened to DamageSequenceEvent

#

did that just die or smth?

gusty moss
#

yes

sand copper
#

@lucid harbor why? this comment that would essentially make it the same as the method it is patched into, which is very different from the rest of the method names in CommonHooks

lucid harbor
#

Virtually all the method names in commonhooks are junk

#

The name should reflect the parameters and result of the method

#

A method name of onXYZ for something that returns a value is incorrect

#

because it isn't a response

sand copper
#

🤔 at this point, it's not important enough for me to fight if we want this merged. I'll change it.

lucid harbor
#

I have been meaning to write up a set of standard things as far as hook names and event conventions

sand copper
#

That'd be nice

lucid harbor
#

but I am quite short on time :p

sand copper
#

mood

lucid harbor
#

I think we're at a "good enough" point now

sand copper
#

Looks like Maty's review needs to be approved for the merge.

maiden dove
#

but similar to you, short on time

gusty moss
sand copper
quick kernel
#

oh damn it happened

#

damn fine work on the PR calti, I greatly appreciate it

neon anvil
quick kernel
#

no

quick kernel
sand copper
#

No, but we might want to drop the @ApiStatus.Internal annotation from DamageContainer. It's throwing a lot of IDE warnings in userdev.

#

nothing critical, just slightly annoying

neon anvil
#

@swift bluff close the one you triaged as duplicate lol

gusty moss
#

wait i just noticed

#

why did you annotate it with @Internal?

sand copper
#

It was leftover from when DamageContainer was an interface. Since it didn't throw any warnings in gametests, i never noticed.

#

I also think we could use some extra helper methods on some of the events to reduce the event.getContainer.xyz() bloat. i'll open another PR for the QOL changes

#

these won't be breaking though

quick kernel
#

.30 -> .31

swift bluff
quick kernel
#

I might hold off porting until that's in

neon anvil
sand copper
#

I know. I ported pmmo to 1.21 using the port PR. Pmmo is a bit of a beast to just casually port to a pr in a test branch though. I should probably make a userdev mod just for PR testing though. That's probably going to come in handy.

echo kiln
#

Am I reading DamageContainer#setReduction wrong, or do the reductions not actually modify newDamage there? (Basically trying to determine if there is a clean way to move the protection the mekasuit gives to the reduction system rather than just directly modifying the total damage in the event. And then after I figure out if there is a clean way, I have to figure out which way makes more sense to have it mechanically)

lucid harbor
#

Iirc setting a reduction doesn't immediately force a computation, but it will recalc when getNewDamage is called

echo kiln
#

getNewDamage is a simple getter

sand copper
#

the reduction functions apply during the vanilla armor rduction logic. Basically the sequence is Vanilla calculates a reduction => reduction functions operate on the output value => new damage is modified according to the final output

echo kiln
# 
this.reductions.put(reduction, modifyReduction(Reduction.ABSORPTION, amount));
this.newDamage -= Math.max(0, amount);
#

shouldn't the modifyReduction be set as the new value of amount

#

before the newDamage -= line?

#

that's what I am talking about

#

also actually

#

why does it pass it as modify absorption rather than the passed in reduction?

sand copper
#

you are correct. the modified amount is not being applied to the new damage.

#

it should be ```java
float newReduction = modifyReduction(reduction, amount)
this.reductions.put(reduction, newReduction);
this.newDamage -= Math.max(0, newReduction);

echo kiln
#

also some of the logic in actuallyHurt seems a bit wonky. Given CommonHooks.onLivingDamagePre returns newDamage, but unless my inutition is wrong, then it does float f = newDamage - newDamage, so shouldn't that always be equal to zero?

#

and setAbsorptionAmount just above that line is like absorption reduction - new damage + 0 OR absorption reduction - new damage + new damage - absorption reduction (which that is equal to zero)

#

so feels like thatlogic could be cleaned up

#

I think that about covers all the comments of things that seem weird or even straight up wrong from my work on switching mek to using the new pipeline

sand copper
#

I made it draft to get more feedback, but if we want to merge just for the bugfix, let me know.

echo kiln
quick kernel
sand copper
# 
/**
     * LivingDamageEvent.Post is fired after health is modified on the entity.<br>
     * The fields in this event represent the FINAL values of what was applied to the entity.
     * <br>
     * Also note that appropriate resources (like armor durability and absorption extra hearts) have already been consumed.<br>
     * This event is fired whenever an Entity is damaged in {@code LivingEntity#actuallyHurt(DamageSource, float)}
     * <br>
     * This event is fired via {@link CommonHooks#onLivingDamagePost(LivingEntity, DamageContainer)}.
     * 
     * @see DamageContainer for more information on the damage sequence
     **/
    public static class Post extends LivingDamageEvent {

and ```java
/**
* LivingDamageEvent.Pre is fired when an Entity is set to be hurt. <br>
* At this point armor, potion and absorption modifiers have already been applied to the damage value
* and the entity.<br>
* This event is fired in {@code LivingEntity#actuallyHurt(DamageSource, float}
* <br>
* For custom posting of this event, the event expects to be fired after
* damage reductions have been calculated but before any changes to the entity
* health has been applied. This event expects a mutable {@link DamageContainer}.
* <br>
* This event is fired via the {@link CommonHooks#onLivingDamagePre(LivingEntity, DamageContainer)}.
*
* @see DamageContainer for more information on the damage sequence
**/
public static class Pre extends LivingDamageEvent {

#

@quick kernel

quick kernel
#

I'm wondering whether it's worth including something in each of the javadocs stating what the primary use of the event is

#

as one last nip in the issue of players using the wrong event for things

#

or maybe just one note in the .Pre event saying if you are just modifying the damage, you should be using incomingdamage if at all possible

sand copper
#

maybe change void setNewDamage to setNewDamageOnlyIfImANoobAndRefuseToUseIncomingDamageEventProperly

#

maybe void overrideSequenceDamageValue(float overrideValue)

#

with a javadoce stating that LivingIncomingDamage is the correct place to make damage changes unless the intention is to alter damage after the armor and magic resist applied to the OG damage

quick kernel
#

nah keep the method name, it's fine

#

just a basic note

#

it's more so we have something in case people only look at that event

sand copper
#

okay, so yeah the absorption also needs tweaked. this is the current code (FQNs removed for readability)java this.getDamageAfterMagicAbsorb(p_21240_, this.damageContainers.peek().getNewDamage()); //properly sets the absorption amount in the reduction map, however modifies the newAmount value in the process this.damageContainers.peek().setReduction(Reduction.ABSORPTION, this.getAbsorptionAmount()); //Attempts to calculate a temporary holder variable for the post-absorption damage amount, but is using the already reduced value float f1 = Math.max(this.damageContainers.peek().getNewDamage() -this.damageContainers.peek().getReduction(Reduction.ABSORPTION), 0.0F); //just mathematically always equals zero. this.setAbsorptionAmount(this.damageContainers.peek().getReduction(Reduction.ABSORPTION) - (this.damageContainers.peek().getNewDamage() - f1)); //properly grabs the newDamage value after the event hook, but the damage in was busted from the start. f1 = net.neoforged.neoforge.common.CommonHooks.onLivingDamagePre(this, this.damageContainers.peek()); if (f1 <= 0) return; new code (same thing with the FQNs) ```java
this.getDamageAfterMagicAbsorb(p_21240_, this.damageContainers.peek().getNewDamage());
//works exactly the same
this.damageContainers.peek().setReduction(Reduction.ABSORPTION, this.getAbsorptionAmount());
//removed the temporary variable as it wasn't needed. Both the absorption reduction and newDamage value are what we want them to be already
//instead we reduce the actual absorption amount by the reduction (which may be larger from events for the sake of uber-absorption effects) but we can't
//actually reduce it less than zero so we clamp.
this.setAbsorptionAmount(Math.min(this.damageContainers.peek().getReduction(Reduction.ABSORPTION), this.getAbsorptionAmount()));
//event now has the correct amount going in
float f1 = net.neoforged.neoforge.common.CommonHooks.onLivingDamagePre(this, this.damageContainers.peek());
if (f1 <= 0) return;

#

@echo kiln ^ sound about right?

quick kernel
#

get a couple opinions before submitting just in case

#

I don't have time to look myself atm

sand copper
#

it's in draft right now as I expect probably a few more things we didn't think of

quick kernel
#

yeah
just want to try get this in as quick as possible since it's basically an addendum to an existing PR

sand copper
#

agreed. the reduction thing is absolutely a bug that makes that non-working

gusty moss
#

huh, it seems like everything introduced in that pr but the actual damage events had tests added

sand copper
#

I can add them in the addendum PR

gusty moss
#

you know what time it is, right? Stabby write a test for those events to make sure that the modifier thing is fixed (if you didn't already in your PR)

echo kiln
sand copper
#

I did. but mobile doesn't recognize java

echo kiln
#

Ah

echo kiln
sand copper
#

anyone know how to debug with gametests? IntelliJ only captures breakpoints in the base module not the neoforge module with the patches applied, so I can't even breakpoint my own patch code.

restive ridge
#

that sounds like a bug

#

I have had it working with the neoforge project for sure

sand copper
#

i'll play around with my IDE and see if it's a local issue

restive ridge
#

whoever wrote this... thank you it's so clear 😄

image.png
quick kernel
#
  1. isn't capitalised though and we should potentially consider execution for treason
sand copper
sand copper
restive ridge
swift bluff
#

smh, not using tone indicators for sarcasm or joking
banned /jk

quick kernel
#

@sand copper
if someone wanted to modify the damage to increase at a reductio nstage

#

is that even possible currently

#

like what if I have a mob effect that increases damage taken

#

as best I can tell that's not possible in a proper fashion because you've clamped the reduction (since the whole system is built on reductions rather than modifications)

#

which then leads to a conflict of naming between addModifier which isn't a modifier, only a reduction function

sand copper
#

🤔 I don't see why that shouldn't be possible.

quick kernel
#

the value is reduced, right?

#

so they'd have to provide a negative value

#

which isn't possible cus you clamp it to 0 min?

#

the whole idea of having to provide negative values is a bit odd though
or at least feels it.. idk

sand copper
#

Reducing by a negative would have that effect, yes. I think since the pipeline views them as reductions on the damage, it make sense to describe and treat them like reductions. However allowing the system to invert that assumption makes sense for those who find utility in that kind of modification.

quick kernel
sand copper
#

Not currently. We were discussing changing that I thought

eternal krakenBOT
#
tslat

[Jump to referenced message](#1198800008225501285 message) in #1198800008225501285

quick kernel
#

?

sand copper
#

yes we currently clamp. no it doesn't have to be. that's what we are talking about changing.

quick kernel
#

Ah

sand copper
#

I've run into an issue with building the gametest for this and i'd like some input. The main issue is how to capture succeed/fail conditions within an event listener. This may be an XY issue, but i'll circle back to that. I attempted to get around this, by caching values in the scoreboard and then checking them in the gametest sequence. This works, however, serverplayers have spawn invulnerability, that I cannot override without an AW so the call to GameTestPlayer#hurt always results in zero damage. So while players can store values in the scoreboard, they don't take damage. If i use a zombie instead, i can deal the damage, but I can't store in the scoreboard.

Where this might be XY is that my approach to the gametest may be the source of my problem. The structure of the gametest is essentially ```
test -> register LivingIncomingDamageEvent, modify damage, and set reduction functions
test -> register LivingDamageEvent.Pre, store post-reduction values, store current newDamage, set damage to zero
test -> register LivingDamageEvent.Post, confirm health change matches expected difference
test -> start sequence with player, give armor, enchant armor, give absorption, give resistance effect, hurt player.

If I could fail a test in an event listener, this would be easy, but the only way I found to do that (which wasn't reliable) was to us an atomic object of type consumer, store the `message -> helper.fail(message)` method in there and `get()#accept` in the event listener, but that just felt clunky and hacky.
sand copper
#

I realize i could just have the gametest wait out the spawn invulnerability, but i'd still like to know if there's a better way to fail a test within a event listener.

sand copper
#

figured it out. just stored the values in the entity's persistentData and added an idle time to the sequence to let the invul fall off

swift bluff
#

note that persistent data has a non-zero chance of being removed in the future™️
probably something for the person doing that removal to fixup kek

sand copper
#

It honestly just needs to be somewhere in the entity NBT, so if you have a location that's more futureproof, i'll use that

lucid harbor
#

"more futureproof" would be adding an attatchment

sand copper
#

that seems overkill for a gametest, unless you think otherwise

gusty moss
#

not really, we have helpers to register attachments easily

quick kernel
#

what's goin on with a bunch of these javadocs

swift bluff
#

IDEA doesn't understand {@return} fully correctly

#

particularly, nested javadocs tags (like {@link} in {@return})

#

a bug with IDEA

quick kernel
#

maybe it's cus the docs are weirdly compressed

swift bluff
#

nah, that's semantically equivalent to having it spread on three lines

quick kernel
#

doing it on my own thing works fine

#

etc..

swift bluff
#

IDEA understands @return the block tag fine -- it's {@return} the inline tag that it has trouble with at present

quick kernel
#

nope

#

well yeah it's the inline tag

#

but specifically it's the braces

#

this works fine

swift bluff
#

yeah, because when it's not an inline tag it's a block tag, which IDEA works fine with

lucid harbor
#

Yeah, {@return ...} doesn't support nested tags

#

If a javadoc needs nested tags it just shouldn't use {@return }

quick kernel
#

then just remove the braces lol

echo kiln
gusty moss
quick kernel
#

I sure love yellow lines lol