Skip to content

Commit

Permalink
Fix NoiseGenerator to use all 64 seed bits, not just the bottom 32.
Browse files Browse the repository at this point in the history
This was an oversight in the original implementation of `NoiseGenerator`.  It turns out that while `std::seed_seq` will happily accept 64-bit entries, it just ignores all but the bottom 32 bits.  ಠ_ಠ  If you want to use 64 seed bits, you have to provide two separate 32-bit entries.

Unfortunately, this change does break backwards seed compatibility for `NoiseGenerator`, which we normally don't want to ever do.  If we needed another fix like this farther down the road, we'd need to do more work to make backwards compatibility possible, but fortunately nobody is relying on this yet, so it's not too late to just fix this the easy way.

PiperOrigin-RevId: 718498101
  • Loading branch information
Ink Open Source authored and copybara-github committed Jan 22, 2025
1 parent bf387a7 commit ca91c62
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 7 deletions.
4 changes: 3 additions & 1 deletion ink/strokes/internal/noise_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
namespace ink::strokes_internal {

NoiseGenerator::NoiseGenerator(uint64_t seed) {
std::seed_seq seq{seed};
// `std::seed_seq` ignores all but the bottom 32 bits of each entry, so we
// need to split our 64-bit seed value into two 32-bit entries.
std::seed_seq seq{seed & 0xffffffff, seed >> 32};
prng_.seed(seq);
std::uniform_real_distribution<float> distribution(0.0f, 1.0f);
prev_value_ = distribution(prng_);
Expand Down
23 changes: 17 additions & 6 deletions ink/strokes/internal/noise_generator_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,26 @@ TEST(NoiseGeneratorTest, RandomSequenceIsFixedForAGivenSeed) {
generator.AdvanceInputBy(0.1);
}
std::vector<float> expected = {
0.933754683, 0.915264189, 0.865075827, 0.791113913, 0.701303065,
0.603567779, 0.505832434, 0.416021585, 0.342059731, 0.291871309,
0.273380876, 0.276017755, 0.283174962, 0.293722451, 0.306530118,
0.320467860, 0.334405601, 0.347213268, 0.357760727, 0.364917934,
0.367554814, 0.363935769, 0.354112744, 0.339636654, 0.322058588,
0.302929521, 0.283800423, 0.266222358, 0.251746297, 0.241923273};
0.323644608, 0.332764149, 0.357517153, 0.393995285, 0.438290149,
0.486493349, 0.534696579, 0.578991473, 0.615469575, 0.640222609,
0.649342120, 0.642169000, 0.622699142, 0.594006658, 0.559165835,
0.521250844, 0.483335823, 0.448494971, 0.419802576, 0.400332719,
0.393159628, 0.388005435, 0.374015540, 0.353398860, 0.328364313,
0.301120877, 0.273877412, 0.248842880, 0.228226215, 0.214236364};
EXPECT_THAT(actual, Pointwise(FloatEq(), expected));
}

TEST(NoiseGeneratorTest, UsesAll64SeedBits) {
// Two different seed values should (in most cases, but in particular in this
// specific case) result in different values generated. We shouldn't, for
// example, just ignore the top or bottom 32 out of 64 seed bits.
NoiseGenerator generator1(0x10000000DeadBeefLL);
NoiseGenerator generator2(0x20000000DeadBeefLL);
NoiseGenerator generator3(0x10000000DeadBeadLL);
EXPECT_NE(generator2.CurrentOutputValue(), generator1.CurrentOutputValue());
EXPECT_NE(generator3.CurrentOutputValue(), generator1.CurrentOutputValue());
}

// Tests that all values emitted from the NoiseGenerator are in [0, 1].
void EmitsValuesBetweenZeroAndOne(uint64_t seed, float advance_by) {
NoiseGenerator generator(seed);
Expand Down

0 comments on commit ca91c62

Please sign in to comment.