Subtle bug in ShapeSet sampling (15.6.4)

[Jesper de Jong]

Hello everybody,

I'm looking at the Sample function of class ShapeSet in paragraph
15.6.4 (page 709) of the pbrt book. I think there is a subtle bug
there.

Suppose that for some reason the condition of the if statement in the
loop never becomes true, then in the last line of the function, sn ==
shapes.size() so that shapes[sn] refers to an element that's one past
the end of the vector - which might crash the program.

regards,
Jesper

[Matt Pharr]

I'm pretty sure that can't happen: the last entry in the CDF array
should be 1.0, exactly, and the random number generator is only
supposed to generate values in the range [0,1). i.e. up to but not
including 1.0. Therefore, I don't think that can ever happen. (Or if
it did, it'd be a bug elsewhere in the system.) (Have you seen a
crash here in practice, or is this just a concern based on reading the
code?)

Thanks,
-matt

[mljack]

When using pbrt 1.02, we do find RandomFloat() can return the exact
1.0f very rarely. Usually it's not big deal, but sometimes it leads
crash. (eg: if you depends on the exactly sample number of each pixel)

workround: regenerate another one
inline float RandomFloat() {

float t=genrand_real2();
while(t>=1.0f)
{
printf("\nGot 1.0f in RandomFloat()\n");
t=genrand_real2();
}

return t;
}

-MLJack

[Matt Pharr]

A-ha--you're right. (I'd never specifically tested this, but a quick
check shows that this bug is there, still in pbrt v2 even.) The issue
is that the Merseene twister code does return numbers in [0,1), but it
returns doubles, and pbrt converts them to floats. Some of the
doubles close to 1 round up to 1, and there you go.

The below fixes it in pbrt v2 (will push this up to the git version
later today):

float RNG::RandomFloat() const
{
return (RandomUInt() & 0xffffff) / float(1 << 24);
}

(The corresponding edit to genrand_real32() in pbrt v1 should fix it
there.)

Thanks for pointing this out!

-matt