Sneak Attack Hit Rate and Crit Rate Fail when facing East-ish

[Hokuten85]

I have:

• searched existing issues (http://github.com/darkstarproject/darkstar/issues/) to see if the issue I am posting has already been addressed or opened by another contributor
• checked the commit log to see if my issue has been resolved since my server was last updated

Client Version (type /ver in game) :

Source Branch (master/stable) :

Additional Information (Steps to reproduce/Expected behavior) :
When determining a successful sneak attack it compares the Attacker's rotation vs Mobs rotation. This is problematic when facing East-ish. Dead East is rotation 0. Facing slightly SE is rotation 1. Facing slightly NE is rotation 255.

The rotation comparison below is what determines success.
abs(PDefender->loc.p.rotation - PAttacker->loc.p.rotation) < 23

This doesn't account for that scenarios where the mob and player are rotating toward different quadrants right around 0. This is used in both GetCritHitRate and GetHitRateEX within battleutils.cpp

[SirGouki]

[REDACTED] I'm checking the code and will edit this with what I find.

Please replace the code inside the else if
with:

uint8 playerRotation = PAttacker->loc.p.rotation;
uint8 mobRotation = PDefender->loc.p.rotation;

    if (mobRotation > (232)) //255-23
    {
        if (playerRotation > 0)
        {
            if (abs(PDefender->loc.p.rotation - (PAttacker->loc.p.rotation + 256)) < 23 || PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
            {
                crithitrate = 100;
            }
        }
        else
        {
            if (abs(PDefender->loc.p.rotation - PAttacker->loc.p.rotation) < 23 || PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
            {
                crithitrate = 100;
            }
        }
    }
    else if (mobRotation < 23)
    {
        if (playerRotation > (232))
        {
            if (abs((PDefender->loc.p.rotation+256) - PAttacker->loc.p.rotation) < 23 || PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
            {
                crithitrate = 100;
            }
        }
        else
        {
            if (abs(PDefender->loc.p.rotation - PAttacker->loc.p.rotation) < 23 || PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
            {
                crithitrate = 100;
            }
        }
    }
    else
    {
        if (abs(PDefender->loc.p.rotation - PAttacker->loc.p.rotation) < 23 || PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
        {
            crithitrate = 100;
        }
    }

and see if that fixes you. I just threw it together real quick, so if it's a "messy" fix I apologize, this is the best way I could see to fix it.

Edit2: Didn't realize this till much later but this is indeed a value going from 0 - 255. (loc.p.rotation is a uint8 which is why I used that for my variables.)

Edit 3: Seems git didn't get any of my later edits, this was to fix that, and remove references to 360 degree math.

[Hokuten85]

Need someone on retail to confirm the rotation range to see if its 0-360(or 0-2π) or 0-255.

This is what I added to my server. I was just hoping that someone had something more elegant.

if (abs(PDefender->loc.p.rotation - PAttacker->loc.p.rotation) < 23
                || (PDefender->loc.p.rotation < 64 && PAttacker->loc.p.rotation > 192 && abs(PAttacker->loc.p.rotation - 256 - PDefender->loc.p.rotation) < 23)
                || (PDefender->loc.p.rotation > 192 && PAttacker->loc.p.rotation < 64 && abs(PDefender->loc.p.rotation - 256 - PAttacker->loc.p.rotation) < 23)
                || PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
            {
                crithitrate = 100;
            }

Also, I think that the ASSASSIN+TRICK_ATTACK Crit Rate check shouldn't be an else if condition, but needs retail verification. I think if you have both SA and Assassin+TA on, only one of the conditions needs to be satisfied for a crit.

[TeoTwawki]

packet has an 8 bit value with different things in the byte son both sides of it, so unless we missed some bit-packing, pretty sure its 0-255 and not 1-360 or 0-359

[SirGouki]

Need someone on retail to confirm the rotation range to see if its 0-360(or 0-2π) or 0-255.

This is what I added to my server. I was just hoping that someone had something more elegant.

if (abs(PDefender->loc.p.rotation - PAttacker->loc.p.rotation) < 23
                || (PDefender->loc.p.rotation < 64 && PAttacker->loc.p.rotation > 192 && abs(PAttacker->loc.p.rotation - 256 - PDefender->loc.p.rotation) < 23)
                || (PDefender->loc.p.rotation > 192 && PAttacker->loc.p.rotation < 64 && abs(PDefender->loc.p.rotation - 256 - PAttacker->loc.p.rotation) < 23)
                || PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
            {
                crithitrate = 100;
            }

Also, I think that the ASSASSIN+TRICK_ATTACK Crit Rate check shouldn't be an else if condition, but needs retail verification. I think if you have both SA and Assassin+TA on, only one of the conditions needs to be satisfied for a crit.

I already confirmed it this morning since I couldn't sleep. Retail does indeed send 1 byte for rotation, which would be a uint8.

Edit: check your math, your numbers are way off. Also, there's no point in switching around PDefender and PAttacker, thats partially why abs() is used. Finally, if you're gonna reference a value that many times, it's usually better to create a temp variable like I did in mine, it saves some time.

You could also save some time by doing the check for hide exclusive to everything else, since having hide just sets the critrate to 100, i'd do:

if(PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
{
    crithitrate = 100;
}
else
{
    //do the mess of checking rotations and such here.
}

this will actually speed up these checks in instances where hide is in effect, because it will skip every other check if hide is active. Your || chain will go through everything and check hide last.

[Hokuten85]

My math was dependent on which quadrant the attacker and defender are rotated toward. I'm reducing values greater than 192(quadrant 4) to be a negative angle from the 0 axis. This can be directly mathed with the positive value in quadrant 1 to come up with a sensical difference between the two. I think the order would matter with what I was doing.

abs(251 - 256 - 5) = 10
abs(5 - 256 - 251) = 502

I can probably do something like this if this would be better??? Would eliminate the swapping.

256 - abs(attacker - defender)

And pulling out the hide check might be a good idea, but I'm not sure. If we are going for efficiency, I'd suggest that people will rarely ever use Hide+SA. It happens, but in normal game play with party mechanics hide is just rarely used. We'd be moving a rarely used check to ALWAYS happen first.

[Hokuten85]

Sorry to be so chatty. This is what I ended up with.

uint8 playerRotation = PAttacker->loc.p.rotation;
uint8 mobRotation = PDefender->loc.p.rotation;

if (abs(mobRotation - playerRotation) < 23
    || (abs(mobRotation - playerRotation) >= 233 && 256 - abs(mobRotation - playerRotation) < 23)
    || PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
{
    crithitrate = 100;
}

if the difference between the rotations was greater than 233 (256 - 23), then we can assume this is math between quadrant 1 and 4... and we really only care about math that is within 23 of 0.

[SirGouki]

Sorry to be so chatty. This is what I ended up with.

uint8 playerRotation = PAttacker->loc.p.rotation;
uint8 mobRotation = PDefender->loc.p.rotation;

if (abs(mobRotation - playerRotation) < 23
    || (abs(mobRotation - playerRotation) >= 233 && 256 - abs(mobRotation - playerRotation) < 23)
    || PAttacker->StatusEffectContainer->HasStatusEffect(EFFECT_HIDE))
{
    crithitrate = 100;
}

if the difference between the rotations was greater than 233 (256 - 23), then we can assume this is math between quadrant 1 and 4... and we really only care about math that is within 23 of 0.

I'd still change this like I mentioned above, since Hide pretty much guarantees Sneak Attack will work, it shouldn't be dependent on check everything else first (or at all, unless its not active). But this looks a lot better than my version. I had an extra case that I now realize wasn't necessary (mob's facing 0+ and player is in the 200s) in my original post. If you don't want to if..else it then you can just move the check for hide to being the first check.