ziotom78 / pytracer

A simple raytracer in pure Python
MIT License
9 stars 4 forks source link

Reflectance not considered in path tracer renderer #17

Closed Samuele-Colombo closed 3 years ago

Samuele-Colombo commented 3 years ago

While examining code i found that in the implementation of PathTracer, while evaluating the luminance of the hit color (here) we do not consider the BRDF reflectance. This can be particularly nasty when, in this line there is a check to avoid wasteful recursions as there might be a BRDF with nonzero pigment color but zero reflectance. Furthermore at [this line] the cumulative radiance is calculated using the variable hit_color which does not consider the effects of reflectance. Further browsing into the project revealed that the radiance field of DiffuseBRDF is never used and seems to be generally ignored outside of the DiffuseBRDF definition. Even in the furnace test implementation does not make use of the built-in reflectance but scales the BRDF pigment during definition.

Since it appears that this field has no actual use in your implementation would it make sense to remove it entirely or should it be better implemented into the code (even if it would be laborious to change all the code)? Surely leaving the code as-is, without either removing the field or better implementing it, would lead to confusion for the users that would find that tuning the reflectance field has no effect on the final result.

ziotom78 commented 3 years ago

Thanks for having spotted this! I was sure I removed that useless parameter from the master branch, but in fact the commit ended up in another branch:

https://github.com/ziotom78/pytracer/commit/f994f863bf2c37b3f3f73f681435895f8117c8fb

The commit is already incorporated in the scenefiles branch, so I'll leave things as they are and will merge it this week.

P.S. The field pigment is the true reflectance, which depends on the (U,V) hit point. The field reflectance (a boring float) was included as a placeholder in an old version of the code which did not support pigments yet, and somewhat I forgot to remove it.