Add Critical hit API

[dawon]

See #12318

[Doc94]

Hi, hmm currently by the event you can only prevent the hit but cannot know if already was "prevented" by the config not?

[dawon]

Oh I could default the value based on the config option, good idea!

[Owen1212055]

I would rather have this as a separate event imo. Tying this to this event means that if this logic moves it'll make this setter painful to upkeep.

[Leguan16]

I think I unfortunately have to agree with Owen here.

[dawon]

I've updated the PR so now there is a separate event, that you can use to cancel or enforce a critical hit as well as change the crit modifier...

[notTamion]

i think a better name would be something like PlayerAttackDamageEvent considering this event is called for all hits and not just critical ones

[dawon]

i think a better name would be something like PlayerAttackDamageEvent considering this event is called for all hits and not just critical ones

Well, but the only purpose of this event is to manage "criticallity" of hits... It can make non-critical hit critical and vice versa... I think therefore Critical should be part of the name...

[notTamion]

why not expand on the event then? considering afaik we won't a nice way to modfiy e.g. the base damage of a specific attack. i just don't think we should call the event CriticalAttackEvent or similar because that is not what it is. it gets called when the damage gets calculated, so why should we restrict ourselves to have it only handle the criticality when it could expose more than just that.

[Owen1212055]

why not expand on the event then? considering afaik we won't a nice way to modfiy e.g. the base damage of a specific attack. i just don't think we should call the event CriticalAttackEvent or similar because that is not what it is. it gets called when the damage gets calculated, so why should we restrict ourselves to have it only handle the criticality when it could expose more than just that.

See the reasoning above. I think having a separate event is good here because damage logic should not be consolidated like the current damage event. It should only control criticality in my opinion.

[notTamion]

See the reasoning above. I think having a separate event is good here because damage logic should not be consolidated like the current damage event. It should only control criticality in my opinion.

i am not saying we should'nt make this a seperate event (with "the event" i meant the event we are going to be adding in this PR), i am saying this event could expose more than just criticality considering the event is placed right around where this stuff is calculated and passed to the hurt methods which then call the damage events. we would'nt be consolidating/forcing this stuff into that event via trickery at all because the event is already placed nicely for that. either way i still don't think PlayerCriticalHitEntityEvent is the best name for this.

[dawon]

I've collected some name suggestions from discord not sure which one to use, but maybe the original Tamion's one (PlayerHitCriticalityEvent) is probably the best. Any other suggestions/ideas?

PlayerHitCriticalityEvent
CriticalHitEvent
PlayerCriticalHitDetermineEvent
PlayerDetermineCriticalHitEvent
PlayerCriticalHitDecisionEvent
PlayerCritDecisionEvent

[lynxplay]

So, another bit that came up in some related discussion is that this concept also applies to e.g. sweeping edge attacks.
They also find themselves with similar "computation" logic in the attack bit.

Might be something to keep in mind when deciding between event naming, event layout etc. Not that I have much time for that rn, but for future reviews on this PR, the concept ought to be considered.

[dawon]

So, another bit that came up in some related discussion is that this concept also applies to e.g. sweeping edge attacks. They also find themselves with similar "computation" logic in the attack bit.

Might be something to keep in mind when deciding between event naming, event layout etc. Not that I have much time for that rn, but for future reviews on this PR, the concept ought to be considered.

Well I am not sure how this can be aligned other than naming the event specific enough for crits. It would be nice to have crits and sweaping under one event, but I don't think that can be done easily. Or you had some solution in mind to unify those two under one event?

[lynxplay]

Heh I didn't have a solution in mind no.
Yea we can either look to unify these, making it potentially harder if they split, or we maybe look for a name that like, can just replace "Critical" with "Sweeping" or something and then they can be semi sister events. But yea, as said, for future people to just keep in mind when they review it.

I'd hate to name it something to specific to crit and then the sweeping edge one comming 3 commits later is named completely differently even tho they sit in the same logic bit.

[dawon]

Ah. got you! I will keep that in mind!

[dawon]

So how about:

PlayerResolveCriticalHitEvent or PlayerResolveCriticalAttackEvent
and potentially in the future PlayerResolveSweepAttackEvent

?

[Maxsh001]

So how about:

PlayerResolveCriticalHitEvent or PlayerResolveCriticalAttackEvent and potentially in the future PlayerResolveSweepAttackEvent

?

Actually I think this should be one event, since you never crit & sweep at the same time. (There are 2 different sound effects for crits & sweeps specifically) Maybe just PlayerAttackEntityEvent and you can modify the "attack type"

[dawon]

So how about:

PlayerResolveCriticalHitEvent or PlayerResolveCriticalAttackEvent and potentially in the future PlayerResolveSweepAttackEvent

?

Actually I think this should be one event, since you never crit & sweep at the same time. (There are 2 different sound effects for crits & sweeps specifically) Maybe just PlayerAttackEntityEvent and you can modify the "attack type"

It would be cool, but those two mechanics are at two a bit different places so either both of the apis would be very limited or it needs to be split in two events.

[Maxsh001]

So how about:
PlayerResolveCriticalHitEvent or PlayerResolveCriticalAttackEvent and potentially in the future PlayerResolveSweepAttackEvent
?

Actually I think this should be one event, since you never crit & sweep at the same time. (There are 2 different sound effects for crits & sweeps specifically) Maybe just PlayerAttackEntityEvent and you can modify the "attack type"

It would be cool, but those two mechanics are at two a bit different places so either both of the apis would be very limited or it needs to be split in two events.

I made this. I would say it's way better

[dawon]

It looks better, if it works, I will only be glad. Let's wait for the team.

So how about:
PlayerResolveCriticalHitEvent or PlayerResolveCriticalAttackEvent and potentially in the future PlayerResolveSweepAttackEvent
?

Actually I think this should be one event, since you never crit & sweep at the same time. (There are 2 different sound effects for crits & sweeps specifically) Maybe just PlayerAttackEntityEvent and you can modify the "attack type"

It would be cool, but those two mechanics are at two a bit different places so either both of the apis would be very limited or it needs to be split in two events.

I made this. I would say it's way better

[dawon]

Closing in favor of #12388.