how to make thread safe raycasting in C++?

Question

i am implementing simple plane raycasting in c++ and i used OpenMP for multithreading.
here is the function that goes through all pixels and computes intersections:

 #pragma omp parallel for num_threads(std::thread::hardware_concurrency())
 for (int x = 0; x < camera_.GetWidth(); x++)
 {
    for (uint32_t y = 0; y < camera_.GetHeight(); y++)
    {
      Ray ray(camera_.GetPosition());
      ray.SetDirection(GetDirection(glm::vec2((2.0f * x) / camera_.GetWidth() - 1.0f, (2.0f * y) / camera_.GetHeight() - 1.0f)));

      cogs::Color3f temp = cogs::Color3f(0, 0, 0);

      for (uint16_t p = 0; p < models_.size(); p++)
      {
        //mutex.lock();
        if (models_.at(p)->Intersection(ray))
        {
          ray.SetParamT(models_.at(p)->GetDistanceT());
          temp = models_.at(p)->GetColor() * (camera_.GetLightEnergy() / ray.GetParamT());
        }
        //mutex.unlock();
      }
      renderer.SetPixel(x, y, temp );
    }
  }  


bool Plane::Intersection(Ray &ray)
{

  float dot = glm::dot(ray.GetDirection(), normal_);

  if (dot == 0.0f)
  {
    return false;
  }

  float t = glm::dot(reference_ - ray.GetOrigin(), normal_) / dot;

  if (t <= 0.0001 || t > ray.GetParamT())
  {
    return false;
  }

  SetDistanceT(t);

  return true;
}

here is the result:

as you can see some of the pixels are mixed up, but if i uncomment mutex lock and unlock from this code, then results is correct, but it takes even longer to compute than not using pragma omp

any idea how could i make this more thread safe? when i started, almost half of the pixels were mixed up, then in my custom classes i added every variable as private and create getters and setters, which helped, but its still not completely correct

EDIT: updated code based on @Jérôme Richard advise and added code for intersection

Answer 1

Calling a method called bool Plane::Intersect(Ray &) that mutates the Plane object in addition to determining whether there is an intersection, and then later pulling the result of this mutation out of the Plane object while hoping it wasn't changed in another thread in the meantime, is likely your problem (or at least one of them), and moreover is just not great software engineering.

I'd recommend getting rid of the whole SetDistanceT / GetDistanceT functionality entirely. Just return the distance from the method that checks intersections directly, and make that method const in both its argument and its instance.

(How do you handle the case where dot is zero? You could either return std::numeric_limits<double>::infinity() in that case, or you could have the method return something like a std::pair<bool, double>, or a custom struct, or...)

EDIT: And while you're at it, I'd declare this method as float Intersect(const Ray &) const after you fix it, so that the compiler enforces that it doesn't mutate the ray or the the plane. (The second const there says that the method doesn't mutate the instance it's called on, in this case the plane you're intersecting with.)