[✨ feature] Implement eval_roughness() for most BSDFs
#589
Conversation
There was a problem hiding this comment.
Thank you for this PR @saeedhd96 , especially for going through all of these BSDFs.
A part from the few minor comments, could I ask you to please add a test for the aov changes? Preferably in a new file src/integrators/tests/test_aov.py -- you only need to test the roughness. You can just spawn a single ray that is supposed to hit a rectangle with known roughness and verify the aovs return value. (We don't have any existing tests for the aov integrator.)
| } | ||
| Float eval_roughness(const SurfaceInteraction3f &si, |
| * This method approximates the roughness for a given | ||
| * direction. For some materials, an exact value can be computed | ||
| * inexpensively. | ||
| * When this is not possible, the value is approximated by | ||
| * evaluating the BSDF for a normal outgoing direction and returning this | ||
| * value multiplied by pi. This is the default behaviour of this method. |
There was a problem hiding this comment.
Why shouldn't it be exactly the same? doesn't it under the hood call same methods as diffuse_reflectance_eval?
There was a problem hiding this comment.
I'm confused.
Doesn't the default behaviour just return 1 currently? It doesn't use the BSDF evaluation at the normal direction.
| * surface position. | ||
| */ | ||
| virtual Float eval_roughness(const SurfaceInteraction3f &si, | ||
| Mask active = true) const; |
There was a problem hiding this comment.
Do you mean I should indent it one more tab or one less?
Isn't it currently in line with diffuse_reflectance?
There was a problem hiding this comment.
The Mask active = true argument should be aligned with the opening parenthesis of the previous line. Like in the other methods. Do you see what I mean?
| SurfaceInteraction3f si(si_); | ||
|
|
||
| if (m_brdf[0] == m_brdf[1]) { | ||
| si.wi.z() = dr::abs(si.wi.z()); | ||
| return m_brdf[0]->eval_roughness(si, active); | ||
| } else { | ||
| Float result = 0.f; | ||
| Mask front_side = Frame3f::cos_theta(si.wi) > 0.f && active, | ||
| back_side = Frame3f::cos_theta(si.wi) < 0.f && active; | ||
|
|
||
| if (dr::any_or<true>(front_side)) | ||
| result = m_brdf[0]->eval_roughness(si, front_side); | ||
|
|
||
| if (dr::any_or<true>(back_side)) { | ||
| si.wi.z() *= -1.f; | ||
| dr::masked(result, back_side) = | ||
| m_brdf[1]->eval_roughness(si, back_side); | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Could we move this logic to a separate function?
There was a problem hiding this comment.
Would this be a concern of this PR? since such a refactoring is a different task than this, and should happen for other methods of this class, I would suggest doing that in another tab. Also, I am not too good at C++, and I cannot do such a refactoring without bad consequences :)
There was a problem hiding this comment.
I'm ok with this. I'll refactor it with a lambda once it's merged.
|
Thanks @saeedhd96 for this PR! It would be beneficial if this feature could also take into account anisotropic roughness. Would it be best to have this method return two roughness values? Is there something I'm not considering? |
|
I can totally get the point of having it return two roughness values for anisotropic. I can do that, but I guess that would be the behavior across all imlementations ... is that OK? |
|
I hadn't considered the anisotropic case. (Sorry for the delay...) |
wjakob
force-pushed
the
master
branch
3 times, most recently
from
3f3b8d0
to
1bdea6e
Compare
Description
Introduce method
eval_roughness()similar to eval_diffuse_reflectance() ...Fixes #454
Testing
Tested on a two-sided BSDF, the test is named
test04_eval_roughnessintest_twosided.pyChecklist
cuda_*andllvm_*variants. If you can't test this, please leave below