Suggestion: override for derived classes

[D-K-E]

This might seem rather unnecessary, since if the reader is careful, overriding methods of the base class shouldn't be a problem. However I noticed that it took me quite a while to track down the bugs related to this problem. An override qualifier can add an extra layer of warning for such bugs.
I am basically suggesting to turn this:

class Material {
public:
  virtual bool scatter(const Ray &r_in, const HitRecord &rec,
                       ScatterRecord &srec) const {
    return false;
  }
};
class lambertian : public material {
public:
    virtual bool scatter(const ray &r_in, const hit_record &rec,
                       scatter_record &srec) const  {}
public:
  shared_ptr<Texture> albedo;
};

to this:

class Material {
public:
  virtual bool scatter(const Ray &r_in, const HitRecord &rec,
                       ScatterRecord &srec) const {
    return false;
  }
};
class lambertian : public material {
public:
    bool scatter(const ray &r_in, const hit_record &rec,
                       scatter_record &srec) const override {...}
public:
  shared_ptr<Texture> albedo;
};

[hollasch]

I like it.

[D-K-E]

I can do a PR if you would like to ?

[hollasch]

Sure. Let's go ahead and drop this in the dev-minor branch for our v3.2.0 release.

You will need to update all code and all instances in the three books. In addition, please work with @trevordblack, as he is in the middle of running through book 2 for an image update pass, updating numerous code issues as he goes.

[hollasch]

Also, I keep getting confused with real names versus GitHub user IDs — have you been added to the acknowledgements yet? (https://raytracing.github.io/books/acknowledgments.md.html). At this point, I think I should be linking a lot of these names to associated GitHub user pages.

[D-K-E]

I tried adding the override to all code related to "In One Weekend" and it compiled. I'll wait for @trevordblack before proceeding with the other two books. Yes I'd been added to acknowledgements: Kaan Eraslan

[trevordblack]

The big thing here is that the override keyword would need to be added to the text of the book as these functions are added in.

All code changes need to go into the book. We've not been diligent about this in the past, but going forward, that's our plan.

If you want to tread forward, you can try to add override to both the source and text, or you can just do the source, or you can just do the text.

What are your thoughts? Steve started the renders for book2, I'm going to finish up, but in going through the renders we're literally copying the inline source from the text into the code to make the renders.

[D-K-E]

In that case I'll add both to code and to text of book 3, since it is not rendered yet, and to the code of book 1 as well. I hesitate to touch upon the text of book 1 since it is quite popular. For book 2, I can change the text, so that it is copied to source during rendering. How does that sound ?

[hollasch]

You'll be making changes to our minor-change branch, not to the live branch. We'll have ample time to review your changes and make sure everything's coordinated. Also note that when we talk about the "text of the book", we are referring to the code listing fragments in each book.

You're correct that there shouldn't be any issues with either book 1 or book 3. I'd recommend that you also make changes to book 2. Trevor can then rebase his progression branch onto the updated dev-minor branch when he's ready.

[trevordblack]

Everything that affects this issue has been fixed for dev-minor. @D-K-E are you still interested in owning this? We are hoping to get 3.2.0 out soonish.

[D-K-E]

I am still up to adding the qualifier, but I have noticed that dev-minor was quite active in the recent weeks, so I waited for everything to settle down a bit. But if you think it is not necessary to add the qualifier, I can drop the idea as well

[hollasch]

Still think it's a good, easy idea. Go for it.

[hollasch]

Any chance you could get this done this weekend? I'm hoping to wrap up my two remaining issues and get v3.2.0 out the door.

[hollasch]

Oops. ↑ @D-K-E

[trevordblack]

Thinking out loud, here, what's the viability and utility of turning some of the parent functions into purely virtual functions?

[hollasch]

material::scatter() is a pure virtual. Which overridden functions are not? Doing a survey to see what's in play with this...

[hollasch]

class hittable (48 overrides)
    : class box
    : class bvh_node
    : class constant_medium
    : class flip_face
    : class hittable_list
    : class moving_sphere
    : class rotate_y
    : class sphere
    : class translate
    : class xy_rect
    : class xz_rect
    : class yz_rect
    + virtual hit() = 0
    + virtual bounding_box() = 0
    - virtual pdf_value()
    - virtual random()

class material (20 overrides)
    : class dielectric
    : class diffuse_light
    : class isotropic
    : class lambertian
    : class metal
    + virtual scatter() = 0
    - virtual scatter()
    - virtual emitted()
    - virtual scattering_pdf()

class pdf (6 overrides)
    : class cosine_pdf
    : class hittable_pdf
    : class mixture_pdf
    + virtual value() = 0
    + virtual generate() = 0

class texture (4 overrides)
    : class checker_texture
    : class image_texture
    : class noise_texture
    : class solid_color
    + virtual value() = 0

So about five non-pure-virtual functions. Note that material::scatter() starts out pure virtual, then gets a default implementation later on.

78 overrides total, by my count.

Still, I think that flagging everything with override still helps the reader.

[hollasch]

(This class hierarchy is pretty helpful. Might be nice to squirrel that away somewhere.)

[hollasch]

I've come this far — I'm just going to whip this out...

[D-K-E]

@hollasch or I can get it done in weekend ... Your wish is my command ...

[hollasch]

This just in: using the override specifier caught a bug in our source. isotropic::scatter had a function signature different from the superclass. Score one for Kaan!

[hollasch]

Good grief. Book 2 has a material::scatter() function that takes four arguments. Book 3 bumps the argument count to five. Book 3 source has a material::scatter() function that takes three arguments (and is actually called with three in ray_color()).

So the code for book three actually used the default material::scatter() method for isotropic materials.

Filed as issue #669.

[trevordblack]

It looks like the only non-pure virtual function that isn't introduced in book 3 is emitted. I think we should consider removing all non-pure virtual functions. If so, emitted should go in 3.2.0

[hollasch]

@trevordblack — I'm not understanding why default implementations are bad. Better than copy-pasting implementations, and I'm definitely not up for laying the groundwork for more whack-a-mole branching updates.

[trevordblack]

Hmm. I guess this was the reason the override keyword exists.

There's nothing wrong with default implementations. I'm just used to having interface style purely virtual classes.
No reason to adhere to it.