Refactor aarect.h

[hollasch]

There are two primary problems with this code:

1. There's a lot of duplicated code, differing only in the axes used. These could somehow derive from a common base class, with appropriate virtuals.
2. Only the xz_rect class has pdf_value() and random() member functions.

[trevordblack]

The quad stuff is my least favorite part of all three books. This can definitely be organized in a simpler way. At present, my plan for chapter 3 of the ethereal book 4 is to rip out a lot of work and replace things with SRT matrices

…

On Wed, Dec 4, 2019, 17:24 Steve Hollasch ***@***.***> wrote: There are two primary problems with this code: 1. There's a lot of duplicated code, differing only in the axes used. These could somehow derive from a common base class, with appropriate virtuals. 2. Only the xz_rect class has pdf_value() and random() member functions. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#292?email_source=notifications&email_token=ACNZVKGN3AAFW23QJRZRKQ3QXBJ3LA5CNFSM4JVSQWWKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H6F6RMQ>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNZVKGCRHQINDOPXYUNLQTQXBJ3LANCNFSM4JVSQWWA> .

[hollasch]

I totally agree with you in spirit, but that's a big head twist for many folks. A lot also get stuck on SRT as the way you do transforms, and are never taught to then make the leap to constructing basis matrices to do transforms (though look-at-from is a more accessible form, and I see that taught for cameras). But just thinking of local transforms and intermediate model space is a book in itself.

[trevordblack]

I'm not saying do SRT here, just in another volume. My only demand is that the solution here not be templates. I am willing to die on this hill.

…

On Wed, Dec 4, 2019, 18:01 Steve Hollasch ***@***.***> wrote: I totally agree with you in spirit, but that's a big head twist for many folks. A lot also get stuck on SRT as the way you do transforms, and are never taught to then make the leap to constructing basis matrices to do transforms (though look-at-from is a more accessible form, and I see that taught for cameras). But just thinking of local transforms and intermediate model space is a book in itself. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#292?email_source=notifications&email_token=ACNZVKABGFPE55KZHDH2SEDQXBOF3A5CNFSM4JVSQWWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF7GMKI#issuecomment-561931817>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNZVKEGSUFZWAPCUA6NWU3QXBOF3ANCNFSM4JVSQWWA> .

[D-K-E]

As per mentioned in #545

I have noticed that there is a quite a bit of code duplication going on in the aarect.h file.
I would like to suggest the following implementation. It adds a simple struct for holding axis information but keeps the entire file under 120 lines.

struct AxisInfo {
  double aligned1;
  double aligned2;
  double notAligned;
};

