5
\$\begingroup\$

Problem Statement/Context:

Represent a ray, given a origin and direction. Besides this a min_t and max_t value are defined. These values define a distance along the ray and make it possible to describe an intervall on the ray. The propose of the ray class is to support 2d ray tracing (a 2D ray tracer visualization tool) as seen on the following picture:

enter image description here

Implementation:

My proposed C++17 solution:

#pragma once
#ifndef Flatland_Ray_ceb9b0b4_4236_4bfe_b918_11a02c72ad7c_h
#define Flatland_Ray_ceb9b0b4_4236_4bfe_b918_11a02c72ad7c_h

#include "flatland/core/namespace.h"
#include "flatland/math/point.h"
#include "flatland/math/vector.h"

#include <iostream>
#include <type_traits>

FLATLAND_BEGIN_NAMESPACE

template <typename PointType, typename VectorType, typename ScalarType, unsigned int Dimension>
class Ray {
public:
    Ray(const PointType &origin, const VectorType &direction, const ScalarType min_t, const ScalarType max_t)
            : origin(origin), direction(direction), min_t(min_t), max_t(max_t) {
    }

    PointType operator()(const float t) const {
        assert(isDirectionVectorNormalized());
        return origin + t * direction;
    }

    bool isDirectionVectorNormalized() const {
        const ScalarType epsilon = static_cast<ScalarType>(0.001);
        return  direction.norm() - static_cast<ScalarType>(1.0) < epsilon;
    }

public:
    PointType origin;
    VectorType direction;
    ScalarType min_t;
    ScalarType max_t;
    enum { dimension = Dimension};
};

template <typename ScalarType>
using Ray2 = Ray<Point2<ScalarType>, Vector2<ScalarType>, ScalarType, 2>;

template <typename ScalarType>
using Ray3 = Ray<Point3<ScalarType>, Vector3<ScalarType>, ScalarType, 3>;

using Ray2f = Ray2<float>;
using Ray2d = Ray2<double>;
using Ray3f = Ray3<float>;
using Ray3d = Ray3<double>;

namespace internal {
    template <typename ScalarType>
    std::string convertTypeToString() {
        std::string typeAsString = "UnknownType";
        if(std::is_same<ScalarType,int>::value)
            typeAsString = "i";
        if(std::is_same<ScalarType,float>::value)
            typeAsString = "f";
        if(std::is_same<ScalarType,double>::value)
            typeAsString = "d";
        return typeAsString;
    }

    template <unsigned int Dimension>
    std::string dimensionAsString() {
        std::string dimensionAsString = "UnknownDimension";
        if(Dimension == 2)
            dimensionAsString = "2";
        if(Dimension == 3)
            dimensionAsString = "3";

        return dimensionAsString;
    }
}

template <typename PointType, typename VectorType, typename ScalarType, unsigned int Dimension>
std::ostream &operator<<(std::ostream &os, const Ray<PointType, VectorType, ScalarType, Dimension> &r) {
    auto typeAsString = internal::convertTypeToString<ScalarType>();
    auto dimensionAsString = internal::dimensionAsString<Dimension>();

    os << "Ray" << dimensionAsString << typeAsString << "[" << std::endl
       << "  origin = " << r.origin << "," << std::endl
       << "  direction = " << r.direction << "," << std::endl
       << "  min_t = " << r.min_t << "," << std::endl
       << "  max_t = " << r.max_t << "" << std::endl
       << "]" << std::endl;

    return os;
}

FLATLAND_END_NAMESPACE

#endif // end define Flatland_Ray_ceb9b0b4_4236_4bfe_b918_11a02c72ad7c_h

My Tests:

#include "flatland/math/ray.h"

#include <gmock/gmock.h>

using namespace Flatland;

TEST(Ray2f, GivenDirectionOriginMaxMinT_WhenRayIsInitialized_ThenInitzalizedRayValues) {
    Point2f origin(0.0f, 0.0f);
    Vector2f direction(1.0f, 0.0f);

    Ray2f r(origin, direction, 1.0f, 10.0f);

    EXPECT_THAT(r.origin.x(), 0.0f);
    EXPECT_THAT(r.origin.y(), 0.0f);
    EXPECT_THAT(r.direction.x(), 1.0f);
    EXPECT_THAT(r.direction.y(), 0.0f);
    EXPECT_THAT(r.min_t, 1.0f);
    EXPECT_THAT(r.max_t, 10.0f);
}

TEST(Ray2f, GivenNormalizedRay_WhenCheckingIfRayIsNormalized_ThenNormalizedIsTrue) {
    Point2f origin(0.0f, 0.0f);
    Vector2f direction(1.0f, 0.0f);

    Ray2f r(origin, direction, 1.0f, 10.0f);

    EXPECT_TRUE(r.isDirectionVectorNormalized());
}

