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

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
208	Do we need this is sample as light is disabled and there are no portals?
intern/cycles/kernel/kernel_light.h
35–36	Space after keyword.
35–36	Same as above.
35–36	Here as well, and in the next chunk, and check rest of the patch as well.
35–36	Don't keep dead code.
39	Don't put operators to the same line, it's more a PITA to debug.
51	Would suggest `background_num_possible_portals`. Plus brace on the new line.
57	Would actually prefer comments /**/, plus space after/before the opening/closing comment brace.
intern/cycles/render/light.h
59 ↗	(On Diff #3745)	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?
193	Same as above, NULL check needed?
intern/cycles/kernel/kernel_types.h
900	KernelData structs are 16 byte aligned, so with 3 new variables you need 1 padding int or float.
intern/cycles/render/light.cpp
377	Style. :)
509	Code style.
515	I would keep these lines (including the "float4 *light_data" definition at the beginning of the function. New code then afterwards.

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
199	I'd call it `use_portal` instead.
219	Use `sample_as_light` instead of getting boolean again.
521	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
35–36	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?
38	It seems you can call it `bool has_possible_portals` and skip doing increments here, saving fetching?
47	Braces.
51	Same as above.
56	Same as above.
85	Is it intended to have different eps comparing to the samecheck in function above?
91	`sampled_portal`?
93	Sentence ends with full stop. Same actually applies to all the comments.
138	What this 's' stands for?
178	Do we really need comments around code which is already self-explanatory?
intern/cycles/render/light.cpp
296–298	`pos` is really ambiguous name which is better to be avoided. You can simply think of better name like `light_index`.
298	Don't put operator onto same line as condition.
508	Why do we first add background and then removing it?
520–523	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
199	Changed.
219	Changed.
521	Would be possible as well, but would probably cause code duplication (all the dupli objects and visibility stuff)...
intern/cycles/kernel/kernel_light.h
35–36	Yes, these names were quite weird, changed them to background_(map/portal/light)_(sample/pdf) now, that should be consistent and intuitive.
38	Yes, changed.
47	All three changed.
85	Not really, changed (all to 1e-5f).
91	Changed.
93	Added punctuation to most comments.
138	"portal_sampling_pdf", changed to that one now.
178	Not really, I just commented everything because the sampling/pdf code is quite tricky with all the conditions.
intern/cycles/render/light.cpp
296–298	Changed.
298	Changed.
508	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.
520–523	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
49	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.
94	Picky, should be 0.0f.
129	can's? :D
intern/cycles/render/light.cpp
155	Picky: Move the bool after num_triangles, also with an empty line in between. It's not part of the counting variables.
398	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 3596

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_object.cpp

intern/cycles/kernel/kernel_emission.h

intern/cycles/kernel/kernel_light.h

intern/cycles/kernel/kernel_types.h

intern/cycles/render/light.cpp

release/scripts/startup/bl_ui/properties_data_lamp.py

source/blender/editors/object/object_add.c

source/blender/editors/space_outliner/outliner_draw.c

source/blender/editors/space_view3d/drawobject.c

source/blender/gpu/intern/gpu_material.c

source/blender/makesdna/DNA_lamp_types.h

source/blender/makesrna/RNA_access.h

source/blender/makesrna/intern/rna_lamp.c

source/blender/render/intern/source/convertblender.c

Cycles: Added support for light portalsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 3596

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_object.cpp

intern/cycles/kernel/kernel_emission.h

intern/cycles/kernel/kernel_light.h

intern/cycles/kernel/kernel_types.h

intern/cycles/render/light.cpp

release/scripts/startup/bl_ui/properties_data_lamp.py

source/blender/editors/object/object_add.c

source/blender/editors/space_outliner/outliner_draw.c

source/blender/editors/space_view3d/drawobject.c

source/blender/gpu/intern/gpu_material.c

source/blender/makesdna/DNA_lamp_types.h

source/blender/makesrna/RNA_access.h

source/blender/makesrna/intern/rna_lamp.c

source/blender/render/intern/source/convertblender.c

Cycles: Added support for light portals
ClosedPublic
Actions

Revision Contents
Changeset List