Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

GotoFinal
Copy link
Contributor

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.

experience -= expNeeded;
startLevel +=1;
return getLevelForExpWithLeftover(experience, startLevel, stopLevel);
public static Vector2i getLevelForExpWithLeftover(long experience, int startLevel, int stopLevel) {
Copy link
Contributor Author

@GotoFinal GotoFinal Jul 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Rover656 added this to the "More Features" (v6.1) milestone Jul 24, 2023
Copy link
Member

@Rover656 Rover656 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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 Rover656 added Type-Bug There is a problem. Area-Backend Backend work not usually visible to players. MC-1.20.1 labels Sep 5, 2023
@Rover656 Rover656 mentioned this pull request Nov 11, 2023
5 tasks
@Rover656
Copy link
Member

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!

@Rover656 Rover656 closed this Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Backend Backend work not usually visible to players. MC-1.20.1 Type-Bug There is a problem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] StackOverflowError when shift+click on fluid tank.
2 participants