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.

Sign up for GitHub

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

Jump to bottom

Consider replacing seeds with rngs #197

Open
Ralith opened this issue Aug 27, 2018 · 5 comments · May be fixed by #350
Open

Consider replacing seeds with rngs #197

Ralith opened this issue Aug 27, 2018 · 5 comments · May be fixed by #350

Comments

@Ralith
Copy link
Contributor

Ralith commented Aug 27, 2018

32-bit seeds aren't great, and pervasive use of mostly-0 seeds may be causing odd results already. I propose that impl Seedable for T be replaced with impl Distribution<T> for Standard per rand 0.5 convention, with Default impls and new using an XorShiftRng with a hardcoded seed for reproducibility and convenience.

This would save users and implementers the effort of having to come up with a good seed (particularly when unpredictable results are desired) and improves the quality of randomness used at no cost. I'm interested in implementing this if people are on board with the idea. The changes involved might look like this:

diff --git a/src/noise_fns/generators/super_simplex.rs b/src/noise_fns/generators/super_simplex.rs
index 34c52fc..fdba524 100644
--- a/src/noise_fns/generators/super_simplex.rs
+++ b/src/noise_fns/generators/super_simplex.rs
@@ -1,6 +1,7 @@
+use rand::{distributions::{Distribution, Standard}, Rng};
 use {gradient, math};
 use math::{Point2, Point3};
-use noise_fns::{NoiseFn, Seedable};
+use noise_fns::{NoiseFn, default_rng};
 use permutationtable::PermutationTable;
 use std::ops::Add;
 
@@ -89,17 +90,14 @@ const LATTICE_LOOKUP_3D: [[i8; 3]; 4 * 16] =
 /// Noise function that outputs 2/3-dimensional Super Simplex noise.
 #[derive(Clone, Copy, Debug)]
 pub struct SuperSimplex {
-    seed: u32,
     perm_table: PermutationTable,
 }
 
 impl SuperSimplex {
-    pub const DEFAULT_SEED: u32 = 0;
-
     pub fn new() -> Self {
+        let mut rng = default_rng();
         SuperSimplex {
-            seed: Self::DEFAULT_SEED,
-            perm_table: PermutationTable::new(Self::DEFAULT_SEED),
+            perm_table: rng.gen(),
         }
     }
 }
@@ -110,23 +108,9 @@ impl Default for SuperSimplex {
     }
 }
 
