Calculation of the normal of bvh::Triangle

[newr5]

Hi,
I'm trying to understand why the normal of bvh::Triangle is calculated differently than the 'standard' way...
By 'standard', I am referring to the way of calculating the normal of a trial that I found described on many internet sources...
If a triangle is given by vertices p0, p1, p2, the standard way states that the normal is calculated as follows:
normal = cross(p1 - p0, p2 - p0)
If I understand correctly, in bvh::Triangle the edges are stored e1 = (p0 - p1) and e2 = (p2 - p0).
In the constructor n is calculated as cross(e1,e2), which makes the normal point in the opposite direction than the normal of the 'standard' case.

Could you please enlighten me on this subject?

[madmann91]

This normal is primarily designed to speed up the intersection routine, not really as a normal you would use for rendering. This normal is what you would get in a left-handed coordinate system, which might not be what you would expect. I think this can be changed by just inverting the two arguments to cross, and changing the sign of the dot products in the intersection routine, but there is a small price to pay in terms of performance, because you now have to perform more operations (I am not sure how much, probably just a few percent, depending on how many intersections you have). I can add an option to do that if you are willing to pay that price.

[madmann91]

Commit 1f2bd69 allows to change the triangle's normal orientation.

[newr5]

Thank you for the quick fix.
In the mean time, I have done a quick benchmark test of the consequence of this change.
The new intersection algorithm is about 3 percent slower.
I don't know if that's acceptable.
The benchmark is only used to time the intersection algorithm, so in normal situations while traversing a proper BVH, the triangle intersection test might not be called so often.
I'm just getting acquainted with BVH's so I cannot judge.
I think you are the best person to judge if it's OK to keep the change or it would be better to revert it.
In any case, thanks again for your efforts!

FYI: here follows part of the code I used to benchmark.
I also keep track of the position where a ray did hit a triangle for comparison with other algorithms.
For the timing comparisons I ran the benchmark program 10 times for old, and 10 times for the new intersection algorithm and then I divided the mean of the timing results. That showed that the new algorithm was 3 % slower.

  typedef double real_t;

  // Create an array of triangles
  bvh::Triangle<real_t> triangle(bvh::Vector3<real_t>( 1.0, -1.0, 1.0),
                                 bvh::Vector3<real_t>(-1.0, -1.0, 1.0),
                                 bvh::Vector3<real_t>(-1.0,  1.0, 1.0));
  int nrOfRays = 10000000;
  std::vector<bvh::Ray<real_t>> rays;

  real_t rayDirX = 1.0;
  real_t rayIncrX = real_t(-2.0)/nrOfRays;


  real_t rayDirY = 1.0;
  real_t rayIncrY = real_t(-3.0)/nrOfRays;


  // Intersect a ray with the data structure
  for (int iRay = 0; iRay < nrOfRays; iRay++)
  {
    rays.emplace_back(bvh::Vector3<real_t>(0.0, 0.0, 0.0), // origin
                      bvh::Vector3<real_t>(rayDirX, rayDirY, 1.0), // direction
                      0.0,                    // minimum distance
                      100.0                   // maximum distance
                      );
    rayDirX += rayIncrX;
    rayDirY += rayIncrY;
  }

  rasp::Timer timer;

  typedef bvh::Triangle<real_t>::Intersection Intersection;


  struct MyIntersection
  {
    MyIntersection(const Intersection& hit, bvh::Triangle<real_t>& triangle)
      : distanceFromOrigin(hit.t)
    {
      hitPosition = triangle.p0 - hit.u * triangle.e1 + hit.v * triangle.e2;
    }

    bvh::Vector3<real_t> hitPosition;
    real_t distanceFromOrigin;
  };
  std::map<int, MyIntersection> intersectionMap;

  timer.start();
  for (int iRay = 0; iRay < nrOfRays; iRay++)
  {
    if (auto hit = triangle.intersect(rays[iRay]))
    {
      auto pair = std::pair<int, MyIntersection>(iRay, MyIntersection(*hit, triangle));
      intersectionMap.insert(std::move(pair));
    }
  }
  timer.stop();

  std::cout << "Triangle_hit using bvh's triangle: " << nrOfRays << " rays: "
            << std::setprecision(3) << std::fixed << timer.elapsedMsec() << " ms" << std::endl;
  std::cout << "NrOfHits: " << intersectionMap.size() << std::endl;

[madmann91]

Well now the change is only enabled if you use bvh::Triangle<Scalar, false>, and it is disabled by default. So I don't think it's a problem since it's now the user's choice, and the API is still compatible with the previous version.