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

Added seeding, increased period, fixed bug in 2D gradient #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

akriegman
Copy link

No description provided.

@SRombauts
Copy link
Owner

Hello,
Thanks for the fix and improvements!
Have you seen it's not compiling because of an implicite cast warning?

@akriegman
Copy link
Author

It wasn't compiling because I made SimplexNoise::noise non-static because I wanted it to have access to mSeed. I also made the cast explicit.

@Phildo
Copy link

Phildo commented Aug 30, 2021

Is there any reason this PR hasn't been merged in? I'm looking for seedable simplex noise, and this looks like it would do the trick!

The only code change that I don't understand is this one:

static float grad(int32_t hash, float x, float y) {
  const int32_t h = hash & 0x3F;  // Convert low 3 bits of hash code
  const float u = h & 4 ? x : y;  // into 8 simple gradient directions,
  const float v = h & 4 ? y : x;

(The & 4 was previously a < 4; now that it's &, the temporary variable h (which is the masked low 6 bits of hash?) seems unnecessary- should be able to pull all the other masks of h straight from hash!)

@akriegman
Copy link
Author

Is there any reason this PR hasn't been merged in? I'm looking for seedable simplex noise, and this looks like it would do the trick!

My PR probably shouldn't be merged in for two reasons: first, it changes the usage by making certain functions non-static, which is necessary so they can use mSeed, but could break existing code using this project. Second, I added a few arithmetic operations into each call of the noise function so that the seed can influence the hash, and people who don't want their noise seeded shouldn't have to pay for those extra operations. I suggest you use my fork if you want seeded noise.

The only code change that I don't understand is this one:

static float grad(int32_t hash, float x, float y) {
  const int32_t h = hash & 0x3F;  // Convert low 3 bits of hash code
  const float u = h & 4 ? x : y;  // into 8 simple gradient directions,
  const float v = h & 4 ? y : x;

(The & 4 was previously a < 4; now that it's &, the temporary variable h (which is the masked low 6 bits of hash?) seems unnecessary- should be able to pull all the other masks of h straight from hash!)

So I'm not totally sure because this was so long ago, but I think what's happening here is that the original conditional h < 4 was meant to test whether the third bit was set, and h & 4 is just a more robust way to test that. I'm also not sure why the mask is 0x3f when the comment says it's using the 3 low bits, that mask should take the 6 low bits. So idk what's going on here.

@Phildo
Copy link

Phildo commented Aug 30, 2021

My PR probably shouldn't be merged in for two reasons: ...

ah ok that's fair. I might create a PR of my own, loosely based on yours?

  1. I'd still like to get the below fix in there,
  2. it should be possible to instead "seed" by shuffling the param array with a seeded prng (which would be a one-time [tiny] perf cost, and the API could stay static if the user is unconcerned with the seeding being global).

...I think what's happening here is that the original conditional h < 4 was meant to test whether the third bit was set, and h & 4 is just a more robust way to test that

The comment is definitely outdated (it's certainly taking the low 6 bits, and not the low 3...). but regarding the intent of the original h < 4- I also have no idea... Regardless though- if you're taking a bit mask which is a subset of another bitmask, you can eliminate the first bitmask (x & 0xFF & 0xF is just x & 0xF). I'm sure the compiler does that anyways? But it might as well get changed (in addition to your s/</&/ fix) for clarity, unless I'm missing something...

@akriegman
Copy link
Author

Okay I think I remember why I made that change. The idea of that function is to take the dot product with one of the eight vectors (1,2), (-2, 1), etc., choosing each with equal probability. The way the function was before I changed it it was not taking those eight vectors with equal probability, since the condition h < 4 was failing more than it was passing, when it was supposed to be 50-50. This was adding a bias to the noise, which I guess was not strong enough to be noticed. My fix made all eight vectors come up with equal probability. You're right, the masking line should be removed for speed since it does nothing. I don't know why a six bit mask was used in the first place, although I'm curious to know.

Do you mean the perm array? You could make that a member of the object. It's up to you if global seeding works for what you're doing, but instance seeding is useful if you need multiple noise fields or a noise vector field (with one SimplexNoise object for each component`).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants