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

[Bug] Fix off-by-one errors in some random number calls #3665

Merged
merged 14 commits into from
Sep 5, 2024

Conversation

DayKev
Copy link
Collaborator

@DayKev DayKev commented Aug 20, 2024

What are the changes the user will see?

Confusion will last 2-5 turns now instead of 2-4, trapping moves like Fire Spin will last 4-5 turns now instead of only 4, etc.
Basically, every move/effect that had a min-max turn range will actually last between the minimum and maximum amount of turns now instead of between min and max - 1.
Also fixes a bug where a move with 99 accuracy couldn't miss, as if it had 100 accuracy.

Why am I making these changes?

There was some usage of the random number functions that was incorrect.

What are the changes from a developer perspective?

Some instances of randSeedInt(range, min) are replaced with randSeedIntRange(min, max) when they were intended to be random between min and max inclusive and min was a number greater than zero.
Some tsdocs are added to the various randSeedInt() functions as well.

How to test the changes?

Liberal use of the browser devtools console and/or console.log() I guess?

Checklist

  • I'm using beta as my base branch
  • There is no overlap with another PR?
  • The PR is self-contained and cannot be split into smaller PRs?
  • Have I provided a clear explanation of the changes?
  • [ ] Have I considered writing automated tests for the issue?
  • [ ] If I have text, did I add placeholders for them in locales?
  • Have I tested the changes (manually)?
    • Are all unit tests still passing? (npm run test)
  • [ ] Are the changes visual?
    • [ ] Have I provided screenshots/videos of the changes?

src/battle-scene.ts Outdated Show resolved Hide resolved
src/battle-scene.ts Outdated Show resolved Hide resolved
@DayKev DayKev added (Legacy) Bug Legacy Label, don't apply to new issues/PRs Question Further information is requested labels Aug 20, 2024
src/utils.ts Outdated Show resolved Hide resolved
src/battle-scene.ts Outdated Show resolved Hide resolved
src/battle-scene.ts Outdated Show resolved Hide resolved
@DayKev DayKev marked this pull request as ready for review September 3, 2024 07:52
@DayKev DayKev requested a review from a team as a code owner September 3, 2024 07:52
@DayKev DayKev changed the title [Bug] Fix off-by-one error in some random number calls [Bug] Fix off-by-one errors in some random number calls Sep 3, 2024
@DayKev DayKev added Documentation Improvements or additions to documentation P2 Bug Minor. Non crashing Incorrect move/ability/interaction Miscellaneous Changes that don't fit under any other label and removed Question Further information is requested labels Sep 3, 2024
frutescens
frutescens previously approved these changes Sep 3, 2024
Tempo-anon
Tempo-anon previously approved these changes Sep 4, 2024
src/battle-scene.ts Show resolved Hide resolved
src/battle-scene.ts Show resolved Hide resolved
src/battle-scene.ts Show resolved Hide resolved
torranx
torranx previously approved these changes Sep 5, 2024
@DayKev DayKev dismissed stale reviews from torranx, Tempo-anon, and frutescens via cacc75d September 5, 2024 08:59
@DayKev DayKev added this pull request to the merge queue Sep 5, 2024
Merged via the queue into pagefaultgames:beta with commit 57a3efd Sep 5, 2024
4 checks passed
@DayKev DayKev deleted the fix-rng branch September 6, 2024 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to documentation (Legacy) Bug Legacy Label, don't apply to new issues/PRs Miscellaneous Changes that don't fit under any other label P2 Bug Minor. Non crashing Incorrect move/ability/interaction
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants