that sounds like a pain to deal with for my uses...
#Damage Events
elder drift
quick kernel
I'm not sure how else it could be approached really
without it being too specific for general use
elder drift
I'm just changing the protection value personally
which is always percentage reductions
reverse engineering percentage from flats is a pain
quick kernel
mmmm not really
elder drift
in a couple cases, I need to go from a piece that spoofs a vanilla enchantment to my own number for how much redunction it should be
elder drift
I have no way to tell if someone modified the value beyond vanilla, do I?
quick kernel
it should be fairly easy to back-trace the reductions
quick kernel
I have no way to tell if someone modified the value beyond vanilla, do I?
you do
the damagecontainer holds it
unless you mean to literally tell if they modified it?
elder drift
suppose vanilla says they have 10% fire protection, then someone says enchantments block 4 more damage. Now I cannot just tell how much fire protection vanilla granted
quick kernel
I'd argue you shouldn't need to - from any mod's perspective, another mod's modification should count as canon
elder drift
Similarly, suppose vanilla says there is 90% enchantment protection, I assume that gets capped down to 80% before I even see the number
quick kernel
that is true
elder drift
both basically force me to recalculate
quick kernel
but changing that functionally changes vanilla workings
elder drift
mods change vanilla workings
elder drift
the API could be setup so I change that, e.g. applying the cap later
elder drift
I get that you want to leave modders flexability to do whatever, but that reduces flexability to be like vanilla
I just want to change vanilla mechanics, not add brand new ones
quick kernel
this doesn't reduce any flexibility from what's already there lol
it significantly adds to it
elder drift
sure, but if I have to bypass your stuff entirely then the API has failed me
quick kernel
I'm not sure what you'd propose to fix that
the armour thing requires patching a class completely unrelated to the damage chain
CombatRules is where the cap is calculated, before it's ever applied
it's literally just a clamping getter on the armour reduction
quick kernel
ok, say we want to look at the enchantments thing
elder drift
that just an integer that gets computed, no reason it cannot be a float in the damage container
quick kernel
we don't even get that value lol
elder drift
no?
quick kernel
the first value we see is already clamped
elder drift
I am just thinking many of these values if I modify them, I'd expect some changes to propigate as they are not all flat reductions
quick kernel
you mean enchant protection?
elder drift
yeah
if nothing else, its a multiplier, not a flat number
I can deal with it as a multiplier
quick kernel
but others may not
elder drift
if I modify armor reduction for instance, encahnt redunction should change
quick kernel
it does
the enchant reduction method receives the newly reduced damage
every stage does
oh wait no cus there's no event
elder drift
its not about receiving the new damage
quick kernel
so yeah you'd have to compute it
elder drift
its about appropiately reducing it
if I do armorRedunction -5, and my enchant value is 50%, I'd expect enchantRedunction to have 2.5 less
quick kernel
how would we achieve that without literally putting in native backcomputing for every possible scenario or having an event at every step
elder drift
if that does not automatically update, this is insufficient, you are prioritizing hypothetical modders over vanilla mechanics
just make it a multiplier
elder drift
how would we achieve that without literally putting in native backcomputing for ...
does not matter how you do it to be honest, it needs to be solved for this PR to be useful
quick kernel
so have a separate setter that stores a multiplier of the value?
elder drift
replace it
elder drift
so becuase you might want to do something, I have to recompute vanilla mechanics to do anything before armor redunction, got it
quick kernel
isn't that the point of this whole discussion?
because you might want to do something I have to change the whole API?
elder drift
all the setters before enchantReduction are useless
because they do not have their changes reflected in enchantReduction
sand copper
This is starting to circle back to lcy's idea of tracking changes in their entirety and then applying them all at the end
quick kernel
^
elder drift
what stops us from just applying a list of named operations to the damage?
let people iterate the list, modify the operations or drop them?
sand copper
The concern is the added complexity that it brings for new users. There's already a disjointed and confusing chain.
sand copper
I'm not saying we can't add those things later, but baby steps.
quick kernel
which then raises the question of what value it's providing in the first place at the cost of the added complexity and obnoxiousness of the api
elder drift
I don't want more tbh, I just want to change vanilla mechanics without having to recompue everything
elder drift
I just add stuff thats like vanilla but cannot use their hooks
I don't want to not be able to use these events too
my old stuff is a messy hack
The way I see it, each stage just applies a list of vanilla operations, and lets modders either drop a new operation in that list, modify the list, or perform sideeffects if this is a reasonable spot. Between each stage, collapse the list to a single value
quick kernel
this is worse than icy's imo
icy's was at least only a single time
this would have to be done on like every modification to the value
so like.. 3-4 times
elder drift
honestly, there might only be one stage where you'd need the list, IncomingDamageEvent
By DamageTaken all the redunctions have been applied
quick kernel
right except the reductions chain one another
elder drift
DamageBlockEvent though would be part of the chain
quick kernel
so unless you mean like some kind of map of modification -> list<operation>
which starts to get absolutely wild lol
sand copper
I'd rather accrue the modifications until the very end and then collapse them on apply
elder drift
thats basically what I am suggesting
quick kernel
isn't that just what Icy's was?
sand copper
basically.
quick kernel
which we disassembled after discussion already
quick kernel
alternative suggestion
sand copper
we disassembled it because it added too much complexity to an already robust PR.
elder drift
the fact that I suggested the exact same thing without seeing it suggests some merit to it though
quick kernel
for a specific niche usage
elder drift
oh sure, but I have to recalculate everything as I did before
I cannot use your damage container in any form except that I now have to copy my recalculations into it
sand copper
yeah. there's no lost functionality than if you used the events as you did before
quick kernel
I don't see how doing your thing would help you either
you're still only going to see post-modification values
unless you just outright don't want the values to change until they're applied
which then removes their functionality for people who want to itneract with their values
elder drift
you don't need to change the values until its applied
quick kernel
which is arguably the majority of use cases
yeah thought so
so now anyone who wants to interact with the modified value cannot
sand copper
even then, the old events still gave you the original and new values
elder drift
you could always add a way to compute the value afetr a particular named operation
quick kernel
right except I would argue that the majority of use cases is interacting with the currently modified value
quick kernel
you think the majority of use cases is specifically people wanting to interact with only vanilla and not the game as a whole?
elder drift
The majority of usecases is either applying some formula adjustment, or responding to the fact damage was taken
formula adjustments don't care what the particular value is, its just a mapping old -> new
let them run their logic directly or let them pass in a mapper
responding to the damage being taken does not typically care the number
quick kernel
that's interesting
given that the API has never allowed for that
so you're saying the majority of use cases has been people doing mixins?
elder drift
the old API is literally that
elder drift
I see damage, I reduce it by 50%
quick kernel
the old api is literally the same as it is now
elder drift
I see damage, I add 5
quick kernel
but now you have more options
elder drift
no, I have more upkeep
quick kernel
then don't use the expanded options?
elder drift
because the new options are insufficient for my usecases but now I have to add all my modifications into it so I don't break other mods
quick kernel
you can use it the exact same way you already did, if you want
sand copper
I see damage, I reduce it by 50%
the only difference is the old way was Event#setAmount(newValue) and the new way is Event#getDamageContainer().setAmount(newValue)
elder drift
you can use it the _exact_ same way you already did, if you want
no I cannot
if I don't update the damage container, then any changes I make are going to break
quick kernel
the damage container is just an abstraction of setAmount/getamount that the old event had in the same place
you still have getAmount/setAmount
in the same location
elder drift
I modify damage at most of those stages apart from potions
and a lot of them require propigating changes
if I apply a redunction pre armor, I need to update armor
if I apply a reduction to armor, I need to update enchantmentReduction
if I apply a redunction to enchantments, I need to update shield
quick kernel
shield is handled before anything else is
but besides that, yes that's all true
but that's already how it is
it's no different
elder drift
no, before its 1 number
quick kernel
it is still 1 number if you want it to be
elder drift
I just modify it and no one cares why
quick kernel
elder drift
its not good before
quick kernel
elder drift
I'm just saything this does not solve anything
quick kernel
it doesn't solve your specific use case*, yes
sand copper
if I apply a redunction to enchantments, I need to update shield
uh, there may be some confusion here. The stored amounts for armor, enchant, etc are not ever used to provide the damage value. there are only there to give modders a value to reference for what happened
elder drift
this is hyperspecific to a hypothetical usecase rather than actual ones
elder drift
uh, there may be some confusion here. The stored amounts for armor, enchant, et...
and if I don't update the references, then the other modders cannot respond to my changes
why would you provide setters if I am not supposed to change them?
quick kernel
maybe they need to be marked as internal caltinor
sand copper
and if I don't update the references, then the other modders cannot respond to m...
yeah, but your'e a mod. don't pretend to be vanilla. Other modders are going to read that value as "this is what vanilla calculated as reduction from armor" and do with that what they want. If I were you I wouldn't change the value at all. I'd just change the amount and move on
quick kernel
that's how mods should be working lol
it's not you vs everyone else
you're a part of the pack
elder drift
all I want to do is modify the protection value
mods can then see protection is higher
sand copper
this sounds like something that should be a mixin to the protection enchantment then. not the damage chain
elder drift
my stuff is not "a mod so its not something they care about"
ah yes, I should use mixin rather than making the forge PR actually useful
why even do a PR at all then? we can all just mixin
sand copper
you're making something out of scope into scope.
quick kernel
ok so from what I'm seeing
there's only two post-damage stages for reduction
armor and magic (enchants)
that sound about right?
elder drift
"out of scope", being able to use the events to change how vanilla reductions apply is out of scope?
Just for context, do either of you acutally make a mod that would use these events?
quick kernel
yes
mint plinth
yes
elder drift
Armor for the most part can be modified via attributes
sand copper
yes. i use all the damage events
elder drift
Enchantments in vanilla is a percentage redunction applied post armor
quick kernel
I'm struggling to find where the newdamage is updated in the container post-reductions
elder drift
the container has a ton of setters suggesting I should be modifying those values based on what happened
sand copper
is newdamage ever updated in this pr?
yes. newDamage is the value that setAmount stores
elder drift
why not just ditch them and make them final?
or is the container created too soon for that?
quick kernel
because they have to be set
container is created at the start
it exists long before any of this
only way around is to recreate a new container every stage
elder drift
so the container is created back when a lot of this stuff does not exist? makes sense
quick kernel
which is.. super gross lol
elder drift
if I set values in the container earlier, will those just get deleted?
quick kernel
yes. newDamage is the value that `setAmount` stores
no I mean
I don't think you're ever modifying the damage
after armour or enchant is meant to reduce it
sand copper
which is.. super gross lol
I could always have the setters be an value = value == null ? newValue : value; so they can only be set once
proper shadow
epf's are definitely out of scope here imo
elder drift
it looks to me like onIncomingDamage runs before the container gets filled
quick kernel
I could always have the setters be an `value = value == null ? newValue : value;...
they need to be marked as internal regardless
yes because incomingdamage is before the damage is modified
elder drift
so it seems to me it should work that if I fill the container with values before hand, they just get applied when vanilla would use them
quick kernel
damagetaken is after
elder drift
I set absorption redunction, and my changes there are ignored until absorption runs
quick kernel
they're not changes
elder drift
and then they get added to the existing redunction
quick kernel
you're fundamentally misinterpreting the class lol
sand copper
oh, dang. you're right.
elder drift
so the class is very misleading
elder drift
honestly, why even have absorption redunction at all then?
it only matters in a single event later
quick kernel
because it's part of the damage chain
yes
that's the important event
all details are potentially relevant there
elder drift
the whole pitch of this PR was letting me modify values at any part of the chain
thats not what this is, this lets me see what the chain did at the end of the chain
quick kernel
and then act accordingly, yes
elder drift
I don't gain any new functionality
by the time wget to DamageTaken, that info is past useful
quick kernel
I agree actually
the point of effect triggers and damage modification should not be the same point
they currently are
elder drift
IMO, I should be able to add to enchantmentRedunction and have that applied when vanilla enchantment redunction is added in
I should be able to subtract from absorptionRedunction to make absorption absorb less
quick kernel
two separate issues
elder drift
If I cannot do that, just make more events
quick kernel
lol
proper shadow
I should be able to subtract from `absorptionRedunction` to make absorption abso...
this functionality seems not very useful
use case?
quick kernel
ok so fundamentally we have two issues
both of which I agree with - 1 is critical, one is less so, but still would be good to resolve, it's just a matter of if and how
1 - currently DamageTakenEvent is also the only place you can fundamentally modify the damage value with respect to all given modifications.
that makes DamageTakenEvent both a trigger point and a modification point
that's bad, and a failure of the API change
and 2. We don't really have a way of modifying individual modifications to the chain, only responding to them and backcomputing as necessary. It's not a killer, but not ideal either if it's practical to resolve
we have 4 modification points to the damage to keep track of:
- Armour reduction
- Mob Effect
- Enchant Reduction
- Absorption
doing a full blown op map was already rejected
proper shadow
IMO, I should be able to add to `enchantmentRedunction` and have that applied wh...
why? override Enchantment#getDamageProtection()
do you want to modify all enchantment protection values altogether?
quick kernel
they want to be able to modify any existing protection value
including to above max
vanilla max*
proper shadow
yeah idk that sounds like mixin territory, and above vanilla max is pretty firmly not vanilla imo
if you want a prot that works differently, you can make a new prot enchant
quick kernel
I agree on that, but I think a middleground could potentially be met
ok so proposal
move IncomingDamageEvent down to here
The early return doesn't need to be there at all since it's pre-modification anyway it's functionally not very useful
keep DamageTakenEvent where it is, it's fine
this allows IncomingDamageEvent to have all the values at their disposal, which is how it should be
the early return can still be there after IncomingDamageEvent (as it currently is)
to short-circuit the DamageTakenEvent if no damage will actually be taken
additionally
Instead of a full blown op-map, we have a Multimap (or EnumMap<E, List<>>), with an enum for:
- Armour reduction
- Mob Effect
- Enchant Reduction
- Absorption
With the value being a BiFunction that takes the damage container & vanilla value and returns a float
The idea being that devs can then add callbacks to the damagecontainer at PreDamageEvent that get iterated through and dynamically modify each individual stage of reduction, as it happens, with the resultant value being the one that gets returned back into the damagecontainer in the setter
this allows us to not have to fire like 8 events for an attack
and not go overboard on a full blown op map which gets crazy, fast
but still allows per-stage modification that propagates through the following stages
then it's all contained in a single enummap, 4-element enum, and a single method to add a callback
it also doesn't interfere with the set/getdamage simplicity for base use cases
that should basically solve knight's use case (as far as I can tell?) Whilst still maintaining everything we're aiming for with this
sand copper
would we treat vanilla reduction as an internal callback in this map?
quick kernel
no need
basically when you capture the value from the source
such as here
and you setter it in the damagecontainer, before you set it you run the list of callbacks on it
then the final set value is the post-modified value
which then becomes the 'reduction' value for that stage
whether it's vanilla or not is only relevant if you're trying to do hyper-specific stuff which I don't see as being within scope here
sand copper
and you setter it in the damagecontainer, before you set it you run the list of ...
you mean before you apply the modification to the newDamage?
quick kernel
yes
base protection computed by vanilla -> run sucessive list of operations -> set protection value in container -> mod newDamage
each callback receives the result of the previous callback
this would allow someone like knightminer to modify the protection value, or the armour value, etc as it's being retrieved
on an individual basis
elder drift
yeah idk that sounds like mixin territory, and above vanilla max is pretty firml...
to be completely honest, this is a disucssion of a forge PR, and I am being told that my usecases are not valid and should just be mixins, this really does not give me confidence in this PR
"just do mixins" is not a good argument for why a PR is fine
proper shadow
I can come up with many use cases that shouldn't be PR'd into forge
just because its a use case doesn't mean its valid
elder drift
just because its not handled does not mean it shouldn't be handled
sand copper
okay so you end up with something like this for each of our 4 enums ```java
void doModifier(float vanillaValue) {
this.modifiedValue = modifiers.get(ThisType).streamAndReducePseudo(...);
this.newDamage -= this.modifiedValue;
}
yep
quick kernel
it's a simplified but hopefully still quite powerful stage-based modification thing
elder drift
if you want a prot that works differently, you can make a new prot enchant
to be clear on this point, no you cannot, because protection is very hardcoded (hence why I need wrappers)
quick kernel
with the full values being modifiable at the end via IncomingDamageEvent
proper shadow
can you just read the proposal I made
it suffers from the "order of operations" issue but honestly its niche enough for this to not really matter
quick kernel
it always will
there's literally no way not to suffer from that
it's an unsolveable problem lol
proper shadow
having an enforced order of valid operations would solve it
but like I said, not worth it
quick kernel
or just have your event listener be a different priority
sand copper
now here's the question. Do use this same pattern to capture changes to the amount in the same way? eg. collect functions then reduce them when the value needs to be committed.
quick kernel
no it's only for the four stages
after that users change via set/get damage as before
elder drift
that should basically solve knight's use case (as far as I can tell?) Whilst sti...
this seems reasonable at a glance
quick kernel
this is just a middleground to try to support knightminer's usage (which I fully agree is a valid use case and useful)
elder drift
I do think I'd argue its worth putting vanilla into that map
quick kernel
without going crazy about it
elder drift
as that makes it easier to remove vanilla
e.g. my usecase is basically replace vanilla with slightly altered vanilla
quick kernel
yeah but you're one mod in a pack
not the mod
we should be working with other mods
not trying to ignore them lol
sand copper
no it's only for the four stages
what we did for the four stages is basically what lcy was asking for. your solution to the 4 is simple enough that we won't confuse users. All we have to do is clear the list each time we flatten so that there isn't an accumulation of functions over the course of the chain
elder drift
which is why I want to modify vanilla instead of just applying an inversion function
quick kernel
what we did for the four stages is basically what lcy was asking for. your solu...
the list is fully populatred in PreDamageEvent
you don't need to clear it at all
it'll be gone by the time the object dies, which is immediately after when you'd be clearing it anyway
so you can leave them alone, more or less
basically if you wanna add callbacks, you add them in PreDamageEvent, that fully populates the map<E, list<mods>>
then each stage just grabs and runs its list of mods by its enum
and at the end we have IncomingDamageEvent, shortcircuiting the return if applicable before the stats are awarded, and DamageTakenEvent is called
proper shadow
the event names probably need to be tweaked, I don't know what they do still besides PreDamageEvent, and I've been following the progress on this
sand copper
and at the end we have IncomingDamageEvent, shortcircuiting the return if applic...
at this point, you might as well move DamageTaken to the end of hurt instead of actuallyHurt so listeners can observe the entity as it is immediately after being hurt, then leave IncomingDamage as the "during actuallyHurt" event
quick kernel
IncomingDamageEvent is when damage is incoming
DamageTaken is when damage is taken lol
quick kernel
at this point, you might as well move DamageTaken to the end of `hurt` instead o...
no because mc messes wit hthe value
hurt can have a different value to actuallyhurt
cus it calls it two different ways
plus the contract of actuallyhurt implies that's where the damage is actually taken
so it makes sense to keep the event there
sand copper
oh right, because there's no callback on actuallyHurt. in that case why not just have one less event?
wait...
we have the damage container
we always know what was actually applied to the entity
quick kernel
oh right, because there's no callback on actuallyHurt. in that case why not jus...
because half the point of this PR is to split modification and triggers
having an event that's used for 'do X when I take damage'
where you know quite reasonably that the damage is there, modified
as opposed to potentially reacting to damage while it's still being modified by other mods
which then makes event priority a mess
yeah no I'm still wanting the event in actuallyhurt, per the contract of that method
moving it out to hurt would only be done to make it so you don't have events too close together, which is an incredibly minor thing given the context imo
proper shadow
burned into my mind is the reality where these aren't split and it hurts
kinda why I ever responded to that initial damage event message months ago haha (ive been killed in frustrating ways from things not happening in the right way)
quick kernel
the damage chain is an absolute mess lol
the fact that vanilla calls the invuln check like 3 times every attack is insane
sand copper
yeah but i don't see how they'd do it differently.
quick kernel
says the guy that just redid the player#actuallyHurt method cus it was so redundant lol
sand copper
lol. different in this case
quick kernel
So 3 things to fix:
- The newdamage is never actually set lol
- Move IncomingDamageEvent down to new spot
- Implement callbacks
I think that's everything
then open to new names from forite if they have any more applicable
proper shadow
and there's a chance im only befuddled because the old hook names were there too the last time I was checking it out
quick kernel
@elder drift that roughly solve your primary concern?
proper shadow
If there exists a much more compelling set of use cases for the 4 things that will be covered by callbacks, would we change that paticular bit? Contrary to how I was before, I'm pretty open for a lot here just because I realized that damage calculations are quite important for a paticular genre of mods that I'd love to see mature
proper shadow
If there exists a much more compelling set of use cases for the 4 things that wi...
I was thinking about how tech mods have a lot of interop infrastructure with things such as energy, fluid, logistics
combat/rpg mods could potentially benefit the same way
sand copper
pardon my braindead status, but is there a better way to write the loop part?
proper shadow
I'm hopign to implement callbacks _now_
im being hypothetical and doing it with a stream of conscioussness my bad
sand copper
👍 but i'm not overlooking some fancy stream method that would do the same right?
sand copper
look good check.
updated DC
still needs javadocs
if functionality looks good, i'll add the javadocs and push
sand copper
check the patch for magic absorb. since that method does mob effects and enchantments, the setters are patched in
sand copper
newDamage is set inside DC like armor
quick kernel
f1 is the damage delta
not the absorption itself
we should be capturing this
then using it
sand copper
f1 is how much of the absorption is consumed by the damage. that's what we want to store. if the user wants the absorption, they shoudl call getAbsoprtionAmount
quick kernel
no f1 is how much damage is left after absorption
which means we want to know and modify absorption before f1 is even computed
sand copper
oh, so i should store f
sand copper
oh wait. i see what you mean
quick kernel
then using that captured value in f1's calculation
so that the modified value is used to do f1
elder drift
<@205154591711559690> that roughly solve your primary concern?
give me a second, catching up
elder drift
they seem reasonably clear to me though
I agree that the new names are way better that what we had before
elder drift
yeah no I'm still wanting the event in actuallyhurt, per the contract of that me...
weird question, do we actually need actually hurt? would it be reasonable to just delete it entirely to make the chain cleaner?
elder drift
<@205154591711559690> that roughly solve your primary concern?
I think that would solve it. One question, is vanilla a callback, or is vanilla the input to/output from the callback?
elder drift
e.g. if I wanted to say "the vanilla protection here does not apply", I'd still have to do my inversion logic, I just have a cleaner spot to drop it
quick kernel
if you want to get in early you can register your callback with a first-priority event handler
so your callback goes first-ish
elder drift
yeah, thats doable
so if I want to change vanilla, I just toss in a callback that applies an inversion (delete vanilla) then runs the vanilla formula with my changes
quick kernel
that's one way to do it yeah
elder drift
which is essentally what I do now, except I have guarantees of when mod handlers will run instead of no idea
quick kernel
you don't even need to invert if you don't want to
you can just register a first-priority listener for your callback
then assume you're getting vanilla
and return your own computed value
elder drift
the vanilla cap and the stacking of percentages is why the inversion is needed
quick kernel
you don't even really need to re-compute vanilla, you can just compare the armour value vs the original damage
if it's > 80% you can mod as necessary
or whatever
elder drift
there is a differene between reduce by 20% and reduce by 10%, reduce by 10%
quick kernel
yeah but if you prioritise your callback
then it's not stacked
well, usually anyway
unless someone else is really trying to do the same thign
but yeah if you wanna be super sure you can invert
elder drift
the alternative is I compute the effective change given vanilla, same idea
quick kernel
yep either way
but the main thing is your change is per-modification now
and propagates
elder drift
yeah, which is what I lacked before
before if someone else is doing say, a potion damage reduction, they just run after my logic same as an armor value reduction
while in reality one should be after and the other before
quick kernel
their reduction would run when all mob effects are done
since they're individual units
so armour -> effects -> enchants -> absorption
sand copper
alright. I think this covers absorption
elder drift
One question worth asking: does it have to be an enum or is a list with arbitrary types reasonable (like tool action interned stuff)
I assume it has to be an enum as we run it in specific spots, not just iterate the enum
quick kernel
^
it doesn't have to be an enum, but this solution is already a middleground to reduce needless complexity
elder drift
in theory if we patched out the vanilla stuff entirely and made it use this system though, the enum is not needed
elder drift
but yeah, thats more complexity
sand copper
oh come one Tslat. what's the harm in patching out entire vanilla methods 😉 /j
quick kernel
also efficient in terms of the collecting map
elder drift
Last question: damage container sticks around, and just gets the result of each stage for communicating it to the hurt method?
or to communicate stages pre this to the map gathering method
quick kernel
basically
elder drift
you know, the container should use the enum
quick kernel
PreDamageEvent: cancel (or if you really want to, modify the damage) of the attack, and register callbacks
elder drift
lets you have just one array for all values from the enum
quick kernel
then all the callbacks are called at each stage
elder drift
reduce a bit of boilerplate
quick kernel
then IncomingDamageEvent is called with the post-modified values
the container does use the enum
watchu mean
the enum isn't even used outside of the container lol
elder drift
I mean, instead of setEnchantmentDamage, you do setIncomingDamage(DamageType.ENCHANTMENT)
sand copper
updated DC
^ container class for reference
quick kernel
that's worse
cus then you have to split setters by enum
funnily enough
the more generic name would also be more confusing to devs
since it'd be more likely to invite confusion RE: which damage setter to use
elder drift
eh, fair enough. Just figured it might reduce effort in using it
quick kernel
as it stands atm we just have 4 setter methods with 2 field sets in each
so it should be pretty clean
elder drift
the other thing the setter could have done is taken in the list to make it clear, its meant to be called using this list
though that would probably require moving vanilla logic, so bigger patch problem again
quick kernel
the list is only ever stored in the container
so it doesn't need to externalise it
basically the way it is currently is the entire callback mechanism is contained within the container
it's never externalised
elder drift
so I only get to add to the list, not remove from it or iterate it?
I don't think I need to do either
elder drift
at most I might want to run the list probably
quick kernel
you already will
if you just make your callback first
then they'll all run after you as normal
although I would argue @sand copper
elder drift
no, I mean, maybe one of my "on damage taken" mechanisms will want to know the resulting value and cannot be in damage taken
quick kernel
that this might be better served as an enum map
LinkedListMultimap<Reduction, Function<Float, Float>> reductionMap = LinkedListMultimap.create();
EnumMap<Reduction, List<BiFunction<DamageContainer, Float, Float>>
elder drift
though I'd probably miss out on vanilla stuff anyways
so I guess if my callback is in incoming damage, I just accept I don't know what will happen after all reductions
quick kernel
wh ycan't it be in damagetaken?
elder drift
that is canceled by absorption/0 damage basically
which for some callbacks is great, for others not so much
quick kernel
you'll have IncomingDamageEvent for that too
since it runs after the staged modifications
elder drift
I cannot think of a callback I have that needs to know the final damage off the top of my head though
quick kernel
but before DamageTaken
elder drift
wait, how does it run after the staged modifications if its where I add staged modifications?
or do I add stages in pre?
quick kernel
pre
elder drift
ah, makes sense now
elder drift
I think that deals with my issues then
Pre -> do my weird armor calculations
Incoming/Hurt -> do my callbacks for when you take damage
ArmorHurt -> useful, but probably won't need as stages will deal with this automatically
quick kernel
pre is where you register callbacks that do your weird calculations
then they're run before Incoming happens
elder drift
yeah, thats what I meant
my weird calculations are based on numbers that are not damge to see if my callbacks are needed
e.g. your modifier levels on armor
quick kernel
mmk
then yeah that's about right
armorhurt is more for modifying the durability damage
not the attack damage
elder drift
yep, I have to handle that right now because I change how much damage is blocked by the armor
quick kernel
yep
elder drift
but shouldn't with the new hooks if incoming is delayed
and pre can add stuff before/after armor hurt
quick kernel
hm
no you'll probably still need it
cus annoyingly enough the armour is damaged before vanilla calculates reduction
which is.. stunningly stupid lol
elder drift
let me double check why I needed what I needed
quick kernel
elder drift
what I did was just consistency with vanilla stuff
elder drift
ah yeah, thats it
quick kernel
no amount of damage modifications should impact armour durability taken
elder drift
I reduce the damage before protection runs
as a result, armor takes less damage as I cause it to not take damage that was reduced by protection
elder drift
so I fix it by just calculating how much damage vanilla would have dealt had I added protection at the "proper spot" then damage the armor by the difference
quick kernel
ahhh
yeah so you won't need to do that anymore
so this just eliminates that entire need for you
elder drift
Just so my use case is mentioned, my shields have a block angle stat, which ranges from 0 to 180 (vanilla is 180)
quick kernel
elder drift
this should let me do a full 0-360 easily
quick kernel
Just so my use case is mentioned, my shields have a block angle stat, which rang...
called it caltinor 😉
sand copper
LinkedListMultimap<Reduction, Function<Float, Float>> reductionMap = LinkedListM...
I used LinkedListMultiMap because it generates a non-null list for every entry. So you don't have to worry about instantiating a list on your first entry into the map. I could still make it a bifunction though
quick kernel
I used LinkedListMultiMap because it generates a non-null list for every entry. ...
I'm just trying to make performance as fast as possible
since htis is a high-traffic area
elder drift
I just need the ability to say
- vanilla would block here but I don't
- vanilla wouldn't block here but I do
- vanilla would block all damage, I only block up to X
quick kernel
that's all possible now yes
sand copper
done, done, and done
quick kernel
you can also modify the durability damage shield takes
sand copper
^
elder drift
before I could do 1 and 3, but 2 required reimplementing all shield logic
oh, modifying durability is good
sand copper
the event's isInvulnerable is the last check before passing back to vanilla. so whatever the event says, is the final say
quick kernel
after that stage the attack is considered successful too
it's just a matter of what damage
sand copper
I'm just trying to make performance as fast as possible
slower than a computeIfAbsent?
quick kernel
you know what still really bugs me
vanilla still does this
modify damage amount after it's used
lol
elder drift
what makes computeIfAbsent slow?
is it the creation of the lambda or something else?
quick kernel
computeifabsent, nothing
but enummap is a fixed size universe with no loading factor
based on two arrays
it's about as fast as you can get for a map
elder drift
so an enum map to list with a compute if absent is probably the best approach
quick kernel
^
enummap skips hashing too
it's an ordinal array check
it's efficient as fk for a map
elder drift
I really should make more use of enum map in my code, I tend to do mostly immutable map builders
granted, I don't do many hot spot methods
quick kernel
I try to be efficient everywhere
legibility -> efficiency
but efficiency isn't discarded just because it's second
it all adds up
elder drift
I both want to be efficient and flexible. If the map is ever exposed publically then I have to qustion whether I need to enforce an immutable map
as most of my objects are immutable
sometimes its just easier to publically expose the map than reimplementing all the useful map getters
quick kernel
google commons has an immutable enum map
elder drift
oh, does it? I should look into that
quick kernel
mm
elder drift
does it have a pretty builder?
quick kernel
it's more of a factory wrapper
ImmutableEnumMap.asImmutable(enummap)
you don't really.. need a builder
elder drift
half the builders in google are just wrappers around a java map anyways
quick kernel
enummaps use array traversal for its iterator
so you can use it in a loop very fast too
people really need to use em more lol
elder drift
I do wish there was a middleground between enum map and object map for objects with a reasonable int ID and max that are not enum
quick kernel
no loading factor, no hashing, fast iteration
elder drift
maybe I should just write my own...
quick kernel
I do wish there was a middleground between enum map and object map for objects w...
Int2ObjectMap(capacity)
elder drift
okay, thats a fair option if I add a layer of abstraction before map calls
quick kernel
sure
it does 99% of the work for you though
additionally
you can use Int2ObjectArrayMap if you do that
so you can get a fast iterating one
elder drift
mostly just makes full key/value iteration trickier, everything else is easier
elder drift
instead of Entry<Key,Value>, I have to do IntEntry<Value> then lookup(entry.getIntKey())
its a trivial amount of additonal code!
quick kernel
nah
Int2ObjectMap.Entry extends Map.Entry<Integer, V>
so you can just implicit upcast
elder drift
only if my object extends integer
quick kernel
it's an int map
elder drift
I mean I want the key object not the key ID
e.g. if the keys are blocks, I want the block
quick kernel
oh that's not an intmap then
wait so now I'm confused as to what you want lol
just sounds like a normal map
fixed-size?
elder drift
blocks are just a conveient example
I have some small key based registries that use a RL key
sand copper
we are updated with an enum map. wrapping up the javadocs now
elder drift
they are small, so a map using them as keys could use ints instead
they could be an enum, I just don't make it an enum to be flexible for addons
elder drift
it just be moderately faster 😛
all the benefits of enum map as my object is enum like but not actually an enum
quick kernel
Object2ObjectArrayMap
but yeah the more abstract you go the less benefit you get
that's just an inevitability
you really should just use an enum lol
if you need to expand later you can
quick kernel
you really should just use an enum lol
No mojang this wasn't directed at you, go away
quick kernel
we are updated with an enum map. wrapping up the javadocs now
so a followup question then:
Do we think we'll be looking at the split-type damagesource thing or is it just too out of scope
then I'll rewrite the primer to match the PR so it can be referenced as an overview
sand copper
I think we look at doing that in phase 2 for now. this wille be enough of a change for people
If we can get people used to using the DC, we can look at expanding the DC to do things like split damage sources
sand copper
so basically Alembic
sand copper
i'm wondering if I should add a child class of DamageContainer that provides an immutable version so that listeners can't change the values and disrupt other listeners in the DamageTakenEvent
just have the hook convert it to it's immutable version and noop the setters
quick kernel
nah leave it
I even mentioned that in the writeup
you may need to modify it there for some reason, although it should be discouraged in general
sand copper
yeah, but modifying the values in this even has no effect to the chain
proper shadow
so a followup question then:
Do we think we'll be looking at the split-type dama...
can this be faked in any effective way via 0'ing iframes and doing more damage calls?
mod impl side
quick kernel
yes
sand copper
so all doing that does is confuse other modders
quick kernel
ah
hmm
yeah in that case then it might be worth making a no-op equivalent to damagecontainer
idk about subclassing it
maybe just straight up a record with the values in it
subclassing could still be misconstrued, especially given that the IDe will still populate suggestions and there's a concerning amount of people who jsut do whatever the IDE says to do
quick kernel
elder drift
You know, rather than marking all the setters internal, why not just make an interface with all the methods modders are supposed to call and use that as the type for the event?
Nothing stops modders from casting to call the setters, but the fact you have to cast makes it more clear it's wrong
sand copper
not at all. i'll make that change now
sand copper
yep!
quick kernel
although the damage setter might still be a problem
elder drift
Could even give each event it's own interface
elder drift
So only the ones populated are exposed
quick kernel
the only difference really is the last event which is noop
sand copper
i've got the implementations nested in the interface. I'm not trying to make more class files, but should it be split?
sand copper
public interface DamageContainer {
/*methods...*/
public class InternalDamageContainer implements Damagecontainer {...}
public record ImmutableDamageContainer(...) implements DamageContainer {...}
}
yeah split like that
maybe ActiveDamageContainer and ResultDamageContainer are better names
sand copper
okay so not separate files. aka keep as is. but just update the names.
gonna keep it as InternalDamageContainer but change the record to ResultDamageContainer. I really want to stress that the internal one really is for internal use. And if someone is dumb enough to cast their container to it in order to get the methods, they've been warned
quick kernel
ehhh
I feel like the fact that you're casting it implies it's internal
it's never overtly exposed
so having it named more appropriately for what it's for is better legibility
elder drift
Just annotate that class with API status internal, should be sufficient
quick kernel
sand copper
thanks. PR updated.
sand copper
@restive ridge regarding #dev-announcements message, Should I expect to resolve merge conflicts after a 1.20.5 port, or would the intention be to merge this before the 1.20.5 release? Of course, that's assuming this is something that is discussed enough to be worth merging.
restive ridge
in any case you'll have to resolve merge conflicts once we have rebased and squashed the porting work
whether we merge it during the 1.20.4 frozen window or during the regular 1.20.5 BC window I don't know
if has a large effect on many mods we should get it merged before the regular BC window
sand copper
okay. I will keep an eye on things so I can resolve conflicts in a timely manner
elder drift
Any reason for the boxing on the float just to call intValue? Is that somehow better than an int cast? In armor damage event
Figured I'd ask here rather than the PR as its not a needed change, just something I'm curious as to the reason
I think for the most part the PR in its current state looks good apart from some minor style nit picks. My only remaining functionality concern is the damage block event looks like it defaults to shields not taking damage, which is a pretty big vanilla break
I think the cleanest fix is just making the following
float shieldDamage() {
if (newBlocked) {
return shieldDamage >= 0 ? shieldDamage : dmgBlocked;
}
return 0;
}
and start shieldDamage at -1. Means the shield defaults to taking damage to mimic the block amount, but can be specifically overridden if you want another amount.
sullen nebula
which way does intValue even round
elder drift
I think it floors like an int cast
vanilla just does an int cast in the relevant code
elder drift
no, its durability loss
sullen nebula
ah, it just casts internally
i'd certainly hope it'd be JITed, but a cast is probably better style
timid kelp
chances are it just truncates
(i think casting truncates... if it doesn't i may have a few bugs to fix)
quick kernel
@sand copper
How you goin with this?
1.20.5 will probably be soon so we need to start probably thinking about what version this gonna target
sand copper
Based on Shadow's comment on GH, this is slated for 1.21. I can resolve merge issues post 1.20.5, but I don't think it matters until we get a 1.21 release.
lucid harbor
I set the 21 target conservatively based off of the commitment we already have to changes for 20.5
sand copper
Just let me know if we need to resolve merge for 1.20.5. I can do that pretty easily.
restive ridge
I think it will mostly boil down to whether Shadows has time to review it
quick kernel
My main thought is that 1.20.5 is already gonna be crazy breaking for mods
so this change slipping into that would be kinda ideal
as opposed to potentially having to make it for 1.21 which may not be as breaking
restive ridge
I don't think that this change is that breaking
quick kernel
it completely changes every damage event the API has
a lot of mods rely on damage events lol
it's pretty breaking
it's not crazy breaking, sure
but it's not really a blip either
restive ridge
given the recent changes anything that doesn't affect at least half the mods is pretty mild
quick kernel
I mean yeah fair call
lol
damage events might not be too far off that though
they're pretty widespread in use
sand copper
Well damn. the hurtArmor method no longer exists in 1.20.5.
or rather, it redirects to a different method
ugh. this is gonna be an annoying refactor. This is a problem for tomorrow me. It's too late for me to wrap my head around it tonight
quick kernel
lol
sand copper
alright, with a fresh mind to work on this, it's actually quite advantageous to us. The armor hurt logic moved to LivingEntity which is great because it opens up the possibility for armor breakage of many more entities. The vanilla case is wolves. The hook also got simpler since the new method takes EquipmentSlots as a parameter and doesn't use the static Inventory.ALL_ARMOR_SLOTS int array.
merging upstream overwrote almost my entire PR. AAAAHHHHHAHH!!!
sand copper
whew. okay looks like it was just the patch stuff which makes sense.
sand copper
does anyone know if there's a way to AT in a gametest? I basically want to invoke a protected method
ah actually, for this test, it wasn't needed. I was overthinking it
sand copper
ugh, this is annoying. having to call Player#hurt just to trigger the armor event means I have to factor in all of the behavior in-between to make sure the test behavior actually gets reached.
quick kernel
for gametest?
sand copper
yeah. i think i found a bug though.
sullen nebula
alright, with a fresh mind to work on this, it's actually quite advantageous to ...
The vanilla case is wolves
what about horses, or are they still special?
do wolves have a special slot for armor or does it go in their chest armor slot? what about horses?
sand copper
I can look when i get off work. it might.
elder drift
does anyone know if there's a way to AT in a gametest? I basically want to invo...
reflection is probably what you want
elder drift
> The vanilla case is wolves
what about horses, or are they still special?
do wo...
horse armor does not have durability
another classic case of vanilla refusing to revisit old features when they add new ones
sand copper
which i'm pretty sure it also doesn't extend ArmorItem which is an instanceof check for all armor damage logic
sand copper
Merge conflicts resolved. The armor change actually simplified the Armor event code, which was nice
Now, hopefully this gets merged before any more patches throw off my changes
sand copper
@lucid harbor any chance we could get the 1.21 flag removed from this PR so it's not overlooked?
lucid harbor
Sure
sand copper
thanks. I also just fixed the branch updating errors, so it's good to merge whenever.
lucid harbor
the classic™️
sand copper
😭
sand copper
so is this going to be held off for 1.21 or should I spend the time to update for the latest NF version? Fixing constant merge and patch errors is getting tedious so i'd like to do it as infrequent as possible.
restive ridge
Hmmm technically we can make it happen if we want to
quick kernel
sand copper
If it's in the cards for this week i can get the PR updated with the latest NF and ready.
restive ridge
I have targeted the PR to 1.20.6
sand copper
@neon anvil (named in the blame) and @gusty moss I encountered this error while updating this PR for the latest updates. https://pastebin.com/HnJpTvR1
what's most noticeable is that the test run says it's successful despite no tests actually being run. I figured this probably undermined the contribution pipeline and was pretty important to bring up.
as a proof point, check the last tests run of this PR and you'll see that it succeeded without any tests run.
sand copper
<@&1128776937410670663> sorry for the group ping, but i worried this test issue might mean larger issues related to PRs being merged that aren't actually gametest valid and don't want this to become a bigger issue.
swift bluff
👀
swift bluff
https://github.com/neoforged/NeoForge/blob/86876fafc86efd8f20ac0cb0a172c7683fe39115/tests/src/main/java/net/neoforged/neoforge/debug/block/BlockTests.java#L150 needs to be fixed to have the size be 3, 3, 3, since the fill occupies a 3x3x3 space (0-indexed coords)
after that fix, tests should work
though really, the game test server should cause a failure if it crashes
neon anvil
how did my pr then pass for that
swift bluff
the problem seems to be that Gradle doesn't rethrow an exception when the server exits by crash
I'm not sure if that's because Gradle is ignoring the exit value, or the game test server isn't exiting with an error exit code
neon anvil
sand copper
Sweet I'll grab that PR commit and get back to testing this PR. Though until the successful failure test is fixed I'll have to manually make sure my tests are succeeding.
sand copper
This is updated and ready for merging.
swift bluff
go go go @restive ridge 
could you post a message with a link to your PR? so I can pin it
lucid harbor
https://github.com/neoforged/NeoForge/pull/792
sand copper
Looking at the milestone this PR is included in, Is there anything NF is waiting on that delays this PR to be merged?
restive ridge
No, we just need people to review it
sand copper
got it.
sand copper
@lucid harbor regarding this comment, is it common to put those annotations and comments in patches?
sand copper
mainly you say to put a javadoc which would either be another line or a bulky text blob on the front. i'm just trying to understand your intended change.
restive ridge
Yes we annotate and comment patches because that's what modders are the most likely to see
Comments -> with moderation of course
Annotations -> hell yes they convey important info. MC assumes non-null by default
sand copper
got it. I know patches try to be minimalistic, so want to respect the balance of informative and simple.
lucid harbor
got it. I know patches try to be minimalistic, so want to respect the balance o...
The only consideration for patches is minimizing the number of hunks, within reason
once you have a hunk the size of that hunk is mostly irrelevant
sand copper
javadocs the size of novels here i come /j lol
sand copper
@lucid harbor regarding the two comments on the invulnerability check, are we good or does this need further discussion?
lucid harbor
I think its fine to re-fire invuln check, mods may invoke actuallyHurt from internal code paths and expect it to behave in a consistent manner
I also need to review how the damage container will handle if someone tries to perform a multi-typed attack (which generally involves an external invocation of actuallyHurt mid-process)
but I won't be able to do that for about a week
sand copper
In our previous discussion on this topic, part of clarifying the damage pipeline was ensuring users understand that actuallyHurt is what it says and actually hurts the entity. Having the invulnerability check here undermines that assertion. @quick kernel correct me if i'm wrong here, but this was the logic behind minimizing isInvulnerableTo calls.
If a modder invokes super.actuallyHurt as part of their own entity, they should be operating under the notion that their entity will be dealt damage with the one caveat that certain events may modify that value.
As for mult-source attacks. That was discussed in Tslat's gist on this topic (see pins) however, such a feature in the base system was going to be more of a rewrite than the other things that are currently in the PR. We essentially phase-two'd that part for a later PR.
quick kernel
In our previous discussion on this topic, part of clarifying the damage pipeline...
this is true, yeah
However the invuln check is meant to be a static check, so there shouldn't really be any harm in having it re-fire to be consistent with vanilla if people are using it proprly
I.E. it shouldn't be used for dynamic invulnerabilities anyway, so it should be functionally no different to it having been checked in hurt to begin with
if it's being called from an external source, thne it's breaking our expected chain anyway, and we'd be best off matching vanilla expectations in that circumstance
TLDR: Yeah keep the is invuln call in actuallyhurt per vanilla expectations, that makes sense
sand copper
I see that point, however, vanilla never calls actually hurt outside of the hurt method. Even overrides of actuallyHurt are only ever invoked via LivingEntity#hurt. Even vanilla expects that actuallyHurt is called within hurt.
Furthermore, the damage sequence relies on the instantiation, tracking, and invalidation of the damage container throughout the process. If we say that it's okay to call actuallyHurt without proxying the call through hurt, events could get passed a DamageContainer that is null or has null properties that it shouldn't. We could add null checks throughout the process to instantiate a new DamageContainer if one does not exist at every step, but that seems like a whole lot of bloat for an edge case that already violates the assumptions set by vanilla.
quick kernel
I may have misunderstood the question
I'm not saying we should add anything additional, I'm saying if we missed something that vanilla already expects we should be accounting for that
sand copper
@lucid harbor I have addressed the other comments from your review pending your response to this follow up response. If there are no further changes, this just needs final approval.
sand copper
i don't touch resourcelocations lol so that's good
lucid harbor
@sand copper General thing - I think it would make sense if the children of DamageSequenceEvent had consistent naming
The current spread is DamageBlockEvent / DamageTakenEvent / EntityPreDamageEvent / IncomingDamageEvent, which is hard to connect without knowing the class heirarchy
I'm inclined to say this is a set of events where using the innerclass pattern is most viable. We have four closely-related events with little to no differences in parameters that are part of a single sequence.
Given that, I would propose the following names:
DamageSequenceEvent.PreDamageSequenceEvent.BlockingDamageSequenceEvent.ApplyDamageSequenceEvent.Post(sidenote: I think it makes sense to move this event to fire at the same point asonDamageTakeninstead of before health changes)
But it sort of depends how we want to relate them; Obviously "Pre" is the start process (in both the current and proposed naming schemes), though a "Pre" event with no corresponding "Post" event goes against the general convention
lucid harbor
Well right now we have four disconnected events
quick kernel
The fact that you had to append an explanation of what the events meant is demonstration that they immediately undo a significant portion of this pr
lucid harbor
that isn't great either
The fact that you had to append an explanation of what the events meant
what
quick kernel
I mean the choice to do some kind of superclass wasnt my idea, but I was very specific in pointing out that the basis of the naming redesign was to make it immediately apparent what the event was for by name alone
Cus one of the biggest issues currently is misuse
lucid harbor
I don't really think the pr-current name set accomplishes that goal
Pre, Taken and Incoming are ambiguous to a degree without looking into the details
The benefit of providing them via innerclass means the context of the others is immediately apparent if you look at any one of them
quick kernel
So the context is only apparent if you look at them as a whole
Which people don’t do
lucid harbor
Innerclass forces you to do that
quick kernel
No it doesn’t
Whatever I’m too disappointed to even argue this
I hate this now, thanks
verbal lintel
Innerclass *forces* you to do that
Based on? An inner class is just a name with another class name before it when you ctrl space
lucid harbor
When you open the class file the others are immediately visible in the surrounding context
The best you can achieve for unique files is linking to all the others from each file, which still requires the user to follow the link
restive ridge
From an inner class, the event order is also immediately visible
sand copper
<@179625893822464000> General thing - I think it would make sense if the childre...
I can see value in nesting them as inner classes, however I don't like those names. like @quick kernel said, the main goal of the name changes was to make it clearer what they do. The current names require no need for the user to understand that there is a sequence, but does convey a bare minimum of distinguishing information. for example
EntityPreDamageEventconveys that this even fires before damage. ✅ primary goal acheivedIncomingDamageEventdamage is incoming and will happen next ✅DamageTakenEventpast tense so we know the damage was already applied ✅DamageBlockEventan event capturing the blocking of damage ✅
The sequence position really doesn't matter unless you are diving deep into this sequence at which point the inheritence lookup ofDamateSequenceEventbecomes relevant as does the detail in the child class javadocs. Tslat's original point with the name changes was that they don't do what people think they do and the average user stops at surface level and uses the first thing they find. A sub-class structure won't necessarily change that. that said, we are using sub-classed events more in NF and the error throwing of abstract classes does mean that users will have to at least have some familiarity with the child classes. So if we would prefer users have some deeper understanding of the sequence, then sub-classing makes sense. Just keep the names as is.
restive ridge
It's unclear to me whether incoming is before or after pre, for example
DamageBlock implies that I can block damage. Is it meant to be DamageBlocked?
sand copper
DamageBlockEvent exposes the actual blocking action itself, the amount of damage blocked, and the durability taken from the shield in one event. Making it past tense would obscure the fact that the blocking action itself can be prevented by this event as well
quick kernel
Fundamentally I feel like the people who maintain these apis end up disconnected from the average person using them
I interact with both new and experienced devs on a daily basis and the naming has always been a significant issue
Half the point was fixing the naming to rectify that - because users don’t go further than surface level because they don’t believe they need to
On top of that, the order of these events isnt even really relevant
Since their goals are different
Not just different phases of the same thing
sand copper
that's why i think a hybrid solution works. sub-classes for code conformity with other abstract events so there is a clear connection to those who do dive deep, but keep the names the same for the end user clarity
restive ridge
`DamageBlockEvent` exposes the actual blocking action itself, the amount of dama...
Ah ok fair enough
sand copper
I have updated the Javadoc and formatting change requests. So looks like we just need a final decision on this naming convention and a resolution to the comment about passing in invulnerability from vanilla into the event.
quick kernel
I'm gonna flip if the names get fucked over by generic bullshit like .Pre
these events are not a sequence, they serve functionally different purposes and we know for a fact that their naming is treated at face value and actively causes issues
lucid harbor
They are explicitly a sequence though
The container is created at the start and all of them are fired in order
quick kernel
yes but they don't matter as a sequence
the container is just there for accessibility
it doesn't matter which one happens first, because you do'nt use them for the sake of order
you use them for the sake of what they do
the thing is, moving to that completely undoes a significant point of this PR
and I feel like any argument made to try and say that those names are self explanatory just shows how disconnected one is from the average modder
lucid harbor
The pr-current names are not substantially more descriptive than the original name set, with the exception of EntityPreDamageEvent (though that one is still ambiguous with "incoming")
quick kernel
I wholeheartedly disagree
but if you want to propose alternate names, feel free
.Pre, .Apply, etc is not better
it's significantly worse
lucid harbor
DamageSequenceEvent.Pre is equally as descriptive as EntityPreDamageEvent
.Apply isn't great
quick kernel
you can't have .Pre without all the others changing
so all the others have to be equally or more descriptive than current naming
lucid harbor
By our own conventions a Pre should be linked with a Post - granted, we have a post event (DamageTakenEvent) but the naming is inconsistent. That should be rectified to EntityPostDamageEvent if we want to use pre-
IncomingDamageEvent is ambiguous at face value; this is fine enough, the prior one (LivingDamageEvent) was as well. Alternatives to this name don't really accomplish anything.
DamageBlockEvent is less descriptive than the prior ShieldBlockEvent, but received a scope expansion. For consistency, EntityBlockDamageEvent is more appropriate.
As a footnote, since these strictly apply to LivingEntity, it should be LivingXYZ instead of EntityXYZ
Applying all of that would yield
LivingPreDamageEvent - immediately obvious as the first event triggered when damage occurs
LivingBlockDamageEvent - Name conveys that a living entity is attempting to block incoming damage by some means
LivingIncomingDamageEvent - Name conveys that damage is incoming to a living entity. This is ambiguous in a literal sense, but not confusable with the others as was the prior case
LivingPostDamageEvent - Conveys that the happens after damage occurs within the pre/post name scheme that modders are familiar with
sand copper
my only nitpick is that Post feels like it happens after the damage is applied so I would expect calling entity.getHealth() to have the after-damage value
lucid harbor
I would suggest it fires after health has been changed
The current one doesn't
Though the new hook on Entity does
I would put the event hook directly after this.onDamageTaken(this.damageContainer);
If necessary we could record the pre-application value of getHealth() and provide it as event context
quick kernel
yeah this fails the point of the name change in the first place
EntityPostDamageEvent
as does this
LivingPreDamageEvent
quick kernel
because you're fundamentally misunderstanding the point of the renames
I'm focussed on actual users
actually using the API
the whole reason I pushed this so hard is because this has been an ongoing and consistent issue for years in the modding community
users do not give a shit about event order
they care about what event they need to use
that's why LivingHurtEvent gets used for everything
cus they see LivingHurtEvent, and go 'yep, that's what I want'
they couldn't care less it's run before LivingDamageEvent or after LivingAttackEvent
you are disconnected from the average modder
and more focussed on the API implementation of the thing as opposed to the people using it
which is disappointing, but not surprising
lucid harbor
I'm totally lost here
you don't agree with the current name for the first event either?
quick kernel
what?
lucid harbor
The event, in the PR, is ALREADY called EntityPreDamageEvent
quick kernel
right
lucid harbor
but you just said that "fails the point"
quick kernel
Pre & Post
that being said, yes I'm open to Pre being chanegd to a more suitable name if the ordering inferrance of the word is a problem for you
I'm happy with LivingBlockDamageEvent as a change though
but the idea was to remove the Living bit for consistency
hence the change to DamageBlockEvent since BlockDamageEvent felt.. weird
sand copper
yeah, the Living change makes sense
lucid harbor
The general majority of events that descend from LivingEvent are named LivingXYZEvent
The ones that descend from EntityEvent are EntityXYZEvent
quick kernel
yeah I get that
but if we're going to change one all would need to change (besides invuln)
quick kernel
Applying all of that would yield
`LivingPreDamageEvent` - immediately obvious a...
so this is fine as a base, but I'm not at all happy with moving to Post
If your issue is Pre, then I'd rather change Pre to something else
lucid harbor
Hm, its just fired after the pre-checks from hurt succeed
quick kernel
what if we swap incomingdamage
lucid harbor
Which can be called in any manner of scenarios
lucid harbor
Maybe?
The actual two events that are "atomic" are the last two
i.e. they always fire in tandem
since they're in the same method (actuallyHurt vs hurt for the others)
quick kernel
I think my slight issue with that
is that the name doesn't really describe that the damage is being taken
although it's kinda the same mod place for both
so that doesn't really matter
lucid harbor
You could have the last two be LivingDamageEvent.Pre/Post
quick kernel
no thanks
lucid harbor
and figure out some new names for the earlier ones in hurt
Because the ones in actuallyHurt are where "damage" occurs
quick kernel
EntityInvulnerabilityCheckEvent
LivingIncomingDamageEvent
LivingBlockDamageEvent
LivingModifyDamageEvent
LivingDamageTakenEvent
possible change?
lucid harbor
The stuff in hurt is related to being "attacked" but not "damaged"
And if an attack is successful, it proceeds to inflict damage (firing the two Damage*) events and calling onDamageTaken
lucid harbor
LivingIncomingAttackEvent + LivingBlockAttackEvent do seemingly work well though
lucid harbor
Granted you could shorten the former to LivingAttackEvent but that's the old name and 
quick kernel
the second one I don't like because it implies you're only blocking the attack, as opposed to modifying it
blockdamage is more than just blocking an attack
it's adjusting the damage that slips through and stuff too
lucid harbor
That would just be a partial block of the attack
quick kernel
you mean a damage reduction
plus all being named damage is conveniently consistent for searching
quick kernel
`EntityInvulnerabilityCheckEvent`
`LivingIncomingDamageEvent`
`LivingBlockDamage...
I'm happy with this
lucid harbor
you mean a _damage_ reduction
I would argue that you would want to reserve that for the damage modification event :p
I think that's why ShieldBlock being succinct about its purpose worked well
quick kernel
yeah but part of this PR expands that event's utility
lucid harbor
But its moreso that one applies before armor is calculated
quick kernel
to include any kind of block reduction
not just a shield
forcefield effect?
mana barrier?
Etc
lucid harbor
But couldn't you just block those in the first event?
Does it provide any meaningful utility to do it in the block event versus the first one?
They fire almost back-to-back
and by almost I think they literally fire in tandem
proper shadow
I agree with this
The shield block event makes the best sense as simply opening up the vanilla blocking to mod intervention. Id almost consider it unrelated to the sequence of damage events to make that more clear
lucid harbor
And a (potentially unintended) side effect is that messing with the blocking logic causes this.blockUsingShield(livingentity); to be called if any "block"-like action occurs, not just a shield thing
quick kernel
I do'nt have time to dig into the relevant code atm, but if we need a case handler for that then we can add one to the event