Apotheosis

Apotheosis

70M Downloads

Enchanting stats are calculated inaccurately

dwentz89 opened this issue ยท 2 comments

commented

I'm running forge-1.16.5-36.2.20-universal.jar, Apotheosis-1.16.5-4.8.4.jar, Placebo-1.16.5-4.6.1.jar, and no other relevant mods.

I surrounded an enchanting table with 15 seashelves enchanted with sea infusion III, so they each give 1.8 eterna. That should give a total of 1.8*15=27 eterna and the 3rd enchantment slot should require level 54, but the table actually showed 26.99 eterna and level 53.

With a separate enchanting table, I placed 4 endshelves. Endshelves give 3.5% quanta and there's a base quanta of 10%, so that should result in 24%, but the table actually displayed 23.9%.

It looks like you're storing these values with float variables. Floating point numbers are bad at precision, especially if you're rounding down or comparing against exact values with no room for imprecision. I don't have an easy way to demonstrate this, but if you've got enough shelves for 75% arcana but the code only counted 74.9999%, it won't give the 3rd guaranteed enchantment for reaching 75%.

I don't see anything you're doing with these that actually requires floating point numbers, so I think it would be much better if you stored these values using shorts or ints. Eterna's variable would range from 0 to 5000 instead of 0.00 to 50.00 (so 12.34 would be stored as 1234) and quanta and arcana's variables would range from 0 to 10000 instead of 0.000 to 10.000.

An alternative fix would be to change them to double instead of float, and also add Math.round() calls before you use the values.

commented

Vanilla's implementation defines enchanting power as a floating point value, so using floats is a requirement. I'm aware of their limitations and shortcomings.

This is technically a non-issue because the way that these are stored has been changed (the space has been changed from [0,10] to [0, 100] for the percent-based fields) in 1.18.

The Eterna to level computation is done via cast because that's just what vanilla does. Could probably change that to a round with no issues.

commented

I'm not convinced that you have to use floats everywhere just because vanilla does. You could convert the float into something else before adding it to your total, and then convert the total back if you need a float again later for some reason. And all the new stats that weren't in vanilla could've just been integers in the first place. Though I guess there's no compelling reason to change it now that the issue is invisible.

Changing the range to [0, 100] helps but does not fix it. I looked into your 1.18 changes though, and it looks like you also stopped using decimal values like .8 that can't be represented exactly as a float, which does "fix" it. You just have to remember not to start using those values again.

When I said to use Math.round, I meant something more like Math.round(eterna * 2 * 100) / 100 so that it's rounded to the 2 decimal places that display in the UI. Your Math.round(eterna * 2) means that 9.75 eterna allows a level 20 enchant instead of the level 19 one that it used to allow.