Details

Reviewers

Thomas Dinges (dingto)
Sergey Sharybin (sergey)

Group Reviewers

Cycles

Commits

rBSf478c2cfbd38: Cycles: Added support for light portals
rC914cea72960f: Added support for light portals
rBf478c2cfbd38: Cycles: Added support for light portals

Summary

This patch adds support for light portals: Objects that help sampling the environment light, therefore improving convergence.
Using them tor other lights in a unidirectional pathtracer is virtually useless.

The sampling is done with the area-preserving code already used for area lamps.
MIS is used both for combination of different portals and for combining portal- and envmap-sampling.

The direction of portals is considered, they aren't used if the sampling point is behind them (since then, the ray winn go into the interior...)

Examples following, I'm uploading this with arcanist...

Diff Detail

Repository: rB Blender
Branch: portals_new

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Thomas Dinges (dingto) commandeered this revision.Mar 15 2015, 1:21 PM

Thomas Dinges (dingto) added a reviewer: Lukas Stockner (lukasstockner97).

I cleaned up the diff a bit, remove whitespace changes which made the diff bigger and if() style. No functional change.
Dumping it here, because if I update the diff, we have this "No context" issue again, as I don't use arc...

portal_clean.diff31 KBDownload

Lukas, feel free to commander this back. :)

Vitor Balbio (vitorbalbio) added a subscriber: Vitor Balbio (vitorbalbio).Mar 16 2015, 4:57 AM

I didn't test functionality yet, but the code does not follow the convention. I didn't point it on every single line, just read the comments and make it so it's met all over the patch.

intern/cycles/blender/blender_object.cpp
218	Do we need this is sample as light is disabled and there are no portals?
intern/cycles/kernel/kernel_light.h
153	Space after keyword.
157	Same as above.
179	Here as well, and in the next chunk, and check rest of the patch as well.
271	Would actually prefer comments /**/, plus space after/before the opening/closing comment brace.
300–449	Don't keep dead code.
304	Don't put operators to the same line, it's more a PITA to debug.
316	Would suggest `background_num_possible_portals`. Plus brace on the new line.
intern/cycles/render/light.h
59	Would call it `is_portal`.

This revision now requires changes to proceed.Mar 18 2015, 10:24 AM

Diego Gangl (januz) added a subscriber: Diego Gangl (januz).Apr 3 2015, 2:33 PM

Any updates here Lukas? If this should go in for 2.75, we need an update pretty soon.

Commandeering this back to post the next update.

The code style should now be fixed (especially the comments). What took so long was the sync_background_light check because I tried to come up with an elegant solution, but for now, the current checking while syncing objects works.
Also, I fixed a really stupid bug that caused problems with additional lamps, even without using portals.

The new version

Tested the functionality with a modified Cornell box and the Sponza scene, looks good so far. More tests are very welcome though.

Some code comments inline, did not check the kernel code extensively yet though.