class AaRect : public Hittable {
protected:
  vec3 axis_normal;
  double a0, a1, // aligned1
      b0, b1;    // aligned2
  AxisInfo ax;

public:
  double k;
  shared_ptr<Material> mat_ptr;

public:
  AaRect() {}
  AaRect(double a_0, double a_1, double b_0, double b_1, double _k,
         shared_ptr<Material> mat, vec3 anormal)
      : a0(a_0), a1(a_1), b0(b_0), b1(b_1), k(_k), mat_ptr(mat),
        axis_normal(anormal) {
    if (anormal.z == 1.0) {
      ax.aligned1 = 0;
      ax.aligned2 = 1;
      ax.notAligned = 2;
    } else if (anormal.x == 1.0) {
      ax.aligned1 = 1;
      ax.aligned2 = 2;
      ax.notAligned = 0;
    } else if (anormal.y == 1.0) {
      ax.aligned1 = 0;
      ax.aligned2 = 2;
      ax.notAligned = 1;
    }
  }
  virtual bool hit(const Ray &r, double t0, double t1, HitRecord &rec) const {
    /*
       point of intersection satisfies
       both P = O + D*m Point = Origin + Direction * magnitude
       and
       x0 < x_i < x1 y0 < y_i < y1 y0,y1 and x0,x1 being the limits of
       rectangle
     */
    auto t = (k - r.orig()[ax.notAligned]) / r.dir()[ax.notAligned];
    if (t < t0 || t > t1)
      return false;
    double a = r.orig()[ax.aligned1] + t * r.dir()[ax.aligned1];
    double b = r.orig()[ax.aligned2] + t * r.dir()[ax.aligned2];
    bool c1 = a0 < a and a < a1;
    bool c2 = b0 < b and b < b1;
    if ((c1 and c2) == false) {
      return false;
    }
    rec.u = (a - a0) / (a1 - a0);
    rec.v = (b - b0) / (b1 - b0);
    rec.dist = t;
    vec3 outward_normal = axis_normal;
    rec.set_face_normal(r, outward_normal);
    rec.mat_ptr = mat_ptr;
    rec.point = r.at(t);
    return true;
  }
  virtual bool bounding_box(double t0, double t1, Aabb &output_box) const {
    // The bounding box must have non-zero width in each dimension, so pad the Z
    // dimension a small amount.
    point3 p1, p2;
    // choose points with axis
    if (ax.notAligned == 2) {
      p1 = point3(a0, b0, k - 0.0001);
      p2 = point3(a1, b1, k + 0.0001);
    } else if (ax.notAligned == 1) {
      p1 = point3(a0, k - 0.0001, b0);
      p2 = point3(a1, k + 0.0001, b1);
    } else if (ax.notAligned == 0) {
      p1 = point3(k - 0.0001, a0, b0);
      p2 = point3(k + 0.0001, a1, b1);
    }
    output_box = Aabb(p1, p2);
    return true;
  }
};

class XYRect : public AaRect {
public:
  double x0, x1, y0, y1;

public:
  XYRect() {}
  XYRect(double _x0, double _x1, double _y0, double _y1, double _k,
         shared_ptr<Material> mat)
      : AaRect(_x0, _x1, _y0, _y1, _k, mat, vec3(0, 0, 1)), x0(_x0), x1(_x1),
        y0(_y0), y1(_y1) {}
};
class XZRect : public AaRect {
public:
  double x0, x1, z0, z1;

public:
  XZRect() {}
  XZRect(double _x0, double _x1, double _z0, double _z1, double _k,
         shared_ptr<Material> mat)
      : AaRect(_x0, _x1, _z0, _z1, _k, mat, vec3(0, 1, 0)), x0(_x0), x1(_x1),
        z0(_z0), z1(_z1) {}
};
class YZRect : public AaRect {
public:
  double y0, y1, z0, z1;

public:
  YZRect() {}
  YZRect(double _y0, double _y1, double _z0, double _z1, double _k,
         shared_ptr<Material> mat)
      : AaRect(_y0, _y1, _z0, _z1, _k, mat, vec3(1, 0, 0)), y0(_y0), y1(_y1),
        z0(_z0), z1(_z1) {}
};

I can also confirm that I had been able to render cornell box with this implementation.

[hollasch]

Thanks @D-K-E ! We'll give it a spin later on. If you want to prioritize this, we're currently focused on getting v3.2.0 out, which is mostly a figure/image update. If you'd like to do a PR, we might get this into that release as well. If not, no problem; we'll probably do this for v3.3.0.

[trevordblack]

I updated this to 3.2.0

It's a smallish change, and it fits neatly into the existing changes made for 3.2.0
v3.3.0 is shaping up to mostly a change in the text and renders of book 3.

So I'd prefer to knock this out for this release.

@D-K-E If you want to throw up a PR, we're angling to get 3.2.0 out around July 10th...

Otherwise, I can take the suggestions you've made here and roll with them

[D-K-E]

@trevordblack Feel free to take these as suggestions and roll with them. While I conceived this, I was about to finish the second book. After finishing all 3 books, I am not really sure if it fits with other books. Quads have couple of extra methods in the third book that I was not aware while I conceived this.

[trevordblack]

The third book just adds pdf_value.
That's not too bad.

However, I'm going to push this issue back. Your solution is about as condensed as it can be.
I just worry that it'll be complicating an already fairly dense part of the book.

Need more data

[hollasch]

Fixed in ed119af