TEST(Ray2f, GivenNonNormalizedRay_WhenCheckingIfRayIsNormalized_ThenNormalizedIsFalse) {
    Point2f origin(0.0f, 0.0f);
    Vector2f direction(2.0f, 0.0f);

    Ray2f r(origin, direction, 1.0f, 10.0f);

    EXPECT_FALSE(r.isDirectionVectorNormalized());
}

TEST(Ray2f, GivenANormalizedRay_WhenDeterminPointAtMinAndMaxT_ThenExpectStartAndEndPoint) {
    Point2f origin(0.0f, 0.0f);
    Vector2f direction(1.0f, 0.0f);

    Ray2f r(origin, direction, 1.0f, 10.0f);

    Point2f start = r(r.min_t);
    Point2f end = r(r.max_t);

    EXPECT_THAT(start.x(), 1.0f);
    EXPECT_THAT(start.y(), 0.0f);
    EXPECT_THAT(end.x(), 10.0f);
    EXPECT_THAT(end.y(), 0.0f);
}

TEST(Ray2f, GivenARay_WhenPrintedToStdOutput_ExpectRayAsStringRepresentation) {
    testing::internal::CaptureStdout();

    Point2f origin(0.0f, 0.0f);
    Vector2f direction(1.0f, 0.0f);

    Ray2f r(origin, direction, 1.0f, 10.0f);

    std::cout << r;

    std::string output = testing::internal::GetCapturedStdout();

    EXPECT_THAT(output, ::testing::HasSubstr("Ray2f["));
    EXPECT_THAT(output, ::testing::HasSubstr("origin ="));
    EXPECT_THAT(output, ::testing::HasSubstr("direction ="));
    EXPECT_THAT(output, ::testing::HasSubstr("min_t = 1"));
    EXPECT_THAT(output, ::testing::HasSubstr("max_t = 10"));
    EXPECT_THAT(output, ::testing::HasSubstr("]"));
}

TEST(Ray2d, GivenARay2d_WhenPrintedToStdOutput_ExpectRayTypeRay2d) {
    testing::internal::CaptureStdout();

    Point2d origin(0.0, 0.0);
    Vector2d direction(1.0, 0.0);
    Ray2d r(origin, direction, 1.0, 10.0);

    std::cout << r;

    std::string output = testing::internal::GetCapturedStdout();

    EXPECT_THAT(output, ::testing::HasSubstr("Ray2d["));
    EXPECT_THAT(output, ::testing::HasSubstr("origin ="));
    EXPECT_THAT(output, ::testing::HasSubstr("direction ="));
    EXPECT_THAT(output, ::testing::HasSubstr("min_t = 1"));
    EXPECT_THAT(output, ::testing::HasSubstr("max_t = 10"));
    EXPECT_THAT(output, ::testing::HasSubstr("]"));
}

TEST(Ray3f, GivenARay3f_WhenPrintedToStdOutput_ExpectRayTypeRay3f) {
    testing::internal::CaptureStdout();

    Point3f origin(0.0f, 0.0f, 0.0f);
    Vector3f direction(1.0f, 0.0f, 0.0f);
    Ray3f r(origin, direction, 1.0f, 10.0f);

    std::cout << r;

    std::string output = testing::internal::GetCapturedStdout();

    EXPECT_THAT(output, ::testing::HasSubstr("Ray3f["));
    EXPECT_THAT(output, ::testing::HasSubstr("origin ="));
    EXPECT_THAT(output, ::testing::HasSubstr("direction ="));
    EXPECT_THAT(output, ::testing::HasSubstr("min_t = 1"));
    EXPECT_THAT(output, ::testing::HasSubstr("max_t = 10"));
    EXPECT_THAT(output, ::testing::HasSubstr("]"));
}

TEST(internal_convertTypToString, GivenIntegralType_WhenConverTypeToString_ThenShortStringRepersentationExpected) {
    EXPECT_THAT(internal::convertTypeToString<int>(), ::testing::HasSubstr("i"));
    EXPECT_THAT(internal::convertTypeToString<float>(), ::testing::HasSubstr("f"));
    EXPECT_THAT(internal::convertTypeToString<double>(), ::testing::HasSubstr("d"));
}

TEST(internal_dimensionAsString, GivenRayDimension_WhenConvertingToStringRepresentation_ThenExpectValidNumber) {
    EXPECT_THAT(internal::dimensionAsString<2u>(), ::testing::HasSubstr("2"));
    EXPECT_THAT(internal::dimensionAsString<3u>(), ::testing::HasSubstr("3"));
}

Questions:

