Rewrote GetManaAmount to result in the same decompiled code as Diablo

[seritools]

Fixed the type of PlayerStruct->pClass while being at it. Thank you @galaxyhaxz for all the help getting into it :)

[mewmew]

Hi @seritools! Looks great. Lovely to see that you added the original names also for the local variables 😈

I can confirm that the decompiled code (as output by IDA Hex-Rays) is indeed identical. The assembly is not exactly identical (e.g. different registers used, see below), but that's fine for now.

I do believe that the original source code did not use shift operations for multiplication and division (e.g. << 6 instead of * 64). Rather, I would believe that the compiler optimized the division and multiplication expressions to shift operations using strength reduction. Figuring out the exact set of compiler flags is tracked by #111.

So, we can merge this PR as is now, and then once we figure out the right compiler flags, we can update the code to use division and multiplication instead.

+1 to merge from me. @galaxyhaxz, feel free to merge if you second.

-       if (spelldata[sn].sMinMana > ma >> 6 )
-               ma = spelldata[sn].sMinMana << 6;
+       if (spelldata[sn].sMinMana > ma / 64 )
+               ma = spelldata[sn].sMinMana * 64;

Original Diablo.exe:

.text:0045744E                 mov     eax, ecx
.text:00457450                 push    esi
.text:00457451                 imul    eax, 54D8h
.text:00457457                 push    edi

Devilution.exe:

.text:0045B36A                 mov     eax, ecx
.text:0045B36C                 push    ebx
.text:0045B36D                 imul    eax, 54D8h
.text:0045B373                 push    ebp
.text:0045B374                 push    esi

[seritools]

Yeah, the optimizer does some interesting things. I've tried changing

if ( plr[id]._pClass == 1 )
		ma -= ma >> 2;

for example, but that resulted in slightly different code.

I just realized that _pClass probably is 32bit and not an unsigned char like in the PR. I found similar code in the other functions. What happens is probably the following:

• _pClass is of an enum type with less than 256 enum values.
• enums are usually 32bit
• knowing that all enum values fit into the lowest byte, the compiler only checks that byte

Basically, these two seem to be equal for such enums:

BOOL foo(player_class pClass) { return pClass = PCLASS_SORC; }
BOOL foo(int pClass) { return (_BYTE)pClass = PCLASS_SORC; }

I found the same to be true for spell_type in the CheckSpell function (tracking my progress over at https://github.com/seritools/devilution/tree/spells-cleanup ).

I'm gonna recheck that in this case. :)

[seritools]

Alright, i just checked the case of _pClass:

Things that speak for it being originally something like enum _ui_classes _pClass;:

• There are three bytes of padding because of the alignment, so could be optimized away for small small enums like this
• Other enum values seem to work similarly
• Speak against it: With our current optimization settings, the compiler generates dword accesses instead of byte accesses, and specifying enum _ui_classes : unsigned char is only possible as of C++11 :(

Things that speak for it being originally something like char _pClass;:

• The PSX used unsigned char char as well (EDIT: thx galaxyhaxz)
• The accesses in both 1.09 and the beta use byte accessing
• unsigned char generates the correct accessing code

So I guess everything is fine leaving it unsigned char now. :)

[mewmew]

LGTM. Great first PR @seritools!

[ghost]

Sorry about the wait, I've been really busy dealing with typical real-world stuff and haven't been around for a few days. The commit looks good, the only thing that needs to be changed is:

00ab67: $00000118 94 Def class MOS type CHAR size 0 name _pClass

_pClass should be a char and not unsigned char. Not that it makes a difference since it never goes past 128. I'll fix it later.

[mewmew]

Sorry about the wait, I've been really busy dealing with typical real-world stuff and haven't been around for a few days. The commit looks good, the only thing that needs to be changed is:

Don't worry. If anything, you've earned some time off! And, being a hobby project you should invest time when you feel like and balance it with other things in life. I think anyone in the community would understand.

To better distribute the work, I'd recommend inviting @seritools as a collaborator on the project too :)