[bug] Ray::tMax is mutable

[klein-j]

I ran into a problem with the Ray::tMax field while experimenting with new rendering algorithms. In my approach, it could sometimes happen that the same ray object was tested against the same scene multiple times. Admittedly, this is suboptimal and I could have probably optimized it, but it is still something that you should be able to do, if you really want. But in pbrt you currently can't.

The reason for that is the Ray::tMax variable, which is used to determine the closest intersection point in the Scene::Intersect* functions. They operate on a const Scene and a const Ray, thus their interface promises that there will be no side effects. However, Ray::tMax is modified and never reset, thus testing the same ray object twice can yield different results (this appears to be somewhat random and has probably something to do with float inaccuracies).

I don't think tMax should be declared as mutable because it is a part of the Ray state that affects computations. In fact, I think Ray should not have any state in the first place. tMax should be a local variable inside the Intersection method and be passed down to the more low-level functions along with the ray object, which in turn should be truly const and stateless.

A simpler fix that works for me for now (after I spent a week of debugging random artifacts in my renderings) is to just reset tMax in Scene::Intersect. I'm however not submitting this as a pull request, as I don't feel like it is an appropriate solution and not solving the underlying design issue.

Although I clearly consider this as a bug, it is somewhat debatable. Having single-use ray objects makes sense, as duplicated intersection test are not efficient. But pbrt is a learning platform, and in this regard a interface that does not work in the way that it is documented is utterly confusing.

[mmp]

It's an interesting question / trade-off. FWIW tMax has always been mutable in pbrt (and we do discuss this in the book text). That said, your experience of this being confusing/unexpected would be nice to prevent in the future.

This would be too big a change to make for pbrt-v3 since we want to keep the source code in sync with the book text. Therefore, I have made a note to reconsider this issue when we do pbrt-v4.

Thanks!

[klein-j]

I agree, that changing this would be a too big change for now.

Also, the book does indeed discuss the mutability of tMax (also quite nicely as mutable is a more advanced C++ features which not all readers can be expected to now of, so I really like this), however not its consequences: That tMax needs to be reset for new tests. It could also be considered, to remove the mutability and pass non-const rays to said functions. I think by convention, mutable should be used for object states that are not visible from the outside, which is clearly not the case here.

Though yeah, probably nobody has experienced this issue before me, and probably nobody will in the next time, so the priority on this is probably somewhat low.