dice-roller / rpg-dice-roller

An advanced JS based dice roller that can roll various types of dice and modifiers, along with mathematical equations.
https://dice-roller.github.io/documentation
MIT License
243 stars 58 forks source link

Switch to a better PRNG #141

Closed devonjones closed 4 years ago

devonjones commented 4 years ago

Is your feature request related to a problem? Please describe. Javascript's random number generator is notoriously bad. I would like to see the use of a better PRNG. My suggestion is https://github.com/boo1ean/mersenne-twister. It's a drop in replacement for Math.Random, but can also take in seeds. What this means is, you can also get a repeatable sequence of random numbers based on that seed selection, which can be super useful for tests and simulations. It also has a handy random_int() function.

Describe the solution you'd like Switch the PRNG to one with higher quality entropy

devonjones commented 4 years ago

window.crypto.getRandomValues is even better.

I have a patch incoming that will use that if it's available, mersenne-twister if its installed, and fails back to math.random

GreenImp commented 4 years ago

An interesting suggestion. I had thought of allowing other number generators to be used in the past, but didn't see much need for it at the time.

What would you say is bad about Math.random()? From what I understand, it's a fairly reasonable PRNG, with the only downside being that it's not cryptographically secure - although I don't think that's a concern for this library, I'm happy to be told otherwise! Do you have any links etc. detailing why it'd bad? I'd genuinely be interested in reading it. I did a quick search but couldn't find anything.

Also window.crypto.getRandomValues is browser only so, if any changes are made, we'll just have to make sure they don't break in Node.

peteroupc commented 4 years ago

I believe the concern is that the exact implementation and thus quality of Math.random() isn't guaranteed: notably, all the ECMAScript specification says is that Math.random() uses an "implementation-dependent algorithm or strategy".

devonjones commented 4 years ago

@GreenImp I made sure the implementation will only use window.crypto and mersenne-twister when available, because I didn't want to bloat your required install. Also, there's a way to force it to any of the three implementations.

As for the problem on Math.random(), what @peteroupc said, but also, using Mersenne-twister you can use seeds, which is useful for testing, but also for simulations.

In one use case, this is so that software that depends on rpg-dice-roller can write tests with knowable results for dice rolls when using the same seed (you can see an example in the test I added). The other use case for high quality randomness is if people are doing simulations. I used this library for example in a personal project where I was doing monte carlo simulations for the damage match ups of different units in Warhammer 40k, so a solid RNG for that is useful.

But really, the impetus behind it is that we're playing D&D online, and the dice app we're using right now in slack is super streaky, and some people don't trust it. I'm writing a replacement for slack, and if I can say that it's using an RNG that's sufficient for secure communications, it's gonna be more than good enough for dice - so it's really about getting the players to trust it.

GreenImp commented 4 years ago

Hi @devonjones. I've had a good look over your PR and left a lengthy (Sorry) response on there, more about the technical aspects of the PR itself.

I like the idea of being able to change the RNG. As mentioned on the PR, I'm wondering if a modularised approach would be better, as it would allow greater flexibility with users swapping the RNG with other implementations, rather than be limited with some specific ones.

Also, I have had to reject your PR as it was on the master branch, instead of develop. I can understand the confusion, as master is set to the default in Github. Unfortunately, there's no way of specifying master as the "live / release" version, and develop as the default to branch from / to.

GreenImp commented 4 years ago

Just as a follow-up, I've come across this: https://www.npmjs.com/package/random-js

I've not had the chance to test it, but it looks like it could help.

GreenImp commented 4 years ago

I've been having a think about this, and have been playing around with random-js, and have come up with what will hopefully be a decent solution.

PR #155 adds a new NumberGenerator class that has two functions, integer and real, which produce a random integer or float number respectively.

By default, it uses Math.random(), but you can specify an engine for it to use from either a built-in list, or your own custom one.

The current built-in options are:

const engines = {
  browserCrypto,
  nodeCrypto,
  MersenneTwister19937,
  nativeMath, // Math.random
};

From your app, you can access the NumberGenerator instance from the NumberGenerator namespace:

import { DiceRoller, NumberGenerator } from 'rpg-dice-roller';

// `engines` is an object containing the built in engines as listed above
console.log(NumberGenerator.engines);

// the generator object
console.log(NumberGenerator.generator);

// store the generator for ease of use
const generator = NumberGenerator.generator;

// use MersenneTwister
generator.engine = NumberGenerator.engines.MersenneTwister19937.seed(521);

// generate an integer between 1 and 4
console.log(generator.integer(1, 4));

// use nodeCrypto
generator.engine = NumberGenerator.engines.nodeCrypto;

// roll a dice using the currently set engine (nodeCrypto)
const roller = new DiceRoller();
console.log(roller.roll('4d6'));

But you can also create your own engines, which only need to be an object with a next() function:

import { NumberGenerator } from 'rpg-dice-roller';

const customEngine = {
  next() {
    // logic for number generation here...
  },
};

NumberGenerator.generator.engine = customEngine;

It uses https://github.com/ckknight/random-js behind the scenes, so you can read the docs for it to get a better idea of how it works, and how the engines function.

The old diceUtils.generateNumber() method has been deprecated, and will be removed in the future.

I'm hoping this does what you were after. I'm hoping to get it out in the next release.

GreenImp commented 4 years ago

PR has passed all tests, and has been merged into develop so this will be released soon.