raysect
Lambert.bsdf() and RoughConductor.bsdf() possibly return incorrect values
Maybe this is a problem of definition, but if the BSDF is defined as dLout/dEin, Lambert.bsdf() is missing 1/π multiplier. Also, RoughConductor.bsdf() is missing 1 / s_incident.z.
Note that evaluate_shading() seems to be correct in both cases, because in Lambert material the 1/π multiplier is in pdf() and in RoughConductor material the BSDF is multiplied by s_outgoing.z to obtain angular reflectivity (s_outgoing is incident here because of backward raytracing).
The bsdf function is really meant to be an internal method of the material here and not called directly (though I know that is not described anywhere atm). The normalisation by the constants is performed by the sampling distribution as you spotted. At present the "bsdf" method is not meant to be used independently of the sampling. In the future (if I ever get time) I intend to rework the material system to allow all these method to be usable directly. For now treat them as internal.
But 1 / s_incident.z is not a constant. Also, there are two demos in demos/reflectivity that plot RoughConductor BSDFs by calling bsdf() method.
I noticed that because I have some measured BSDFs which I try to fit with a combination of Lambertian and Cook-Torrance BSDFs, for that I needed these methods.
The projected area (1 / s_incident.z) is handled by the cosine sampling:
cdef HemisphereCosineSampler hemisphere_sampler = HemisphereCosineSampler()
You can define BSDF a few different ways, some definitions include the area projection in the BSDF, some don't. Radiometry is a messy, inconsistent field and it is very frustrating.
I didn't add any demo calling BSDF.... that must have been Matt. I'll have to dig through it and check it is valid. Please be aware I'm commenting from a very old memory here, I've not got the time to go though this in detail at present. It is possible I'm misremembering and we did actually fix the definition of bsdf.
Thank you for your answer.
The bsdf() method is not used in the sampling at all, it seems that it was added by @mattngc just for testing (see this #214). But it is definitely useful, so the clear definition is needed.
Ok I've just have a closer look. Yes we use evaluate_shading() in the tracing not bsdf(). When I scanned over your issue and remembered back to the raytracing implementation I had assumed that we were talking about the evaluate_shading() when you said the bsdf() function. My apologies for further confusing the issue, I didn't remember a bsdf() function being added.
There shouldn't be a bsdf() function returning incorrect values, even if it was a test.... that is just bad. I'll ask @mattngc to clarify what the definition is. It could actually be a bug then. If that is the case, I'm very sorry, we'll get it fixed.
Removed the bsdf methods and associated demo until the code can be reworked.