Implementation

  • From a C++17 perspective: Are there more modern features of the language that I should use?
  • I was thinking about changing Ray in something like template <typename ScalarType, int Dim> class Ray and then use PointType<ScalarType, Dim> in the class. But I think that makes it harder if I am going to change PointType in the future (currently Eigen is used under thee hood - maybe I will switch later to something else) - and opinons about this?

Testing

  • Do you think there is to much duplication in the tests?
  • Are there any important edge cases that I should also consider?
  • Any other hints? suggestions?

Do you have other feedback, improvements for the implementation and testing?v

\$\endgroup\$
5
\$\begingroup\$

Keep it simple

Use the KISS principle, and don't make things more complicated than necessary. Using more language features should not be a goal in itself. Also, don't add more things in a class than necessary. Why do you keep track of dimension? Nothing inside the class makes use of it. The dimension is a property of PointType and VectorType, so it's already there in some way. Similarly, does it make sense to specify ScalarType differently from the underlying scalar type of PointType and VectorType?

class Ray has nothing which is private. Consider making it a struct. Also, as it is right now you don't even need to explicitly create a constructor for it. Here's the struct with some of the unnecessary baggage removed:

template <typename PointType, typename VectorType>
struct Ray {
    using ScalarType = PointType::value_type;

    PointType operator()(const float t) const {
        assert(isDirectionVectorNormalized());
        return origin + t * direction;
    }

    bool isDirectionVectorNormalized() const {
        const ScalarType epsilon{0.001};
        return direction.norm() - ScalarType{1.0} < epsilon;
    }

    PointType origin{};
    VectorType direction{};
    ScalarType min_t{};
    ScalarType max_t{};
};

I assume here that PointType contains information about the underlying type of the value it stores. I also used the fact that we can construct a value of a type instead of static_cast<>ing it. It saves a bit of typing.

So how to get the dimensionality of a Ray? Again I would just ensure your PointType contains that information, so to get the dimension of a Ray you just write something like:

Ray<...> ray = ...;
auto dimension = ray.origin.dimension;

Prevent being able to use different types for origin and direction

What would it mean to create a Ray<Point2<float>, Vector3<double>>? It doesn't make sense. Either I would just use one template type:

template <typename VectorType>
struct Ray {
    ...
    VectorType origin;
    VectorType direction;
};

Or use some SFINAE trick to enforce that the dimension and underlying scalar type is the same.

Enforce normalization

If you should not construct a Ray with an unnormalized direction, then instead of checking this at runtime with an assert(), I would just write a constructor that normalizes direction:

template <...>
struct Ray {
    Ray(const PointType &origin, const VectorType &direction, ...)
        : origin(origin)
        , direction(direction.normalized())
        , ...

Also note that your function isDirectionNormalized() forgets to take the absolute value of the difference. Floating point comparisons are hard, enforcing normalization sidesteps this issue.

About testing

Are you doing too much testing? Maybe. I would honestly not write test cases that just check whether the constructor has properly copied values into member variables, as that is such a basic language function that if that doesn't work, you have bigger problems. But if you would have a constructor that normalizes the direction, then I would definitely test that.

When testing the std::ostream operator of Ray, I would not write to std::cout and then have your testing library jump through hoops to get the standard output back, but rather I would just create a std::ostringstream to capture the output in. And, instead of checking for the presence of substrings, I would verify that the whole string is exactly as expected:

Ray2f r({1.0f, 2.0f}, {3.0f, 4.0f}, 5.0f, 6.0f);
std::ostringstream os;
os << r;

EXPECT_THAT(os.str(), std::string(R"(Ray2f[
  origin = {1.0, 2.0},
  direciton = {3.0, 4.0},
  min_t = 5.0,
  max_t = 6.0
])"));
\$\endgroup\$
2
\$\begingroup\$

Your implementations of convertTypeToString and dimensionAsString are a little inefficient and unnatural. First, there is no need to use a variable inside the function which you assign to and finally return. Second, you should not represent an unknown or nonacceptable value as a string. Think about it from a user perspective - it's not nice (nor robust to change) to have to compare against some "magic value" like "UnknownType". Moreover, you already have the type information at compile-time, so why wouldn't you use this there?

For example, instead, we could write:

template <typename ScalarType>
std::string convertTypeToString();

template <>
std::string convertTypeToString<int>() {
    return "i";
}

template <>
std::string convertTypeToString<float>() {
    return "f";
}

template <>
std::string convertTypeToString<double>() {
    return "d";
}

And otherwise you'll hit error while compiling if you tried to do say convertTypeToString<bool>();. You could also use a static_assert to output a more meaningful error message if you wanted to, like "not implemented" or so.

You might as well make this fully compile-time by using constexpr and classes:

template <typename ScalarType>
struct convertTypeToString;

template <>
struct convertTypeToString<int> {
    constexpr static char value = 'i';
};

// Later on, you just do
convertTypeToString<int>::value;
\$\endgroup\$

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.