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

API: skillMaxUpgradeCount returns 1 when SP is too small and current level is too high #1693

Open
catloversg opened this issue Oct 15, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@catloversg
Copy link
Contributor

#1475 added skillMaxUpgradeCount formula API.

When the skill point (SP) is not enough to upgrade the skill, this API should return 0. However, due to floating-point imprecision, when SP is too small and the current level is too high, this API may return 1. For example:

ns.print(ns.formulas.bladeburner.skillMaxUpgradeCount("Hyperdrive", 1e50, 1)); // Print 1

This is how we are implementing it:

const result = Math.round((m + delta) / this.costInc);
const costOfResultPlus1 = this.calculateCost(currentLevel, (result + 1) as PositiveInteger);
if (costOfResultPlus1 <= cost) {
  return result + 1;
}
const costOfResult = this.calculateCost(currentLevel, result as PositiveInteger);
if (costOfResult <= cost) {
  return result;
}
return result - 1;

result is 0, cost is 1, and costOfResultPlus1 is 0, so it returns 1. In this case, the current level is too high (1e50), so increasing it by 1 (result + 1) won't do anything (costOfResultPlus1 = 0) (if the player calls ns.bladeburner.upgradeSkill with count = 1, we will notify them that it's impossible to do that).

At first glance, I think that this behavior is (somewhat ?) reasonable. However, some players may be confused: "Why does it return 1 when it's obvious that I won't be able to upgrade the skill with that amount of SP?".

@d0sboots What do you think about this problem? It only matters when the player is farming int and they don't know the pitfall of floating-point imprecision (A(big) + B(small) may be exactly A in some cases), so I don't think that it's a big deal.

@d0sboots
Copy link
Collaborator

I think there's an actual problem here. For the right large values, I think the "result" calculation in skillMaxUpgradeCount can return a single number that is some power-of-2 due to rounding in the fundamental calculation. However, this result will be too big, and then the +-1 won't be enough to correct that, so the final result ends up being too big and fails if you try to pass it through on the other side.

Instead of using +-1, it needs to be using increments that are it minimum 1, but scale with the precision of the calculation. Ugh.

@Alpheus
Copy link
Contributor

Alpheus commented Oct 23, 2024

Isn't max level on all skills 1e18?
Nevermind, this is dev, I was looking at 2.6.2.

I'm adding draft PR to test cover the bugged behavior with some additional cases.

Alpheus added a commit to Alpheus/bitburner-src that referenced this issue Oct 23, 2024
…eCount returns 1 when SP is too small and current level is too high
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants