We just uncovered a silly bug that was completely my fault. I’m still smacking my head for the sloppiness! Ultimately, it came down to using the same names for member variables and local/parameters variables—nothing some good coding habits would have avoided. In my defense, pbrt is quite horrendous in terms of coding style and when you work on an existing code base that is huge and not written to the best standards, it rubs off on you. For example, they have two methods for testing intersections on shapes, Intersect and IntersectP both of which do the exact same work. The only difference is that one returns a bool and the other will get information about the actual intersection.
Anyhow, on to the bug. I had written the photon gathering step earlier and based it off of the existing implementation. It had something along the lines of the following:
class VolumetricPhotonMapping {
private:
static Spectrum gather(const PhotonMap* pmap, float maxDist) {
Photons* nearby = pmap->lookup(pmap, maxDist);
// process nearby photons and return spectrum
}
float maxDist;
PhotonMap* pmap;
};
Basically what gather does is it takes the PhotonMap pointer and distance by value. It is static because all the information it needs gets passed in. This is all good an well, until I had to move some of the functionality of gather into a performance critical area. One of the operations it performs is
pmap->lookup(nPhotons, maxDist);
What this method does is looks for the nPhotons nearest photons within maxDist. However, if it could find it in closer, it will change maxDist to be the smallest value such that it found nPhotons. Hence, maxDist gets passed in by reference.
Following suit of how most of the existing code base is written, I copied and pasted gather wholesale and simply moved different sections around a section of code. What I forgot, however, was that the member variable maxDist inside of gather was masked by the argument passed in. When copying this over to a separate method, the compiler did not complain; however, now it was changing the member variable.
Catching this was rather quick. We suspected some state was changing when it shouldn’t. We tacked on the const keyword to the new method I added and it immediately caught that line.
The moral: good coding habits prevents sloppy software bugs.
What I should have done, first of all, was not copy code around. That is the main thing I hamper by students about, yet I fall prey to it just as easily. Also, many coders adopt the habit of prefixing member variables with an _. It takes a while to get used to at first, but is actually quite a useful decision. It allows also allows you to have a method with the actual variable name:
class foo {
public:
int bar() const { return _bar; }
private:
int _bar;
};
I think having something like bar() is much more pleasant and natural than getBar(). Finally, probably the most important thing (that I usually tend too!) is const-correctness. In almost every software engineering class I’ve been in, we always stress the importance of catching bugs at compile time. const-correctness allows you to control when and where state gets changed. Learn it, live it, love it.
Leave a comment