Revisit public vs private member variable access qualifiers

[GalenMoo]

Looks like this line should be private rather than public for the ray class

raytracing.github.io/src/common/ray.h

Line 36 in 588b950

public:

[trevordblack]

Yep. I think you're right

[hollasch]

Actually, it should be public, and we should remove the accessors. There's no reason to guard these members, as it's not possible to create an inconsistent state, so the encapsulation machinery provides only additional complexity for no real value.

[hollasch]

Note that in general we could do an encapsulation pass, as this is something we have scattered among our various classes. The way the code is structured, many of these are simple classes/structs with only helper methods, and no real state management to do.

[trevordblack]

If we look at the v1.42.0, the original ray class was:

class ray
{
    public:
        ray() {}
        ray(const vec3& a, const vec3& b, float ti = 0.0) { A = a; B = b; _time = ti;}  
        vec3 origin() const       { return A; }
        vec3 direction() const    { return B; }
        float time() const    { return _time; }
        vec3 point_at_parameter(float t) const { return A + t*B; }

        vec3 A;
        vec3 B;
        float _time;
};

So I can see how today's kludge evolved.

[hollasch]

I added the additional public specifier to mark the beginning of member variables, preserving original access class. If we decided in the future to convert to greater encapsulation, we could update the access. In this case, it doesn't need to be private, though the method accessors suggest it.

We could convert such situations to use struct instead of class. Even though these keywords are just about synonyms in C++, struct more closely hints at how the class operates logically.

I do admit that the multiple public-public labels are unconventional.

[hollasch]

More and more (and this is a personal opinion), I chafe at the automatic getter-setter method for private member variables, when they are uniformly implemented in trivial fashion. It feels knee-jerk and a bit cargo-cultish to me, adds no additional real value or protection, and often adds a considerable about of useless glitter.

For example, I'm now tempted to just do this:

struct ray {
    point3 origin;
    vec3 direction;
    double time;

    ray(const point3& o, const vec3& d) : origin(o), direction(d), time(0) {}
    ray(const point3& o, const vec3& d, double t) : origin(o), direction(d), time(t) {}

    point3 at(double t) const {
        return orig + t*dir;
    }
};

Notes

1. Really, our ray is just a struct with three values, helpful constructors, and a single evaluate
   method.

2. For simple structs, I prefer the data members up front.

3. The original ray() {} constructor pattern isn't great. It bypasses the standard language default constructor (which default-initializes all members). This is more efficient when allocating blocks of ray that you don't want to spend time initializing, but we don't do that. We have several classes that use this pattern, and it might make sense to revisit this.

4. I could almost see naming the members 'p' and 'd', but naming time 't' would be a point of
   confusion. Since we can't do that, I prefer to spell the names out, as they don't have unambiguous
   abbreviations.

[hollasch]

Just found out that we need the ray() {} constructor, because we have uninitialized declarations (later assigned), and the compiler doesn't automatically provide the default constructor. You'd need to explicitly include ray() = default; to get that.

[hollasch]

Also, out of curiosity I tried just making all member variables const, but that leaves you with the inability to assign new values to ray variables. Guess that's why you don't often see that pattern.

[trevordblack]

We removed all structs from the codebase.

We did it for good reason

[hollasch]

? The only thing struct does is default access to public. Flipping to class doesn't change any of the above.

[trevordblack]

I don't remember the specific conversation (and I don't really want to go looking for it), but we decided that we should unify as either all struct or all class.

The c++-ism is pretty exclusive to that language.

The specification differences between structs and classes are incredibly minor (and not a good interview question!), but they imply legions.
I don't want to needlessly expose readers to the implication.

For some reason I am feeling very strong about this one.

[trevordblack]

I am perfectly content with:

class ray {
public:
    point3 origin;
    vec3 direction;
    double time;

    ray(const point3& o, const vec3& d) : origin(o), direction(d), time(0) {}
    ray(const point3& o, const vec3& d, double t) : origin(o), direction(d), time(t) {}

    point3 at(double t) const {
        return origin + t*direction;
    }
};

[hollasch]

Well, I look at that and see a struct, but I am feeling very not strong about this one. Win win! The only change to the above is that we need to add back the ray() {} constructor.

[hollasch]

I think it makes sense to revisit this across all of our classes/structs. My proposal:

• If no member variables are directly accessed/set, then we should just flip them to private uniformly.
• For simple primitives that are more like structs with methods, just move the data to the top of the class (immediately following public:), and eliminate any trivial getters. Such classes should be robust against any single member variable modification (no zombie/invalid states).
• For more complicated classes, particularly those where member variable state is not trivial, work to make the member variables private. If absolutely necessary (fingers crossed that this doesn't happen), split the member variables as appropriate to public and private sections.

[hollasch]

So the camera class is a useful case where member variables are split into public and private sections. I like the division here, as it's clear which members are more like a public API than the others, which manage state (set in camera::initialize(...)).

One question here is whether the public member variables should be moved to the top of the class. I'm beginning to think they should be.

[hollasch]

Came back review our v4 issues, planning to argue for what we've already apparently agreed upon. Settable data at the top, followed by the public API. Private data and methods follow. Sweet.