jwagner / simplex-noise.js

A fast simplex noise implementation in Javascript / Typescript.
MIT License
1.61k stars 130 forks source link

Export alea function #49

Closed pspeter3 closed 1 year ago

pspeter3 commented 2 years ago

Export the alea function so other packages can use the same version as this library.

jwagner commented 2 years ago

Hey @pspeter3 Thanks for the PR. What use case are you trying to cover with this?

pspeter3 commented 2 years ago

Primarily trying to reduce dependencies. If I'm using the Simplex Noise library, I'd like to avoid having to install the alea library if possible and have multiple version of the algorithm. I also find your implementation easier to read (TypeScript) so I prefer to use it when possible.

SReject commented 2 years ago

I support this over PR #50 as it keeps this package self-contained, while also reducing end-user duplicate functionality by importing a secondary Alea Library.

My current use case is I am generating top-down 2d maps of which I layer multiple 2d noise maps.

To do this I seed an ALEA instance with the world seed then use calls to the instance to get the seed for each noise map.

/* psuedo code */

import Alea from 'alea';
import SimplexNoise from 'simplex-noise';

/* ... */

// seed ALEA prng
const prng = new Alea(world_seed);

// use ALEA prng to seed noise maps
const heightMap = new SimplexNoise(prng.random());
const tempMap = new SimplexNoise(prng.random());

/* ... */

Currently, the ALEA functionality is provided by a external dependency even though the functionality is contained within SimplexNoise and is leveraged to create noise maps.

If the ALEA functionality is exported this removes duplicate code within the project, and cleans up end-user interfaces

import { default as SimplexNoise, alea } from 'simplex-noise';

/* ... */
SReject commented 2 years ago

One suggestion I would make, is to capitalize the naming of the exported function. While not a constructor in nature it is a constructor in principal and thus should be denoted as Alea

jwagner commented 2 years ago

@SReject Thanks for your input.

One suggestion I would make, is to capitalize the naming of the exported function. While not a constructor in nature it is a constructor in principal and thus should be denoted as Alea

I follow the convention that I only capitalize actual constructors (to be invoked with new). I find that capitalizing functions that could be considered constructors creates more friction for both the people working on the project (arguing how constructor like a function is) as well as the users (not knowing whether the new keyword is required or not).

For the use case above you could also just use const heightMap = new SimplexNoise('heightmap_' + world_seed);, etc. In that case you wouldn't need access to the prng at all.

I'm still torn between having alea builtin and using an additional module. There are arguments either way, and at least for me they seem to just about even out. Guess that means I can just roll some dice. :D

pspeter3 commented 2 years ago

I agree with the naming convention distinction here. I'm happy with either solution as well, I just would rather not have two copies of Alea around. The advantage of dependency injecting the PRNG is that it matches the API of other libraries, like Poisson Disk Sampling. Additionally it could provide an opportunity to inject hash based noise functions. (For example, I often calculate the 32 bit MurmurHash and divide by 232 to get a float. This means I can do `hash(seed, x, y) / 2 32` and get the same value every time)

jwagner commented 2 years ago

Simplex-noise.js 4.0 has landed, and it removes the prng/alea completely. That seems like the cleanest option to me. But if there is demand I would also consider adding it back. With tree shaking in place there wouldn't be any real overhead for people not using it anymore. It could even lead to some fairly nicely reading API createNoise2D(seed('something')).