Add p5.strands support for randomGaussian()#8800
Conversation
| augmentFn(fn, p5, 'randomGaussian', function(...args){ | ||
| if(!strandsContext.active){ | ||
| return originalRandomGaussian.apply(this, args); | ||
| } | ||
| const mean = args.length >= 1 ? args[0] : 0; | ||
| const stdDev = args.length>=2 ? args[1] : 1; | ||
|
|
||
| const u1 = this.random(); | ||
| const u2 = this.random(); | ||
| const z = this.sqrt(this.log(u1).mult(-2)).mult(this.cos(u2.mult(2*Math.PI))); | ||
|
|
||
| return z.mult(p5.strandsNode(stdDev)).add(p5.strandsNode(mean)); | ||
| }); |
There was a problem hiding this comment.
Hi, thanks for the work @harshiltewari2004 , just one concern I have: are you defining randomGaussian inside the random function intentionally? Do you think it should be outside, otherwise randomGaussian won't exist until the user calls random() at least once, and it'll get redefined on every random() call.
Also, can you ad tests for this implementation?
There was a problem hiding this comment.
Good catch, thanks @perminder-17 — fixed in the latest commit. The randomGaussian augmentation is now a sibling of random, not nested inside it.
|
Happy to add tests @perminder-17 — I noticed there's no test/unit/strands/ directory yet, so I held off. Should I establish that pattern here, or is there an existing test approach for strands I missed? |
You can go with the visual tests, that would make more sense. Thanks for the update :) |
|
Thanks @perminder-17! Here's the plan — would appreciate a thumbs-up before I write the tests. Location: inside the existing Tests (two per file, four total):
WebGPU versions will be Reference screenshots will be generated on first run, sanity-checked (test 1: Gaussian noise around mid-gray; test 2: near-uniform gray), then committed alongside the test code. Does this look reasonable? Happy to adjust means/sd/sample counts, or add a third test for a different property. |
Yeah, everything sounds good to me. You can go with adding that. Thanks |
The averaging test (loop of 20 samples) revealed that randomGaussian was returning values from N(0.5, 1.0) instead of N(0.5, stdDev), causing heavy clipping visible as outlier pixels throughout the canvas. Root cause: unnecessary p5.strandsNode() wrapping around the stdDev and mean scalars in the final scale-and-shift expression. Removing the wrapping applies the multiplication correctly, mirroring how scalar literals (-2, 2*PI) are already passed to .mult() elsewhere in the same body. Tests added in both webgl and webgpu suites: - randomGaussian() colors a basic shader (single-sample distribution) - randomGaussian() in a fragment loop averages to the mean (variance reduction)
|
Tests added per discussion — randomGaussian() colors a basic shader and randomGaussian() in a fragment loop averages to the mean, in both webgl and webgpu visual suites. Following the existing patterns from the random() tests in the same files. |
|
|
||
| const u1 = this.random(); | ||
| const u2 = this.random(); | ||
| const z = this.sqrt(this.log(u1).mult(-2)).mult(this.cos(u2.mult(2*Math.PI))); |
There was a problem hiding this comment.
When we are doing this.log(u1) we need to ensure that the value is finite, but when u1 value would be 0, then this.log(u1) would be -infinity, do you think we can handle this case for u1?
There was a problem hiding this comment.
Good catch — yes, this is exactly it. When random() returns a value at or very near 0, log(u1) goes to -∞ and the sample blows up. I actually saw the symptom while writing the visual tests: the loop-averaging snapshot had a few scattered pure-white/black outlier pixels that the math said shouldn't be possible from normal Box-Muller tails. I flagged it in the PR description as a possible non-independence issue, but your read on log(0) is the real mechanism.
I'll clamp u1 away from 0 before the log — something like max(random(), epsilon) — which keeps it finite and should clear up those outliers. Will also regenerate the Test 2 snapshot and confirm the outliers are gone. Does that approach sound right to you?
There was a problem hiding this comment.
yeah, approach sounds good to me!
There was a problem hiding this comment.
Done — clamped u1 with this.max(this.random(), 1e-6) before the log, so when random() returns at or near 0, log(u1) stays finite instead of going to -∞. That caps the sample magnitude at roughly 5σ. I regenerated the loop-averaging snapshots and the outlier pixels I'd flagged in the PR description are now gone — Test 2 shows the expected uniform mid-gray. Thanks for catching the root cause.
When random() returns a value at or near 0, log(u1) goes to -infinity and the Box-Muller sample explodes, producing extreme outlier pixels. Clamping u1 with max(random(), 1e-6) bounds log(u1) at ~-13.8, capping the sample magnitude at ~5 sigma. Regenerated the loop-averaging visual snapshots, which now show the expected uniform-gray distribution with the outliers gone.
2 participants
Resolves #8775
Changes
Adds
randomGaussian()support for p5.strands, following @davepagurek's guidance in #8775. The implementation is a JS-side wrapper over the existingrandom()strands function, applying a Box-Muller transform to two uniform samples to produce a normal-distributed sample.src/strands/strands_api.jsnext to the existingrandom()registration.randomGaussian()when called outside a strands shader context.this.random()calls + chained node math.Implementation note
Uses
this.x()rather thanfn.x()per @davepagurek's note in the issue thread.Testing
No unit tests included — there's no existing
test/unit/strands/directory and no clear precedent for unit-testing strands functions (which produce IR nodes rather than directly-testable values). Happy to add tests in whichever direction the maintainers prefer.Local
npm testshows the same 4 flaky visual regression test failures (test/unit/visual/cases/typography.js) that I observed on dev-2.0 before my changes — unrelated to this PR.PR Checklist
npm run lintpasses