LightSampler refactored into Backward and ForwardLightSampler, use of EMTs with the LightTree #1544

gospodnetic · 2017-07-19T10:11:34Z

LightSampler is no longer used so I suggest removing it and adapting the tests to Backward and ForwarLightSamplers

dictoon · 2017-07-19T10:13:42Z

CMakeLists.txt

@@ -109,7 +109,7 @@ option (WITH_TOOLS                          "Build appleseed tools"
 option (WITH_PYTHON                         "Build Python bindings"                                 ON)
 option (WITH_DISNEY_MATERIAL                "Build Disney material"                                 OFF)

-option (USE_STATIC_BOOST                    "Use static Boost libraries"                            ON)
+option (USE_STATIC_BOOST                    "Use static Boost libraries"                            OFF)


dictoon · 2017-07-19T10:22:40Z

src/appleseed/renderer/kernel/lighting/lighttracing/lighttracingsamplegenerator.h

@@ -42,7 +42,7 @@

 // Forward declarations.
 namespace renderer  { class Frame; }
-namespace renderer  { class LightSampler; }
+namespace renderer  { class ForwardLightSampler; }


Order alphabetically.

dictoon · 2017-07-19T13:51:37Z

sandbox/tests/test scenes/many light sampling/20x20_white_point_lights.appleseed

@@ -4899,7 +4899,52 @@
    </output>
    <configurations>
        <configuration name="final" base="base_final">
+            <parameter name="lighting_engine" value="pt" />


It would be nice if these files were left exactly as they were generated by the Python scripts. In other words, if you would like to change rendering settings, I'd suggest changing them in the Python scripts instead.

to be honest, I am not sure where did this come from as i didn't go directly into the file. Maybe I saved it by accident from the studio.

dictoon · 2017-07-19T14:01:32Z

sandbox/tests/test scenes/many light sampling/grid_EMT.py

+        mat = orientation * asr.Matrix4d.make_translation(asr.Vector3d(0.0, 0.0, 0.0))
+        instance = asr.ObjectInstance(
+                                        instance_name,
+                                        {"visibility":


Full visibility is the default, you don't need to specify anything in this case.

dictoon · 2017-07-19T14:03:33Z

src/appleseed/renderer/kernel/lighting/backwardlightsampler.h

+//
+// Copyright (c) 2010-2013 Francois Beaune, Jupiter Jazz Limited
+// Copyright (c) 2014-2017 Francois Beaune, The appleseedhq Organization
+// Copyright (c) 2017 Petra Gospodnetic, The appleseedhq Organization


Maybe one day we'll simplify the copyright rules... But for now you need to either remove the original copyright notices, or remove the last one.

dictoon · 2017-07-19T14:10:50Z

src/appleseed/renderer/kernel/lighting/directlightingintegrator.h

@@ -46,7 +46,7 @@

 // Forward declarations.
 namespace renderer  { class LightSample; }
-namespace renderer  { class LightSampler; }
+namespace renderer  { class BackwardLightSampler; }


Order alphabetically.

dictoon · 2017-07-19T14:37:56Z

src/appleseed/renderer/kernel/rendering/masterrenderer.cpp

@@ -292,6 +292,9 @@ IRendererController::Status MasterRenderer::render_frame_sequence(
            return IRendererController::AbortRendering;
        }

+        if (components.is_backward_light_sampler_used())


Instead of is_backward_light_sampler_used(), couldn't we simply check if get_backward_light_sampler() returns nullptr or not?

lol that's right. :D

dictoon · 2017-07-19T14:38:05Z

src/appleseed/renderer/kernel/rendering/renderercomponents.cpp

  , m_shading_engine(get_child_and_inherit_globals(params, "shading_engine"))
  , m_texture_store(texture_store)
  , m_texture_system(texture_system)
  , m_shading_system(shading_system)
+  , m_forward_light_sampler(0)


Use nullptr.

dictoon · 2017-07-19T14:39:07Z

src/appleseed/renderer/kernel/rendering/renderercomponents.h

  private:
    const Project&              m_project;
    const ParamArray&           m_params;
    ITileCallbackFactory*       m_tile_callback_factory;
    const Scene&                m_scene;
    const Frame&                m_frame;
    const TraceContext&         m_trace_context;
-    LightSampler                m_light_sampler;
+    ForwardLightSampler*        m_forward_light_sampler;


If you must store pointers, use std::unique_ptr<> instead.

dictoon · 2017-07-19T14:39:17Z

src/appleseed/renderer/kernel/rendering/renderercomponents.h

@@ -84,14 +85,19 @@ class RendererComponents

    IFrameRenderer& get_frame_renderer();

+    BackwardLightSampler* get_backward_light_sampler();
+
+    bool is_backward_light_sampler_used();


I suggest removing this.

dictoon · 2017-07-19T14:39:32Z

src/appleseed/renderer/kernel/rendering/renderercomponents.h

@@ -84,14 +85,19 @@ class RendererComponents

    IFrameRenderer& get_frame_renderer();

+    BackwardLightSampler* get_backward_light_sampler();


This method should be made const.

dictoon · 2017-07-19T14:41:00Z

src/appleseed/renderer/kernel/lighting/backwardlightsampler.cpp

@@ -565,11 +567,13 @@ float BackwardLightSampler::evaluate_pdf(const ShadingPoint& shading_point) cons
    const EmittingTriangle* triangle = m_emitting_triangle_hash_table.get(triangle_key);

    // Traverse the tree and update triangle_prob
-    const float prob = m_light_tree.light_probability(
+    float triangle_probability;
+    m_light_tree.light_probability(


I suggest to rename this method to compute_probability().

dictoon · 2017-07-19T19:47:54Z

src/appleseed/renderer/kernel/lighting/backwardlightsampler.cpp

@@ -344,18 +335,9 @@ void BackwardLightSampler::collect_emitting_triangles(
                    if (material == 0 || material->has_emission() == false)
                        continue;

-                    // Retrieve the EDF and get the importance multiplier.


We mut ensure that importance multipliers are still taken into account.

dictoon · 2017-07-19T19:49:07Z

src/appleseed/renderer/meta/tests/test_lightsampler.cpp

@@ -39,14 +40,16 @@
 using namespace foundation;
 using namespace renderer;

-TEST_SUITE(Renderer_Kernel_Lighting_LightSampler)
+TEST_SUITE(Renderer_Kernel_Lighting_LightSamplers)


Ideally we would have two test suites (and thus two source files), one per sampler type.

dictoon · 2017-07-19T19:50:27Z

sandbox/tests/test scenes/many light sampling/grid_EMT.py

-                                                "subsurface": "true",
-                                                "transparency": "true"
-                                            }},
+                                        {"visibility": {}},


You can even remove visibility and just keep {}.

dictoon · 2017-07-19T19:51:23Z

src/appleseed/renderer/kernel/lighting/forwardlightsampler.h

@@ -68,7 +61,8 @@ namespace renderer
 {

 //
-// The light sampler collects all the light-emitting entities (non-physical lights, mesh lights)
+// The backward light sampler is intended to be used with backward tracing methods.


Copy-paste error :)

dictoon · 2017-07-19T20:47:11Z

src/appleseed/renderer/kernel/lighting/backwardlightsampler.cpp

+        // Store the probability density of this light.
+        light_sample.m_probability = light_prob;
+    }
+    else if (light_type == LightSource::EmittingTriangleType)


Can it be anything else?

If not, I suggest removing the if and replacing it by an assertion:

else { assert(light_type == LightSource::EmittingTriangleType); ... }

… from LightSampler

…a specific tree node

…hed_edf to use the right intensity value

dictoon reviewed Jul 19, 2017

View reviewed changes

gospodnetic force-pushed the LT_EMT branch 3 times, most recently from 6912096 to 647496a Compare July 19, 2017 20:38

dictoon reviewed Jul 19, 2017

View reviewed changes

gospodnetic force-pushed the LT_EMT branch 2 times, most recently from c8e7739 to 4df7e59 Compare July 20, 2017 13:00

gospodnetic added 7 commits July 20, 2017 15:22

Introduce Backward- and ForwardLightSampler classes

3a58f64

Use BackwardLightSampler instead of LightSampler, extract LightSample…

e8ffe32

… from LightSampler

Use both Forward and BackwardLightSampler, cosmetics

1730324

Evaluate triangle pdf using reverse light probability evaluation for …

cbe5fc1

…a specific tree node

Make LightTree sample both NPL and EMT seamlessly

f280116

Update BackwardLightSampler to use EMTs within the tree

3c7a72c

Build the light-tree after the scene is set-up in order for the uncac…

72f7dd6

…hed_edf to use the right intensity value

gospodnetic force-pushed the LT_EMT branch 3 times, most recently from 16194c6 to 979af79 Compare July 20, 2017 14:02

gospodnetic added 2 commits July 20, 2017 16:05

Cosmetics

995ce7a

Generate EMT scene with a python script

7db25d9

gospodnetic force-pushed the LT_EMT branch from 979af79 to 7db25d9 Compare July 20, 2017 14:05

dictoon merged commit 7a8761f into appleseedhq:master Jul 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LightSampler refactored into Backward and ForwardLightSampler, use of EMTs with the LightTree #1544

LightSampler refactored into Backward and ForwardLightSampler, use of EMTs with the LightTree #1544

gospodnetic commented Jul 19, 2017

dictoon Jul 19, 2017

gospodnetic Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

gospodnetic Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

gospodnetic Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

gospodnetic Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

dictoon Jul 19, 2017

		@@ -84,14 +85,19 @@ class RendererComponents

		IFrameRenderer& get_frame_renderer();

		BackwardLightSampler* get_backward_light_sampler();

LightSampler refactored into Backward and ForwardLightSampler, use of EMTs with the LightTree #1544

LightSampler refactored into Backward and ForwardLightSampler, use of EMTs with the LightTree #1544

Conversation

gospodnetic commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment