Improve xp calculation to avoid stack overflow and int overflow issues #329

2023-07-22T17:03:42Z

GotoFinal commented

2023-07-22 17:03:42 +00:00

(Migrated from github.com)

Description

Improved calculation of XP to avoid long loops and some overflow issues in more critical areas.

Closes #313

Checklist:

My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you).
I have performed a self-review of my own code.
I have commented my code in areas it may be challenging to understand.
I have made corresponding changes to the documentation.
My changes are ready for review from a contributor.

# Description Improved calculation of XP to avoid long loops and some overflow issues in more critical areas. Closes #313 # Checklist: - [x] My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you). - [x] I have performed a self-review of my own code. - [x] I have commented my code in areas it may be challenging to understand.  - [x] I have made corresponding changes to the documentation. - [x] My changes are ready for review from a contributor.

GotoFinal (Migrated from github.com) reviewed

2023-07-22 17:04:33 +00:00

src/main/java/com/enderio/base/common/util/ExperienceUtil.java

					
				@ -70,0 +22,4 @@

				        long xpNeededToStop = getExpFromLevel(stopLevel);

				        if (totalXp >= xpNeededToStop) {

				            int leftoverXP = (int) Math.min(totalXp - xpNeededToStop, Integer.MAX_VALUE);

				            return new Vector2i(stopLevel, leftoverXP);

GotoFinal (Migrated from github.com) commented

2023-07-22 17:04:33 +00:00

note: depending on needs it might be good idea to return here something that can handle bigger leftover values, but I don't think its an issue for now

Rover656 (Migrated from github.com) reviewed

2023-08-02 18:24:55 +00:00

Rover656 (Migrated from github.com) left a comment

Sorry; been rather busy and am only just getting to this PR.
I'm not really sure the changes to use long is worth it. The binary search fix for the actual calculation is definitely the right fix for the problem, but if we're going to truncate the results to integers everywhere, we might as well do that in the method and just return an int, as I don't really see a scenario where we need to handle such a significant amount of XP.

CC @agnor99 - feel free to disagree with me here :)

I'm not going to mark this as a Request changes, as my opinion may not be the best way forward :)

Sorry; been rather busy and am only just getting to this PR. I'm not really sure the changes to use `long` is worth it. The binary search fix for the actual calculation is definitely the right fix for the problem, but if we're going to truncate the results to integers everywhere, we might as well do that in the method and just return an int, as I don't really see a scenario where we need to handle such a significant amount of XP. CC @agnor99 - feel free to disagree with me here :) I'm not going to mark this as a Request changes, as my opinion may not be the best way forward :)

GotoFinal commented

2023-08-02 19:35:46 +00:00

(Migrated from github.com)

@Rover656 Well best option would be to actually propagate this long to other places too, but didn't want to spend hours on touching half of the files in the project. Currently it still helps avoiding any overflows during calculations, as you still can change big amount of XP into small amount of levels and small leftover.

With all the crazy mods out there I don't think its really overkill. Ints cause a lot of issues in all of the mods because people just keep going crazy with their mods and constructions - especially power.
In this case overflows will start happen when dealing with levels above 21863, so sure, its quite a lot, but I don't think its out of modded minecraft reach. And with using longs the limit can be raised to 960383883 levels.

Speaking of what... I've noticed a small mistake in my code so pushing a small fix.

@Rover656 Well best option would be to actually propagate this long to other places too, but didn't want to spend hours on touching half of the files in the project. Currently it still helps avoiding any overflows during calculations, as you still can change big amount of XP into small amount of levels and small leftover. With all the crazy mods out there I don't think its really overkill. Ints cause a lot of issues in all of the mods because people just keep going crazy with their mods and constructions - especially power. In this case overflows will start happen when dealing with levels above 21863, so sure, its quite a lot, but I don't think its out of modded minecraft reach. And with using longs the limit can be raised to 960383883 levels. Speaking of what... I've noticed a small mistake in my code so pushing a small fix.

Rover656 commented

2023-11-15 21:42:39 +00:00

(Migrated from github.com)

Thanks for your contribution, however we've opted to design this in a different manner in #547 (feel free to review this PR if you have any feedback on this variant) - thanks for taking the time to help us investigate this issue however!