6
\$\begingroup\$

I'm trying to follow the Raytracing in One Weekend book, and I found that he uses the same Vec3 class for everything - colors, coordinates, direction, and more:

struct Vec3 { float x, y, z };

He also overloads every possible operator of the Vec3 class to make it convenient to use, and adds plenty of member functions (length, distance_to, etc). While this definitely reduces the amount of code one has to write, it isn't terribly safe, and allows for things doesn't make sense (why would you want a distance_to between two Colors?). I was trying to fix that.

I defined a base Vector type:

struct Vector {
  Vector(float, float, float);

  float x;
  float y;
  float z;

  float length() const;
};

According to the Wikipedia page, there can be 2 types of vectors - bound vectors (goes from point A to point B) and free vectors (the particular point is of no significance, only the magnitude and direction are). I defined these types too, but with very limited operators.

Notice how you can add a FreeVector to a BoundVector to give a BoundVector, but you cannot add 2 BoundVectors:

struct BoundVector : public Vector {
  BoundVector(float, float, float);

  BoundVector& operator+=(const FreeVector&);
  BoundVector& operator-=(const FreeVector&);

  FreeVector operator-(const BoundVector&) const;
};

struct FreeVector : public Vector {
  FreeVector(float, float, float);
  explicit FreeVector(const UnitVector&);

  FreeVector& operator+=(const FreeVector&);
  FreeVector& operator-=(const FreeVector&);

  FreeVector& operator*=(float);
  FreeVector& operator/=(float);

  UnitVector unit() const;
  float dot(const FreeVector&) const;
};

BoundVector operator+(BoundVector, const FreeVector&);
BoundVector operator-(BoundVector, const FreeVector&);

FreeVector operator+(FreeVector, const FreeVector&);
FreeVector operator-(FreeVector, const FreeVector&);

FreeVector operator*(FreeVector, float);
FreeVector operator/(FreeVector, float);

I would have liked the UnitVector class to extend the FreeVector (or even the Vector) class, but I can't, since the implementation is completely different. For starters, to ensure that it is guaranteed to be a unit vector (x*x + y*y + z*z == 1) I have to make all members const:

struct UnitVector {
  UnitVector(float, float, float);
  explicit UnitVector(const FreeVector&);

  const float x;
  const float y;
  const float z;

  FreeVector operator*(float) const;
  FreeVector operator/(float) const;

 private:
  UnitVector(float, float, float, float);
};

UnitVector::UnitVector(const float x, const float y, const float z)
  : UnitVector(x, y, z, std::sqrt(x * x + y * y + z * z)) {}

UnitVector::UnitVector(const FreeVector& v) : UnitVector(v.x, v.y, v.z) {}

FreeVector UnitVector::operator*(const float k) const {
  return FreeVector(x * k, y * k, z * k);
}

FreeVector UnitVector::operator/(const float k) const {
  return FreeVector(x / k, y / k, z / k);
}

UnitVector::UnitVector(const float x, const float y, const float z, const float r)
  : x{x / r}, y{y / r}, z{z / r} {}

UnitVectors allow you to define directions in a much better manner:

struct Ray {
  BoundVector source;
  UnitVector direction;

  BoundVector parametric_eq(float) const;
};

BoundVector Ray::parametric_eq(const float t) const {
  return source + direction * t;
}

However, this is not all sunshine and roses, as it sometimes results in very ugly-looking static_casts:

struct Lambertian : public Material {
  FreeVector albedo;
  std::optional<Scatter> scatter(const Ray&, const Strike&) const override;
};

FreeVector random_in_unit_sphere() {
  std::random_device r;
  std::default_random_engine gen(r());
  std::uniform_real_distribution<float> distribution(0, 1);

  while (true) {
    const FreeVector v(distribution(gen), distribution(gen), distribution(gen));
    if (v.length() < 1) return v;
  }
}

std::optional<Scatter> Lambertian::scatter(const Ray& ray,
                                           const Strike& strike) const {
  return Scatter{.attenuation = albedo,
                 .scattered = Ray{.source = strike.point,
                                  .direction = static_cast<UnitVector>(
                                      static_cast<FreeVector>(strike.normal) +
                                      random_in_unit_sphere())}};
}

Note that the std::optional is added here because the material may choose to absorb the Ray completely with some probability, and hence not scatter it at all.

Is there a way to reduce the number of static_casts in the last example (or at least the overhead due to them)?

Any other feedback, comments and nitpickings are also welcomed.

