-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Hemispherical distant sensor plugin #11
base: master
Are you sure you want to change the base?
Conversation
Thanks for this PR @leroyvn , Would it make sense for On the other hand, this is not a lot of code, I don't think it would be worth it to share some of this in a header file. |
I hadn't thought of that: actually I think these two methods will be different from the |
Okay then, let's keep those seperate 👍 It would be great to have them yes. Otherwise we will need to revisite those plugins once again when the light tracer has landed. |
I looked into it and it seems that the |
Makes sense. @merlinND is this something you encountered in your work on the light tracer? |
Hi @Speierers, I just pushed an update with tests and docs. It now requires a commit on the |
Hi @leroyvn, Great illustration for the doc! I am just a little confused with the |
I'll make a new proposal, hopefully clearer.
Yes, and the direction pointed by |
Hi there! I'll sneak in to this discussion 😋 I like the illustrations, but I am a bit confused! Is the hemisphere located above the blue plane in the |
Hi @tomasiser, it's indeed clearer with the full docs. The sample scene has three
In practice,
Parametrising |
@leroyvn I like very much the new illustration :) |
src/sensors/hdistant.cpp
Outdated
std::tie(ray, ray_weight) = sample_ray( | ||
time, wavelength_sample, film_sample, aperture_sample, active); | ||
|
||
// Since the film size is always 1x1, we don't have differentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that really true? The example shows rendered 2D images with spatial variation. The sample_ray_differential
function should track how adjacent rays change either (or both) in their 'o' and 'd' parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's not, sorry for this mistake! I started from the distant
code and forgot to update that part, I'll check that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added ray differentials using the same code as for the main ray direction and origin (basically skipping spectrum-related parts), I hope it's fine.
d43e297
to
8bc5ae2
Compare
Hi Vincent, I just took another look at this PR and thought a bit more about the 'shape' target feature common to this sensor and 'distant.cpp'. A few thoughts:
Thanks, |
Update data submodule
Hi @wjakob, sorry it took me a while to get back to this. I tried to address your comments:
If this is fine, I'll also propagate the documentation update to |
Description
This PR adds a hemispherical distant sensor, which is similar to an adjoint to the
envmap
emitter. It records radiance leaving the scene in a hemisphere. It can be useful to get a qualitative idea of the effective BRDF of a surface with complex geometry.Code is mostly taken from the
distant
sensor plugin, and I don't know to what extent it would be desirable and/or doable to move common parts to a library header (in the end, only the constructor and direction sampling code are different).An example of RGB output (a simple rectangular surface with a
roughconductor
BSDF; coloured dots each correspond to a directional emitter in the scene; note that pixel index origin is taken at the bottom left of the image):To do
Testing
A set of tests similar to those written for
distant
is included. We test for object construction, ray sampling and overall result correctness in a few scenarios.Checklist
cuda_*
andllvm_*
variants. If you can't test this, please leave below