-impl Seedable for SuperSimplex {
-    /// Sets the seed value for Super Simplex noise
-    fn set_seed(self, seed: u32) -> Self {
-        // If the new seed is the same as the current seed, just return self.
-        if self.seed == seed {
-            return self;
-        }
-
-        // Otherwise, regenerate the permutation table based on the new seed.
-        SuperSimplex {
-            seed,
-            perm_table: PermutationTable::new(seed),
-        }
-    }
-
-    fn seed(&self) -> u32 {
-        self.seed
+impl Distribution<SuperSimplex> for Standard {
+    fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> SuperSimplex {
+        SuperSimplex { perm_table: rng.gen() }
     }
 }
 
diff --git a/src/noise_fns/mod.rs b/src/noise_fns/mod.rs
index 82d5924..503b88c 100644
--- a/src/noise_fns/mod.rs
+++ b/src/noise_fns/mod.rs
@@ -1,3 +1,5 @@
+use rand::{SeedableRng, XorShiftRng};
+
 pub use self::cache::*;
 pub use self::combiners::*;
 pub use self::generators::*;
@@ -44,3 +46,7 @@ pub trait Seedable {
     /// Getter to retrieve the seed from the function
     fn seed(&self) -> u32;
 }
+
+const DEFAULT_SEED: [u8; 16] = [0x4f, 0x09, 0xd6, 0x9f, 0x62, 0x9b, 0x09, 0x0c, 0x0c, 0x49, 0x09, 0xfe, 0x6f, 0x1d, 0x4a, 0x38];
+
+fn default_rng() -> XorShiftRng { XorShiftRng::from_seed(DEFAULT_SEED) }
@Razaekel
Copy link
Owner

The purpose of having seeds is to provide reproducible results by using the same seed while giving users the ability to specify the seed as part of their generation process. Having a hard-coded seed would defeat the purpose of this library.

@Ralith
Copy link
Contributor Author

Ralith commented Aug 27, 2018 •
edited
Loading

Sorry, I must have been unclear. The library already contains many hard-coded seeds. This proposal contains two parts: improving the hard-coded seeds, and adjusting, not removing, the interface by which the user controls dynamic seeding. As outlined in the patch above, instead of supplying a u32, the user would provide a &mut T where T: Rng which is then used to initialize the noise function's internal state as appropriate. The user can seed an RNG in whatever manner suits them (from a constant, from storage, from entropy, ...).

@Razaekel
Copy link
Owner

Some discussion with Ralith indicated that I may have been misinterpreting the proposal, so I'm re-opening it for further discussion and expanding the details.

@Razaekel Razaekel reopened this Aug 28, 2018
@Ralith
Copy link
Contributor Author

Ralith commented Aug 29, 2018

Seeds in noise-rs are used to initialize internal RNGs, which are in turn used to construct noise function internal states (currently just permutation tables, maybe other things in the future?). The heart of this proposal is to instead allow the user to pass in a reference to a user-constructed RNG and use that directly, via the standard rand API. To construct a non-default instance of a noise function, a user would first construct some RNG, then call gen on it. For example:

let mut rng = XorShiftRng::from_seed(seed);
let noise = rng.gen::<SuperSimplex>();
noise.get(...);

This has a number of benefits:

  • Randomness
    A 32-bit seed is small; even the low-quality XorShiftRng is designed to take 128-bit seeds. This proposal allows the user to use whatever RNG suits them, and seed it in its preferred manner. No future API changes would be necessary to support new RNGs with different seeding requirements.
    While quality of randomness used may not currently be the dominant factor in the quality of noise functions, the existing interface nonetheless imposes an unnecessary and easily-corrected bottleneck.
  • Composability
    A number of noise functions are defined in terms of inner noise functions, and composing noise functions is in general a popular approach to obtaining high-quality output. Attempting to construct an composition of noise functions from a single seed using the current interface is awkward; using the same seed, or simple variations of it, for each one produces undesirable correlated output. By contrast, passing a reference to the same RNG through each component noise function's constructor preserves the full strength of the statistical independence guarantees provided by the RNG.
  • Ease of use
    Applications which require multiple noise functions and/or additional random data can generate as much as they like based on a single seed by making repeated calls to gen. Users who want irreproducible results can use rand::thread_rng(). Users who don't care about any of this can still call Default::default() for high-quality deterministic output.

The proposed interface is not mutually exclusive with the existing Seedable trait. If desired, it could be retained, although the seed getter could not return meaningful results for functions generated via the proposed method. For the above reasons I think it should at least be marked deprecated.

@Ralith Ralith mentioned this issue Oct 7, 2018
Closed
@v0x0g
Copy link

v0x0g commented May 17, 2024

What about a combined approach?

I was thinking of providing some default method such as Seedable::new_seed() that generates a high quality seed using the OsRng. This would be called by default when constructing the structs, but could be modified afterwards.
The noise generators like noise::Perlin already have private fields, so this should not be a breaking change, but it should significantly increase the quality of generators since they won't have a fixed default seed all the time.

I still support @Ralith with this one though, I think adding RNGs would be a good idea and I would personally like it if it were implemented.

Razaekel added a commit that referenced this issue May 19, 2024
@Razaekel
implement rng in place of u32 seeds
3710f0f 
Closes #197
@Razaekel Razaekel linked a pull request May 19, 2024 that will close this issue
Open
Razaekel added a commit that referenced this issue May 19, 2024
@Razaekel
implement rng in place of u32 seeds
89cd382 
Closes #197
Razaekel added a commit that referenced this issue May 19, 2024
@Razaekel
implement rng in place of u32 seeds
1953743 
Closes #197
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

implement rng in place of u32 seeds Razaekel/noise-rs
3 participants
@Ralith @Razaekel @v0x0g