search

diasurgical / devilutionX

Diablo build for modern operating systems
Other
8.02k stars 786 forks source link

armor pierce conversion between vanilla and hellfire doesn't work at all #4053

Open qndel opened 2 years ago

qndel commented 2 years ago

Important information Compiled master - https://github.com/diasurgical/devilutionX/commit/90fbf1c535106826489e052ed976e2836c3bccbc

Describe armor pierce values aren't being converted between vanilla and hellfire at all

To Reproduce Generate any item with armor pierce in vanilla or hellfire Convert your character to hellfire/vanilla The values don't change. This is pretty serious imo as you can get an item with _iPLEnAc = 24 in vanilla (bashing affix), then it stays 24 after being converted to hellfire

    int CalculateArmorPierce(int monsterArmor, bool isMelee) const
    {
        int tmac = monsterArmor;
        if (_pIEnAc > 0) {
            if (gbIsHellfire) {
                int pIEnAc = _pIEnAc - 1;
                if (pIEnAc > 0)
                    tmac >>= pIEnAc;
                else

so it basically sets the armor of all enemies to 0

Expected behavior Values should convert between vanilla and hellfire

Additional context Easy to test with replacing showing armor pierce with

    case IPL_TARGAC:
        return _("penetrates target's armor") + fmt::format(" ({:d})", item._iPLEnAc);

to see the actual value

DakkJaniels commented 2 years ago

We could add a value to the item struct, and have it use that for the Diablo _iPLEnAc value. So when the affix is created, it saves two version of it, one for Hellfire (set at the param1 value) and one for Diablo (set at a random number between param1 and param2). When getting the item stats, it could then use whichever version is appropriate for the version of the game you are in. Then when saving and loading a character, check which version is being loaded, and load the appropriate version of the affix.

Or do nothing since the difference between _pIEnAC =4 and _pIEnAC = 24 isn't really that much (in a relative sense). Hellfire so OP. I guess to be fair, it's likely that any of these affix are going to be super overpowered if created in Diablo and not Hellfire. Just thought of a maybe better idea: We should know the affix name, so just check the affix name and determine the correct value to use that way. Probably much easier than doing what I originally said.

Edit again - maybe not, the suffix name isn't saved (except as a language dependent string), just the type of power it is. Maybe adding another variable is the way to go 🤷🏽‍♂️

StephenCWills commented 10 months ago

I believe this should already be fixed for MP in 1.5.1, due to #6258. We may be able to use a similar approach to fix this in SP.