ondras / rot.js

ROguelike Toolkit in JavaScript. Cool dungeon-related stuff, interactive manual, documentation, tests!
https://ondras.github.io/rot.js/hp/
BSD 3-Clause "New" or "Revised" License
2.32k stars 254 forks source link

ROT.RNG.setSeed unexpected behavior #184

Open nluqo opened 3 years ago

nluqo commented 3 years ago

I'm using setSeed for seeded runs in a game and was really surprised by the behavior. Two main issues:

Perhaps these are known behaviors, but from the developer perspective I found them really confusing... and it's quite easy to write some broken randomness by not knowing it. Even some notes on the docs would help out a lot.

ondras commented 3 years ago

Hi @nluqo,

good finding! Thanks for the report.

* The only valid input seems to be positive numbers.

Right, the number type is also mentioned in the autogenerated documentation: https://ondras.github.io/rot.js/doc/classes/_rng_.rng.html#setseed

* Similar seeds produce similar random numbers at least for the first one.

This is a problem indeed. I am open to fixing it, but we are facing a backwards compatibility issue - folks might be expecting a particular behavior for their already-generated seeds.

* Passing a string would be really useful,

So, how about we add a seed-by-string feature that uses a better seeding mechanism and produces the desired avalanche effect? This would be a compatible change - and the documentation can mention that seeding with a string produces generally more robust results.

I am, however, not sure how to properly implement the seeding with a string. The current RNG implementation (Alea) is based on the work at https://github.com/nquinlan/better-random-numbers-for-javascript-mirror.

blinkdog commented 3 years ago

I am, however, not sure how to properly implement the seeding with a string. The current RNG implementation (Alea) is based on the work at https://github.com/nquinlan/better-random-numbers-for-javascript-mirror.

I'd put (pre-salt + the string + post-salt) through SHA-256 and then pull the least significant bytes to turn into a seed value.

https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest

The salt strings can be anything and don't need to be kept secret. Running dd if=/dev/urandom bs=1 count=24 | base64 gave me these:

PRE_SALT = "j2wAXcEUrFnZGpt9WsZE1RV4oNAOBcBO"
POST_SALT = "c5/0KcU6ad912gQ88c/53Ng+magf2x7P"
ondras commented 3 years ago

I'd put (pre-salt + the string + post-salt) through SHA-256 and then pull the least significant bytes to turn into a seed value.

The RNG uses four state values, though I am not sure how exactly is the _c supposed to work - (current) seeding sets it always to 1. So you suggest using the last 8*3 bytes to set _s0, _s1 and _s2 ?

ondras commented 3 years ago

https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest

Also, I would prefer the seeding process to be synchronous :-)

blinkdog commented 3 years ago

I'd put (pre-salt + the string + post-salt) through SHA-256 and then pull the least significant bytes to turn into a seed value.

The RNG uses four state values, though I am not sure how exactly is the _c supposed to work - (current) seeding sets it always to 1. So you suggest using the last 8*3 bytes to set _s0, _s1 and _s2 ?

Yeah, that'd be a good way to do it.

blinkdog commented 3 years ago

https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest Also, I would prefer the seeding process to be synchronous :-)

Hmmm, in Node.js I'd use: https://nodejs.org/api/crypto.html#crypto_crypto_createhash_algorithm_options

And the browser I'd borrow Browserify's browser implementation of Node's crypto: https://github.com/crypto-browserify/crypto-browserify https://github.com/crypto-browserify/createHash https://github.com/crypto-browserify/sha.js

These would give compatible synchronous strong hash functions.

ondras commented 3 years ago

Sounds a bit like an overkill to me. We just need a way to convert a string to three 64bit values, without any particular crypto/security properties... how about the original Baagoe's Mash function? I would say this should have been used from the very beginning.

blinkdog commented 3 years ago

Sounds a bit like an overkill to me. We just need a way to convert a string to three 64bit values, without any particular crypto/security properties... how about the original Baagoe's Mash function? I would say this should have been used from the very beginning.

Yeah, that's pretty cool. I'll bookmark this one.

nluqo commented 3 years ago

Right, the number type is also mentioned in the autogenerated documentation:

Ah yes, I forgot this even existed. Woops. Though I still think it should mention the sign of the number. I was using a library to generate SHA256 digests and getting an int out of it and it only returned signed integers... and thought I was good for a while.

It's easy to test a couple times (positive input, negative input, positive input) and think "these all look different enough" but not realize all your negative inputs will degenerate to the same state.

So, how about we add a seed-by-string feature

Sounds good to me. It will change from the current behavior if you were misusing the API and passing a string, but I assume this is unlikely.