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

Gives rpg-dice-roller access to higher quality random number generators. #142

Closed
wants to merge 1 commit into from

Conversation

devonjones
Copy link

Fixes #141 - Adds ability to use window.crypto and Mersenne Twister f…or the PRNG

@GreenImp
Copy link
Collaborator

Hey @devonjones, this is on the master branch, so I'm going to have to reject it, I'm afraid. We use git-flow, so all PRs need to be branched from develop and merged back in to develop.
I know master shows up as the default branch on Github, so it's confusing, but I can't set the default to develop, otherwise it causes issues with other things.
Check out the contributing guide "Contributing code" and "Styles and standards" sections: https://github.com/GreenImp/rpg-dice-roller/blob/master/CONTRIBUTING.md#setting-up

I've got a few thoughts about this PR, that I think could be really helpful to discuss:

  • I think if we're introducing a new class, it should be as a separate ES module, like the Dice, Modifiers etc., rather than inside the utils file. It makes it easier to maintain and test that way.
  • The library is written predominantly is ES6, using module imports, rather than require, so that it works environments that understand module imports. There's a few require statements in the source code, for importing commonJS modules, that get compiled into the lib bundle for a release, which works nicely.
    I've notice though, that the require("mersenne-twister") doesn't get bundled in, and the require statement remains in the bundled code. I'm assuming this is because it's not at the top level of the file. Unfortunately, this means that it will break for non-node environments, and there's no way of getting it to use the Mersenne Twister code in those environments, even if you have it installed another way.
    Because we're providing both an ES module, and UMD version of the library, I'm not keen on introducing code that can't be compiled to one or the other.
    I'm also not sure that I'd want to bundle the Mersenne Twister code in with the library as we're then forcing it to be used. I'm not necessarily averse to this, but the library hasn't been updated in around 4 years, so don't really want to rely on something that is unsupported.
  • There's a few ESlint errors in the utils.js file that aren't being thrown because there's two /* eslint-disable */ lines in the file, which was stopping it linting the entire file (I thought it just stopped it linting within the if statements they were in).
    Ideally, they'd need to be resolved before merging, if you wouldn't mind. If you just remove the /* eslint-disable */ one lines 2, and 32, and run npm run lint it'll show the errors. Don't worry about the ones in the array flat() and flatMap() methods - they're temporary polyfills for old Node versions that'll probably be removed soon.
    I'm going to get a fix in soon so that ESLint correctly lints that file, but ignores those two polyfills.
  • I feel as though the method specific number functions (e.g. cryptoGetRandomInt, mersenneTwisterGetRandomInt, etc.) should probably be private, so people only use the generic one. I can imagine that they might change, or be removed if different libraries are used.

A different approach?

I like the idea of being able to use different RNGs, but I'm wondering if it might be better to go down a plugin style route, where you can pass in a function, or Class instance that it will use.
So, instead of hard-coding a series of ones to check, we just allow a user to pass through one of:

  • A function that accepts a min and max value and returns an integer.
  • An object that contains a generate (or similarly named) method, that accepts a min and max value and returns an integer.

The object one would allow the user themselves control over the implementation and, if they have a Mersenne Twister implementation, or another library (e.g random.org), they can pass it through to the dice roller (Most likely in a small wrapper).

For the mersenne-twister library you were using, I'd imagine it would look something like:

const { DiceRoller } = require('rpg-dice-roller/lib/umd/bundle.js');
const MersenneTwister require('mersenne-twister');

const seed = 1234;
const mtGenerator = new MersenneTwister(seed);

// using a function
const diceRoller = new DiceRoller({
  options: {
    rng(min, max) {
      return Math.floor(mtGenerator.random() * (max - min + 1) + min);
    },
  },
});

// using an object
const diceRoller = new DiceRoller({
  options: {
    rng: {
      generator: new MersenneTwister(seed),
      generate (min, max) {
        return Math.floor(this.generator.random() * (max - min + 1) + min);
      },
    },
  },
});

The idea of passing it through to the DiceRoller like that is completely in my head at the moment, as there is currently no sense of an ability to set options. Although I've been thinking of implementing something similar for other purposes as well (Mainly the output functionality #132), so this might be a good time to start.


I'd greatly appreciate feedback on those points. Sorry it got a bit long-winded!

As mentioned above, this PR will have to be closed, just because it's on the wrong branch. But it would be good to continue the discussion on the functionality in here 😄

@GreenImp GreenImp closed this Jun 27, 2020
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.

Switch to a better PRNG
2 participants