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

Listing 37: Incorrectly generating random unit vector #697

Closed
pmcmorris opened this issue Aug 16, 2020 · 6 comments
Closed

Listing 37: Incorrectly generating random unit vector #697

pmcmorris opened this issue Aug 16, 2020 · 6 comments
Assignees
Milestone

Comments

@pmcmorris
Copy link

pmcmorris commented Aug 16, 2020

Maybe I'm reading this incorrectly but what the code does doesn't seem to match the function name. The current code appears to do a uniform distribution on cylinder and then stretch to a sphere. Doesn't that raise the probability of points near the poles?

vec3 random_unit_vector() {
    auto a = random_double(0, 2*pi);
    auto z = random_double(-1, 1);  // uniform in z means the same # of hits over less surface area
    auto r = sqrt(1 - z*z);
    return vec3(r*cos(a), r*sin(a), z);
}

I was expecting something more like this:

vec3 random_unit_vector() {
        auto a = random_double(0, 2 * pi);
        auto b = random_double(-pi/2, pi/2);
        auto z = sin(b);
        auto r = sqrt(1 - z * z);
        return vec3(r * cos(a), r * sin(a), z);
}
@yig
Copy link

yig commented Aug 18, 2020

This is related to #696

@hollasch hollasch self-assigned this Aug 18, 2020
@hollasch hollasch added this to the v3.2.1 milestone Aug 18, 2020
@hollasch hollasch removed their assignment Aug 20, 2020
@hollasch hollasch modified the milestones: v3.2.1, v3.2.2 Sep 30, 2020
@hollasch
Copy link
Collaborator

See https://mathworld.wolfram.com/SpherePointPicking.html for the derivation of the random_unit_vector() function. Still, the text needs to be made more clear.

@hollasch hollasch self-assigned this Oct 11, 2020
@pmcmorris
Copy link
Author

Reading my own advice... don't to that either it has a similar problem.

But I'm assuming wolfram would have a much better fix. :)

@hollasch
Copy link
Collaborator

No fix to the function is needed. The current function is identical to the approach illustrated in Wolfram, though the variable names are different.

@hollasch
Copy link
Collaborator

hollasch commented Oct 11, 2020

Hmmm. Now I'm doubting myself. I think you may be right. Investigating. Specifically, for equations 6-8 on the Wolfram page, u ∈ [-1, 1], but in the text above says “we can pick u = cos φ to be uniformly distributed”. That's not the same as u = random_double(-1, 1).

@hollasch
Copy link
Collaborator

Took a while to get through my thick skull. Turns out I added this alternate algorithm 2019-10-21 (one year ago), and yes, misunderstood what u meant above.

The original form used random_in_unit_sphere() and then normalized those points. I'm going to revert back to that, since it's most comprehensible.

hollasch added a commit that referenced this issue Oct 11, 2020
I added an alternate formula for uniformly-distributed points on a unit
sphere for this function. It's presented in
https://mathworld.wolfram.com/SpherePointPicking.html, equations 6-8.

Unfortunately, I misinterpreted "u in [-1, 1]" to be equivalent to
`u = random_double(-1,1)`. However, you're supposed to pick u = cos(phi)
to be uniformly distributed.

I replaced this body with the original one, just generating a unit point
_in_ the unit sphere, and normalizing the result.

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

No branches or pull requests

3 participants