mmp / pbrt-v3

Source code for pbrt, the renderer described in the third edition of "Physically Based Rendering: From Theory To Implementation", by Matt Pharr, Wenzel Jakob, and Greg Humphreys.
http://pbrt.org
BSD 2-Clause "Simplified" License
4.89k stars 1.19k forks source link

Sphere thetaMin and thetaMax wrong #259

Closed FlashPanda closed 4 years ago

FlashPanda commented 4 years ago

in file "src/shapes/sphere.h", line 55 to line 58 since z = r * cos(theta) according to the book. Theta is in [0, pi]. Obviously, z is a monotone decreasing function to theta in [0, pi]. Is it should be thetaMin(std::acos(Clamp(std::max(zMin, zMax) / radius, -1, 1))), thetaMax(std::acos(Clamp(std::min(zMin, zMax) / radius, -1, 1))), since if zMax equals 1 ( let's say it's a unit sphere), the theta should be 0, which is the minimum value in its range.

mmp commented 4 years ago

Thanks for noting this. It's subtle--it seems that thetaMin corresponds to "the corresponding theta value at zMin", rather than the minimum theta value. (I had thought that your fix was correct, applied it, and then saw unexpected results and had to think about it for a while.)

In any case, it's not good that that's unclear; in pbrt-v4 that is now renamed to thetaZMin and thetaZMax...