Calculation of spell book requirement seems extremely strange

[thebigMuh]

Summary The calculation for how much Magic it takes to learn a spell book seems exceedingly strange. I tried to replicate the behavior and simplify it for my own use, and the results were different, which is why I noticed this.

Describe Each spell has a base Magic requirement (sMinInt property). For each spell level beyond level 0, it takes 20% additional Magic to learn the spell. So e.g. Chain Lightning has sMinInt 54. To learn level 1, it takes floor(sMinInt 1.2), so 64. Level 2 takes floor(64 1.2) or 76. And so on. The result is clamped to 255.

The expectation would be that this results in Chain Lightning level 1 and up requiring the following Magic values: 64, 76, 91, 109, 130, 156, 187, 224, 255, 255, ...

The calculation in game happens in a very strangely constructed loop, that has an exit condition which checks if the NEXT spell level to learn requires Magic >= 255, and if it does it sets the Magic required to 255 and exits the loop.

This results in Chain Lightning having the following Magic requirements: 64, 76, 91, 109, 130, 156, 187, 255, 255, 255, ...

Essentially, all Magic requirements above 212 get raised up to 255.

This is the code used, I simplified it for readability:

book._iMinMag = spelldata[spellId].sMinInt;
slvl = player._pSplLvl[spellId];
while (slvl != 0) {
    book._iMinMag += 20 * book._iMinMag / 100;
    slvl--;
    if (book._iMinMag + 20 * book._iMinMag / 100 > 255) {
        book._iMinMag = 255;
        slvl = 0;
    }
}

On the one hand, this seems like a bug introduced by someone trying overly hard to avoid an overflow error, since the _iMinMag property that stores the Magic requirement of an item is only a Uint8.

On the other hand, the exact same code exists in three functions; very likely copypasta:

• CheckBookLevel in inv.cpp
• CalcPlrBookVals in items.cpp
• WitchBookLevel in items.cpp

Conclusion I have no conclusion. Should this be fixed or not? It smells very much like a bug, but then again the only characters that would really be affected by this are high level Sorcerers, and those reach max. Magic quickly anyway.

The simplified version of the code above that I used, which produces the different result, is somewhat like this:

int magNeeded = spelldata[spellId].sMinInt;
slvl = player._pSplLvl[spellId];
while (slvl --> 0 && magNeeded < 255) {
    magNeeded += 20 * magNeeded / 100;
}
book._iMinMag = std::min(minMag, 255);

[NikoVP]

I think it should be changed. It does not alter gameplay to high extent. Mage class easily can exceed the max magic requirements, while the rest of the classes get at most 1 more spell level. Still the topic should be debated first.

[julealgon]

has an exit condition which checks if the NEXT spell level to learn requires Magic >= 255, and if it does it sets the Magic required to 255 and exits the loop.

Seems like a bug to me, a failed attempt at limiting the maximum value in the wrong iteration.

[julealgon]

Also, the formula seems to describe a geometric progression with factor 1.2, so we should probably simplify the formula to the formula for the nth element of a geometric progression with 1st element X, capped at 255. I don't see a need for loops here.

[Chance4us]

I have no conclusion. Should this be fixed or not? It smells very much like a bug, but then again the only characters that would > really be affected by this are high level Sorcerers, and those reach max. Magic quickly anyway.

Did you compare the behavior in vanilla?

[thebigMuh]

Did you compare the behavior in vanilla?

This behavior always existed, also in vanilla. See e.g. Jarulf's Guide (http://www.lurkerlounge.com/diablo/jarulf/jarulf162.pdf), page 72 at the bottom:

Note that if the magic requirement is 213 or higher, it is always adjusted to 255.

[thebigMuh]

Also, the formula seems to describe a geometric progression with factor 1.2, so we should probably simplify the formula to the formula for the nth element of a geometric progression with 1st element X, capped at 255. I don't see a need for loops here.

It is basically a geometric progression / compound interest / whatever series. They likely used a loop because they are using integer maths and didn't want to have a pow in there. Also, just changing it now would influence the result quite a lot:

If the formula was changed to

magNeeded = std::pow(1.2, spellLevel) * sMinInt;

Then instead of 54, 64, 76, 91, 109, 130, 156, 187, 224, 255 you would get (truncating to int) 54, 64, 77, 93, 111, 134, 161, 193, 232, 255

So to keep stylistically in line with the rest of the code, and keep the results mostly identical, I would stay with the loop approach and just fix the exit condition.

[julealgon]

If the formula was changed to

magNeeded = std::pow(1.2, spellLevel) * sMinInt;

Then instead of 54, 64, 76, 91, 109, 130, 156, 187, 224, 255 you would get (truncating to int) 54, 64, 77, 93, 111, 134, 161, 193, 232, 255

So to keep stylistically in line with the rest of the code, and keep the results mostly identical, I would stay with the loop approach and just fix the exit condition.

I'm totally ok with that change in the numbers. They would cause extremely minor impact (near zero).

To me its more important we model the concept properly (20% "interest rate") as succintly as possible and allow it to be extended later. Getting rid of the loop in favor of the formula would dramatically simplify the code and would model the intent much more clearly.

[AJenbo]

I would prefer not to change the number, they are well defined and documented in multiple guies. With a near-zero impact, I would prefer not to have to explain that to the user and instead have them see zero change.

[julealgon]

I would prefer not to have to explain that to the user and instead have them see zero change.

That's what a changelog would do. No need to explain, just direct them to the changelog and explain that we improved the precision of requirements calculation.

[FitzRoyX]

Doesnt seem all that different from the repair formula bug that got fixed

[AJenbo]

Yeah, but that isn't well defined. https://diablo2.diablowiki.net/D1_Spellbooks#Magic_Requirements_for_Spellbooks