Page MenuHome

Split "No Caustics" option into Reflection and Refraction caustic controls
ClosedPublic

Authored by Thomas Dinges (dingto) on Aug 30 2014, 6:27 PM.

Diff Detail

Repository
rB Blender

Event Timeline

Thomas Dinges (dingto) retitled this revision from to WIP: Split "No Caustics" option into Reflection and Refraction caustic controls.Aug 30 2014, 6:27 PM
Thomas Dinges (dingto) updated this object.
Thomas Dinges (dingto) updated this revision to Diff 2505.

In general, you might also want to make it work more like the lightpath trick seen here.
http://cgcookie.com/blender/2013/01/14/reducing-caustic-noise-blender-cycles/

This would mean that disabling caustics in effect will not remove near as much realism as it does now, which would then serve as a good compromise for a lot of people doing things like animation.

Regarding TODO 2 :

  • backward compatibility can be solved with version patching in blenloader/intern and a version bump (no_caustics maps to caustics_reflective = caustics_refractive = false)
  • if you also want to preserve upward compatibility, you could keep the old no_caustics variable in DNA and add a get/setter, which updates it accordingly, when one of the other variables gets changed (and leaves you with the interesting question, what to do with the mixed cases :) )

the first you should definitely do, the later is somewhat hacky...

It's not a DNA value, it's only available in py. But I just found out that we have a version patching system for that too (just added by Sergey a few days ago). See intern\cycles\blender\addon\version_update.py That should do the trick. :)

it is also available in DNA using properties API (everything in blender goes into DNA, there is no other way that data can end up in a blend file :),
but Sergey's new python version patching for cycles looks like it is a lot easier to read and maintain, so go ahead :)

Thomas Dinges (dingto) updated this revision to Diff 2517.

Add backward compatibility code.

Did a few test renders of my own. Glad to see this functionality added so quickly after I mentioned it! These were done on 6 of the 8 cores on my 4790k. You can see the big time savings in scenes where you need reflective caustics, but where you can stand to not have/fake refractive ones.

Hey @Brecht Van Lommel (brecht) and @Sergey Sharybin (sergey) :)

I am not 100% sure how to make this work for OSL. I checked and it seems OSL distinguishes between scattering and event types. See https://github.com/imageworks/OpenShadingLanguage/blob/8ca9327413f3179f4be9c2714c533c705ad3e07c/src/include/OSL/oslclosure.h for example (Labels class).

Do we need to add a m_event_label, next to m_scattering_label in CBSDFClosure()? Some hint would be appreciated. :)

We don't use those labels from OSL, only our own LABEL_* flags, which are bitflags that you can combine. In osl_closures.cpp, bsdf_phong_ramp.cpp and osl_shader.cpp you can replace LABEL_GLOSSY by LABEL_GLOSSY|LABEL_REFLECT and LABEL_GLOSSY|LABEL_TRANSMIT depending if the BSDF reflects or transmits.

By the way, the light path trick mentioned above is like a more aggressive version of filter glossy. Filter glossy will increase BSDF roughness up to 0.5, this trick replaces it with a diffuse BSDF. Filter glossy could be changed or made configured so it can go all the way up to 1.0, especially with the new glossy BSDF sampling that is less noisy for high roughness. Roughness 1 glossy is not quite the same as diffuse, but it's a very soft reflection too.

Thomas Dinges (dingto) updated this revision to Diff 2533.

OSL now works as well, thank you @Brecht Van Lommel (brecht) for the help. :) I will check on the filter_glossy option later, might be a good improvement too.

I did not change all closure labels though, because in SVM the caustic option also only affects some shaders (Glossy, Glass, Aniso), we can make this affect more shaders later, but then it should be consistent for both backends.

I consider this done now, ready for review.

Thomas Dinges (dingto) retitled this revision from WIP: Split "No Caustics" option into Reflection and Refraction caustic controls to Split "No Caustics" option into Reflection and Refraction caustic controls.Sep 4 2014, 3:57 PM
Sergey Sharybin (sergey) requested changes to this revision.
Sergey Sharybin (sergey) added inline comments.
intern/cycles/blender/addon/properties.py
262

You don't need this, see later.

intern/cycles/blender/addon/version_update.py
36

You're having spaces in the empty line, which makes the file pep8-uncomplaint.

37

I'm not really happy with bumping blender subversion for changes like this. Subversion bump is to be done when major changes to the file database are done.

Ideally we would need to have Cycles version, so we can do whatever versioning code we want to regardless to blender.

For now checking for whether cscene's caustics_{reflective,refractive} are both unset before assigning would avoid subversion bump.

40

Instead of doing this and requiring deprecated property to be hanging around you can do something like:

try:
    if cscene["no_caustics"]:
        do_versioning_code()

@Campbell Barton (campbellbarton) could give some better advice here.

intern/cycles/kernel/osl/osl_shader.cpp
172

Alignment could be fixed here. Also would prefer at least having braces around the body when having multiple-line condition. Same applies to some conditions below.

P.S. Would also prefer 80-chars limit, but that's a general headache in cycles..

intern/cycles/kernel/svm/svm_closure.h
267

Not so much fan of if(!(!foo)), double-negative is never healthy way to think about.

Suggestions:

/* So it's more verbose at least. */
if(!(kernel_data.integrator.caustics_reflective == false && (path_flag & PATH_RAY_DIFFUSE)))

or

/* Expanded expression which is more clear to follow IMO. */
if(kernel_data.integrator.caustics_reflective || (path_flag & PATH_RAY_DIFFUSE) == 0)
source/blender/blenkernel/BKE_blender.h
45 ↗(On Diff #2533)

Unhappyness of this i've already mentioned.

This revision now requires changes to proceed.Sep 5 2014, 9:18 AM
Thomas Dinges (dingto) updated this revision to Diff 2541.

Addressed review comments.

Sergey Sharybin (sergey) accepted this revision.

Minot things:

  • Extra EOL in update_versions.py
  • Line consists of spaces at line 276 of svm_closure.h
This revision is now accepted and ready to land.Sep 5 2014, 8:20 PM