\$\endgroup\$
  • \$\begingroup\$ designated initializers are a very nice feature of C++20... but we're in 2018! I'm not even sure which compilers do support it already \$\endgroup\$ – papagaga Nov 24 '18 at 1:57
  • \$\begingroup\$ Strangely, CMake didn't support it so I couldn't use set(CMAKE_CXX_STANDARD 2a), but GCC stopped complaining once I added set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++2a) to the CMakeLists.txt file. \$\endgroup\$ – ajeetdsouza Nov 24 '18 at 2:02
  • 1
    \$\begingroup\$ From a purely mathematical point of view, a bounded vector is in fact a pair of two free vectors. Try to model your classes with that in mind. A mathematical point of view could be very helpful. \$\endgroup\$ – vnp Nov 24 '18 at 6:25
  • \$\begingroup\$ I did explore that option too, but I eventually settled for this (all bound vectors relative to the origin) because the bounded vectors were more space efficient (and even a little boost in efficiency really matters when you're doing raytracing). Although, in most cases, they are bound to the origin, in some cases in the book itself you come across a situation where the 2 free vector representation would have been helpful (such as the Camera class). In such cases, I used one bound vector position and a free vector to denote the direction taken from there. \$\endgroup\$ – ajeetdsouza Nov 24 '18 at 11:53
  • 1
    \$\begingroup\$ That's fine - I didn't want to answer if you needed to edit the question. BTW, there are subtleties to even such simple functions - I've seen implementations that do more copying than is necessary, for example. \$\endgroup\$ – Toby Speight Nov 29 '18 at 8:55
3
\$\begingroup\$

Consider a typedef instead of hardcoding float everywhere

Whilst the current use-case requires float, we might want to convert to a template in future, so that we could use with double or long double. We can ease that by defining a type alias, so there's less work to change when we do so:

using value_type = float;

Consider implementing unary minus

If we implement unary operator-() for FreeVector, we can use that to implement subtraction in terms of addition (without loss of efficiency).

Use standard Euclidean-length function

Instead of writing std::sqrt(x * x + y * y + z * z), we could use std::hypot() instead for an algorithm that remains stable for very large and very small values. Since C++17, there's an overload that takes all three inputs:

float Vector::length() const {
    return std::hypot(x, y, z);
}

Could UnitVector be implemented using FreeVector?

Instead of having constant members (which of course inhibits assignment operators), perhaps it's worthwhile having a private FreeVector member in a UnitVector, and forwarding access? Something like this:

struct UnitVector
{
    UnitVector(float x, float y, float z);
    explicit UnitVector(const FreeVector& other);

    operator const FreeVector&() const { return v; }

private:
    FreeVector v;
};

UnitVector::UnitVector(const float x, const float y, const float z)
    : UnitVector{FreeVector{x, y, z}}
{}

UnitVector::UnitVector(const FreeVector& v)
    : v{v / v.length()}
{}

Note that I've provided a non-explicit conversion to FreeVector, as a replacement for the FreeVector(UnitVector) constructor. This means that we no longer need to implement arithmetic operators for UnitVector, as they will simply promote to FreeVector in such contexts.

Document the behaviour of indefinite unit vectors

What happens when we try to create a unit vector when its length is zero? I think we end up with all NaNs - we should make it clearer to users what they should expect (without them having to read the implementation). We might even need an operator bool() that tests whether any element of the vector is NaN.

Style - give names to formal parameters

It's subjective, but I think it makes an interface easier to read if the formal parameters have names, particularly when there are multiple arguments of the same type.

Kudos

I often forget to give this, so: well done on good use of const and explicit; I was pleased to see that the binary operations take one argument by copy and one by const-ref, so the copy can be modified and returned.

\$\endgroup\$
  • \$\begingroup\$ While I had considered the type alias, I wasn't sure whether to add float or value_type to parameters and return types. In case of a change, won't there be external functions that rely on having to work with a float that are suddenly forced to use whatever the new value_type is aliased to? Also, is value_type a standard naming practice, and should I be namespacing it to geometry::value_type? If yes, should I namespace everything else too? \$\endgroup\$ – ajeetdsouza Dec 1 '18 at 4:43
  • \$\begingroup\$ You're right - the UnitVector divison by zero does give NaN. How would you suggest handling this? \$\endgroup\$ – ajeetdsouza Dec 1 '18 at 4:45
  • 1
    \$\begingroup\$ My suggestion is to provide the means to test whether the UnitVector is valid, and then leave it up to the user to check. But you could choose to throw an exception instead. You'll have to choose which best suits your users. As for value_type, as long as that's an alias of float, no change required by users - even when Vector becomes a template (provided the type defaults to float). \$\endgroup\$ – Toby Speight Dec 3 '18 at 9:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.