intern/cycles/blender/addon/properties.py
698	Would write "Use" instead of "Uses".
intern/cycles/blender/blender_object.cpp
100	I might be missing something, but it seems we don't need the has_portal NULL check?
182	Same as above, NULL check needed?
intern/cycles/kernel/kernel_types.h
898	KernelData structs are 16 byte aligned, so with 3 new variables you need 1 padding int or float.
intern/cycles/render/light.cpp
383	Style. :)
543	I would keep these lines (including the "float4 *light_data" definition at the beginning of the function. New code then afterwards.
553	Code style.

Thomas Dinges (dingto) requested changes to this revision.Apr 21 2015, 10:47 PM

Thomas Dinges (dingto) edited edge metadata.

This revision now requires changes to proceed.Apr 21 2015, 10:47 PM

Implemented the comments, except for the "keep these lines at the beginning of the function" one: These lines use lights.size(), which will be changed by the background light removal, which in turn needs the has_portal info.

Sergey Sharybin (sergey) requested changes to this revision.Apr 22 2015, 5:33 PM

Sergey Sharybin (sergey) edited edge metadata.

Sergey Sharybin (sergey) added inline comments.

intern/cycles/blender/blender_object.cpp
196	I'd call it `use_portal` instead.
217–218	Use `sample_as_light` instead of getting boolean again.
522	Thinking whether it'll be more clear to have an utility function called `object_is_portal` in order to avoid passing `has_portal` into all the sync calls hierarchy.
intern/cycles/kernel/kernel_light.h
223	naming of this function and previous one are not totally clear to me. First once seems to be more `background_light_sample_cdf`, but it actually returns PDF and not CDF? Second one is confusing, is it returning CDF of PDF? Or otherwise what cdf_pdf means?
252	It seems you can call it `bool has_possible_portals` and skip doing increments here, saving fetching?
312	Braces.
316	Same as above.
321	Same as above.
350	Is it intended to have different eps comparing to the samecheck in function above?
356	`sampled_portal`?
358	Sentence ends with full stop. Same actually applies to all the comments.
403	What this 's' stands for?
443	Do we really need comments around code which is already self-explanatory?
intern/cycles/render/light.cpp
301–304	`pos` is really ambiguous name which is better to be avoided. You can simply think of better name like `light_index`.
303	Don't put operator onto same line as condition.
552	Why do we first add background and then removing it?
556	Same story as above with `pos`.

This revision now requires changes to proceed.Apr 22 2015, 5:33 PM

Lukas Stockner (lukasstockner97) added inline comments.Apr 23 2015, 3:47 PM

intern/cycles/blender/blender_object.cpp
196	Changed.
217–218	Changed.
522	Would be possible as well, but would probably cause code duplication (all the dupli objects and visibility stuff)...
intern/cycles/kernel/kernel_light.h
223	Yes, these names were quite weird, changed them to background_(map/portal/light)_(sample/pdf) now, that should be consistent and intuitive.
252	Yes, changed.
312	All three changed.
350	Not really, changed (all to 1e-5f).
356	Changed.
358	Added punctuation to most comments.
403	"portal_sampling_pdf", changed to that one now.
443	Not really, I just commented everything because the sampling/pdf code is quite tricky with all the conditions.
intern/cycles/render/light.cpp
301–304	Changed.
303	Changed.
552	The "(has_portals \|\| light->use_mis)" was actually supposed to replace the missing check in sync_background_light, but is now obsolete. Reverted to old removing code (only if advanced_shading is false). Actually, this also makes the has_portals check unnecessary.
556	Changed.

Implemented the review, also now the difference between background_portal, background_map (the "old" CDF sampling for World MIS) and background_light (combining the two) should be clearer.

David Michael Sisco (dsisco11) added a subscriber: David Michael Sisco (dsisco11).Apr 23 2015, 8:13 PM

Jason Clarke (jtheninja) added a subscriber: Jason Clarke (jtheninja).Apr 24 2015, 4:01 AM

Looks mostly good to me now. Some picky comments below. :)

intern/cycles/kernel/kernel_light.h
263	Performance: Maybe put data1/2 and the corresponding axisu/v a bit below, right before it's actually used, as we might not reach these lines (289..) at all.
359	Picky, should be 0.0f.
394	can's? :D
intern/cycles/render/light.cpp
160	Picky: Move the bool after num_triangles, also with an empty line in between. It's not part of the counting variables.
405	Picky: keep kintegrator entries together, then the kfilm one.

Thomas Dinges (dingto) requested changes to this revision.Apr 24 2015, 4:18 AM

Thomas Dinges (dingto) edited edge metadata.

This revision now requires changes to proceed.Apr 24 2015, 4:18 AM

Implemented all comments...

Compiled latest patch and did a quick test, works as expected. Code is good enough for master imho. @Sergey Sharybin (sergey), any remarks left?

Ronny G (nutel) added a subscriber: Ronny G (nutel).Apr 25 2015, 12:45 PM

Lapineige added a subscriber: Lapineige.Apr 26 2015, 11:01 PM

Yes, there're some remarks but they're easier to just address myself when applying the patch.

Closed by commit rBf478c2cfbd38: Cycles: Added support for light portals (authored by Lukas Stockner (lukasstockner97), committed by Sergey Sharybin (sergey)). · Explain WhyApr 27 2015, 10:31 PM

This revision was automatically updated to reflect the committed changes.

Cycles: Added support for light portals
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 4052

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_object.cpp

intern/cycles/blender/blender_sync.h

intern/cycles/kernel/kernel_emission.h

intern/cycles/kernel/kernel_light.h

intern/cycles/kernel/kernel_types.h

intern/cycles/render/light.h

intern/cycles/render/light.cpp

Cycles: Added support for light portalsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 4052

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_object.cpp

intern/cycles/blender/blender_sync.h

intern/cycles/kernel/kernel_emission.h

intern/cycles/kernel/kernel_light.h

intern/cycles/kernel/kernel_types.h

intern/cycles/render/light.h

intern/cycles/render/light.cpp

Cycles: Added support for light portals
ClosedPublic
Actions

Revision Contents
Changeset List