Importance sampling of OSL environment EDFs

[lorentzo]

Contribution of this PR

Issue:

This is PR for issue #701 (Importance sampling of OSL environment EDFs).

Summary:

• Implementation of OSLEnvironmentSampler which is using OSLShadingSystem and ShaderGroup for evaluating background shader. (NEW: instead of directly using OSLShadingSystem I am using OSLShaderGroupExec)
• Building of importance map inside of importance_sampler (instance of ImageImportanceSampler which is using OSLEnvironmentSampler as sampler in ImageImportanceSampler::rebuild()). This is done by modifying build_importance_map().
• Modification of OSLEnvironmentSampler::sample(), OSLEnvironmentSampler::evaluate() and OSLEnvironmentSampler::evaluate_pdf() so it is using the importance_sampler.
• Some of my additional comments during solving this issue are available here: https://github.com/appleseedhq/appleseed/issues/701
• My work was inspired by:
  • LatLongMapEnvironmentEDF
  • OSL's test render component: background

Notes:

• I haven't used OSLShaderGroupExec class nor ShadingContext class for background shader evaluation since OSLShaderGroupExec::execute_background() is private. Instead I have written similar code for evaluation of background shader. (NEW: I am using OSLShaderGroupExec for evaluation of background shader and now OSLShaderGroupExec::execute_background() is made public)
• Size of the importance map is is hard-coded - but can be changed easily later (noted as TODO in code). Let me know how would you like it to be set. (NEW: I am using entity meta data for enabling user values and default values)
• Next to importance map size, there may be some additional optional input parameters (noted as TODO in code).
• Next to those "TODO" code comments, there are some code comments that I find useful. Let me know if they should stay.
• For the testing I have executed "29 - osl environment reflection direct lighting.appleseed" and executed modification of "17 - latlong environment map out-of-range luminance.appleseed" where OSL shaders are used. I can also add this test to PR if needed.
  • I have some doubts about resulting test images. I see no difference in resulting images of my implementation of importance sampling of OSL environment EDFs and one that existed before.
  • Also, test "17 - latlong environment map out-of-range luminance.appleseed" results in image where really bright pixels are visible - I don't see that in my modification of this test when using my implementation of iImportance sampling of OSL environment EDFs.

Further work

• I am interested into testing this code. I will try to think of some goods tests. But let me know if you have some ideas so I can try them out.

[lorentzo]

@dictoon thank you for detailed code review. I have fixed everything you asked. I have two doubts for now:

• moving build_importance_map() from on_frame_begin() to on_render_begin() as you proposed in https://github.com/appleseedhq/appleseed/pull/2784#discussion_r386053142 results in segmentation error while calling m_osl_shadergroup_exec.execute_background() (this function is needed for building the importance map). After some experiments I think I have figured out what might be the problem. I think that when on_render_begin() is called, OSL shaders haven't been configured yet (path, headers etc.). While when build_importance_map() is in on_frame_begin(), no problem occurs. That is because on_frame_begin() is called after the OSL shaders have been configured. I will discuss this further on discord.
• Do you require some additional test scenes for this issue? For example I have modified some test scenes for OSL environment where metadata is used.

[lorentzo]

OK, now all commits have been squashed and rebased on master. No conflicts. Buildable.

Only this is left to consider: https://github.com/appleseedhq/appleseed/pull/2784#issuecomment-593100136.

I have discussed this problem in Discord #dev, here: https://discordapp.com/channels/430063582777049089/430064067873472526/683677424088514571.

[dictoon]

To answer your previous points:

Moving build_importance_map() from on_frame_begin() to on_render_begin() as you proposed in #2784 (comment) results in segmentation error while calling m_osl_shadergroup_exec.execute_background() (this function is needed for building the importance map). After some experiments I think I have figured out what might be the problem. I think that when on_render_begin() is called, OSL shaders haven't been configured yet (path, headers etc.). While when build_importance_map() is in on_frame_begin(), no problem occurs. That is because on_frame_begin() is called after the OSL shaders have been configured. I will discuss this further on discord.

That's interesting. I haven't checked exactly what's going on but I trust your analysis. What we could do to prevent this PR from stalling is keep this in on_frame_begin() for now, with a comment explaining the problem, then investigate how to fix the problem in a second time.

The impact of rebuilding the importance map in on_frame_begin() is low since you check if the shader group version has changed.

Do you require some additional test scenes for this issue? For example I have modified some test scenes for OSL environment where metadata is used.

It would be nice to have at least one scene in the test suite that checks that this code works correctly.

[lorentzo]

To answer your previous points:

Moving build_importance_map() from on_frame_begin() to on_render_begin() as you proposed in #2784 (comment) results in segmentation error while calling m_osl_shadergroup_exec.execute_background() (this function is needed for building the importance map). After some experiments I think I have figured out what might be the problem. I think that when on_render_begin() is called, OSL shaders haven't been configured yet (path, headers etc.). While when build_importance_map() is in on_frame_begin(), no problem occurs. That is because on_frame_begin() is called after the OSL shaders have been configured. I will discuss this further on discord.

That's interesting. I haven't checked exactly what's going on but I trust your analysis. What we could do to prevent this PR from stalling is keep this in on_frame_begin() for now, with a comment explaining the problem, then investigate how to fix the problem in a second time.

The impact of rebuilding the importance map in on_frame_begin() is low since you check if the shader group version has changed.

I would suggest opening an issue: "Investigate a way to add build importance map inside of OSLEnvironmentEDF::on_render_begin() instead of OSLEnvironmentEDF::on_frame_begin()". I would reference this PR and provide analysis that I have done. Do you agree?

Do you require some additional test scenes for this issue? For example I have modified some test scenes for OSL environment where metadata is used.

It would be nice to have at least one scene in the test suite that checks that this code works correctly.

The test suite already contains a test scene that uses/test the code I have created, here: https://github.com/appleseedhq/appleseed/blob/master/sandbox/tests/test%20scenes/environment/29%20-%20osl%20environment%20reflection%20direct%20lighting.appleseed. This test is a good comparison with the LongLat environment EDF.

NEW: I have also added the test scene in this PR. I have used HDR from texturehaven.com. I have set explicit importance map size to 2048. Also, I have stored the reference image. Now, there are 2 test scenes for OSL environment EDF!

[lorentzo]

OK, now all the required changes are done. PR is squashed and rebased on the master. It is ready for merging. Summary:

• Added importance sampling of OSL environment EDF.
• Added test scene for importance sampling of OSL environment EDF, new HDR image for the test and resulting image of the test.
• Refactoring of latlongmapenvironmentedf.cpp and oslshadergroupexec.h

Further work. Issue: "Investigate a way to add build importance map inside of OSLEnvironmentEDF::on_render_begin() instead of OSLEnvironmentEDF::on_frame_begin()". NEW: @est77 proposed:

"in masterrenderer.cpp, line 359 is where we compile OSL shaders if we move that a few lines, before on_render_begin is called, we would have compiled OSL shaders in on_render_begin"

[lorentzo]

HDR map colorful_studio_2k.hdr is taken from https://hdrihaven.com/ which states:

All HDRIs are licenced as CC0 and can be downloaded instantly, giving you complete freedom.

[dictoon]

Thanks for your contribution!