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

chaos scroll fix #261

Merged
merged 2 commits into from
Sep 13, 2024
Merged

chaos scroll fix #261

merged 2 commits into from
Sep 13, 2024

Conversation

pimittens
Copy link

Description

The intention of the rand method (which is used to determine stat changes when using chaos scrolls) is to return a pseudorandom uniformly distributed int between lbound and ubound (both inclusive). The previous implementation did not work as intended when the lower bound was negative since java rounds up instead of down when casting a negative double to an int. As a result, +0 was twice as likely as any other result, and -lbound was only possible when nextDouble returned exactly 0. Adding the lower bound after the cast rather than before fixes this since the number being cast to an int will always be positive.

Also changed the CHSCROLL_STAT_RANGE from 6 to 5 in the config to be consistent with gms.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested my changes

The intention of the rand method is to return a pseudorandom uniformly distributed int between lbound and ubound (both inclusive). The previous implementation did not work as intended when the lower bound was negative since java round up instead of down when casting a negative double to an int. Adding the lower bound after the cast rather than before fixes this.

Also changed the CHSCROLL_STAT_RANGE from 6 to 5 to be consistent with gms.
Copy link
Owner

@P0nk P0nk left a comment

Choose a reason for hiding this comment

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

Good fix, just missing a test.

@@ -35,6 +35,6 @@ public static long nextLong() {
}

public static int rand(final int lbound, final int ubound) {
return (int) ((rand.nextDouble() * (ubound - lbound + 1)) + lbound);
return ((int) (rand.nextDouble() * (ubound - lbound + 1))) + lbound;
Copy link
Owner

Choose a reason for hiding this comment

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

Could really use a unit test. I wrote one to test out your change so you might as well just take it:

package tools;

import org.junit.jupiter.api.Test;

import java.util.HashMap;
import java.util.Map;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

class RandomizerTest {

@Test
void randShouldIncludeEntireRange() {
    Map<Integer, Integer> rands = new HashMap<>();

    final int rounds = 100_000;
    for (int i = 0; i < rounds; i++) {
        int randomValue = Randomizer.rand(-5, 5);
        rands.compute(randomValue, (k, v) -> v == null ? 0 : v + 1);
    }

    assertFalse(rands.containsKey(-6));
    assertTrue(rands.containsKey(-5));
    assertTrue(rands.containsKey(-4));
    assertTrue(rands.containsKey(-3));
    assertTrue(rands.containsKey(-2));
    assertTrue(rands.containsKey(-1));
    assertTrue(rands.containsKey(0));
    assertTrue(rands.containsKey(1));
    assertTrue(rands.containsKey(2));
    assertTrue(rands.containsKey(3));
    assertTrue(rands.containsKey(4));
    assertTrue(rands.containsKey(5));
    assertFalse(rands.containsKey(6));
}

}

@P0nk P0nk merged commit 5fabbaf into P0nk:master Sep 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants