Refactoring

[lforg37]

Split refactoring commit from #42

[lforg37]

Continuing the discussion from #42

But I am not convinced by the new design.
I cannot see a reason to use this explicit visitor pattern from a pre-modern C++ or Java world when there was no generic lambdas...
You have created a new explicit visitor with all the object-dependent implementation in it breaking the encapsulation by in-lining here all what was in the hit function before.

I don't think we "break" the encapsulation. In fact, now a sphere or rectangle don't have to be aware of what is a hit_record or a task_context, so this new design avoid a tight coupling between the scene graph objects and the rendering task.
There remains a few bit of it that could be further simplified, (especially the image_texture that still has the non satisfactory requirement of a dependency injection of a sycl::global_ptr<uint8_t>).

Speaking strictly of design, I think there is a rational for a sphere class, and rational for a ray class, but the sphere should be able to leave without knowledge of the ray (for instance if we plan to provide a scene editor, or a renderer by rasterisation...), so it does not seems a bad idea to me to have the intersection in the outside of both of these classes.

An alternative to a complete visitor class would be to have a set of overloaded intersect function, such as:

bool intersect(scene::box const& b, task_context& ctx, const ray& r, real_t min, real_t max,
               hit_record& rec, material_t&  hit_material_type) {
    hit_record temp_rec;
    scene::material_t temp_material_type;
    auto hit_anything = false;
    auto closest_so_far = max;
    // Checking if the ray hits any of the sides
    for (const auto& side : b.sides) {
      if (dev_visit(_hitable_hit(ctx, r, min, closest_so_far, temp_rec,
                                 temp_material_type),
                    side)) {
        hit_anything = true;
        closest_so_far = temp_rec.t;
        rec = temp_rec;
        hit_material_type = temp_material_type;
      }
    }
    return hit_anything;
  }

And creating implicit visitor by using

dev_visit([&ctx, &r, =min, =max, &rec,  &material](auto&& obj){
    return intersect(obj, ctx, r, min, max, rec, material);
}, variant);

But having the complete visitor class allow to DRY.

After looking at the code now with the increased complexity with new kitchen-sink visitor requiring direct access to public object members or über-friendship, I am even less convinced...

It is always read-only access on object properties, if these property become private there is no problem to get an accessor on it.
I don't think I introduced any friendship relation with this refactoring.

Having a distributed hit like before does prevent to add some hierarchy if you have 2 different implementations of sphere for example, by having a hit implemented in some sphere specific class which is inherited by the various spheres.

Not sure I understand you completely here

[keryell]

I need to dive into the code again...
It would be interesting whether the https://en.wikipedia.org/wiki/Bounding_volume_hierarchy will have an impact on this.

[keryell]

I am unsure we want to close this. Perhaps keeping this as a Draft PR instead to keep track of this?