Create a diagnostic AOV for invalid samples #1888

oktomus · 2018-03-06T12:38:40Z

This implements #1815.

oktomus · 2018-03-06T12:39:05Z

src/appleseed/renderer/kernel/rendering/final/adaptivepixelrenderer.cpp

@@ -73,6 +73,10 @@ namespace renderer

 namespace
 {
+


I will remove this line

dictoon

I like your approach, it's very sound architecturally. Clean code too! I've made a number of remarks which need to be addressed.

Thanks!

dictoon · 2018-03-07T09:04:13Z

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


 // Forward declarations.
 namespace foundation    { class Tile; }
 namespace renderer      { class AOVAccumulatorContainer; }
 namespace renderer      { class Frame; }
 namespace renderer      { class TileStack; }

+using namespace foundation;


We completely forbid using declarations in header files since this defeats the point of namespaces entirely. Please use fully qualified names.

That makes sense, we should change that also on the adaptive pixel renderer

dictoon · 2018-03-07T09:04:51Z

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


 // appleseed.foundation headers.
+#include "foundation/image/tile.h"


I presume you need this for std::unique_ptr<>? If you do, then please remove the Tile forward declaration below.

dictoon · 2018-03-07T09:05:08Z

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

@@ -32,20 +32,26 @@

 // appleseed.renderer headers.
 #include "renderer/kernel/rendering/ipixelrenderer.h"
+#include "renderer/modeling/frame/frame.h"


There's already a Frame forward declaration so this seems redundant.

Yes, my bad

dictoon · 2018-03-07T09:05:56Z

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

@@ -60,6 +66,8 @@ class PixelRendererBase
    // Constructor.


Change to plural now that there are two constructors.

dictoon · 2018-03-07T09:06:18Z

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

@@ -60,6 +66,8 @@ class PixelRendererBase
    // Constructor.
    PixelRendererBase();



Remove this blank line so that the comment applies to both constructors.

dictoon · 2018-03-07T09:13:10Z

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

@@ -41,6 +42,8 @@ using namespace foundation;

 namespace renderer
 {
+const uint8 InvalidSampleHint = 1;


Move after the "PixelRendererBase class implementation" block comment since this is technically part of the implementation of PixelRendererBase.

dictoon · 2018-03-07T09:13:37Z

src/appleseed/renderer/kernel/rendering/final/uniformpixelrenderer.h

@@ -32,6 +32,7 @@

 // appleseed.renderer headers.
 #include "renderer/kernel/rendering/ipixelrenderer.h"
+#include "renderer/modeling/frame/frame.h"


Prefer forward decls whenever possible (to reduce coupling and compile times).

dictoon · 2018-03-07T09:14:00Z

src/appleseed/renderer/kernel/rendering/final/uniformpixelrenderer.cpp

@@ -181,7 +183,7 @@ namespace
                    m_total_sampling_dim.insert(child_sampling_context.get_total_dimension());

                    // Merge the sample into the framebuffer.
-                    if (shading_result.is_valid())
+                    if (shading_result.is_valid() && shading_result.m_main.r < 0.8f)


Shouldn't be part of the PR :)

Woops, that's weird, I used --patch `to add and was careful.

dictoon · 2018-03-07T09:14:33Z

src/appleseed/renderer/kernel/rendering/final/adaptivepixelrenderer.cpp

@@ -148,6 +150,7 @@ namespace
            Tile&                       tile,
            TileStack&                  aov_tiles) override
        {
+            PixelRendererBase::on_tile_end(frame, tile, aov_tiles);


I'd suggest adding a blank line after this to make it stand out.

dictoon · 2018-03-07T09:14:35Z

src/appleseed/renderer/kernel/rendering/final/adaptivepixelrenderer.cpp

@@ -129,6 +130,7 @@ namespace
            Tile&                       tile,
            TileStack&                  aov_tiles) override
        {
+            PixelRendererBase::on_tile_begin(frame, tile, aov_tiles);


I'd suggest adding a blank line after this to make it stand out.

oktomus · 2018-03-07T09:49:15Z

Thanks for the review @dictoon. Regarding different colors for different kind of errors, I was thinking of:

Create another ShadingResult::is_valid so that we can get a error code (an enum telling what is the error)
Don't extend the original one to avoid useless tests when we don't want the AOV

dictoon · 2018-03-07T10:14:49Z

Create another ShadingResult::is_valid so that we can get a error code (an enum telling what is the error)

That sounds good, my only concerns are about complexity and especially performance. Let's try it (in a separate PR) and see how it goes!

dictoon · 2018-03-07T15:19:40Z

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

@@ -58,7 +62,7 @@ class PixelRendererBase
 {
  public:
    // Constructor.
-    PixelRendererBase();
+    PixelRendererBase(const Frame& frame, const size_t thread_index, const ParamArray& params);


You can make this more readable by having one parameter per line.

dictoon · 2018-03-07T15:20:35Z

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

+  protected:
+    const Parameters                m_base_params;
+};
+


Add a second blank line before block comments. That's the only case where we use two blank lines.

dictoon · 2018-03-07T15:21:52Z

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

-    size_t                          m_invalid_pixel_count;
+    struct Parameters
+    {
+        const bool                  m_diagnostics;


It would look nicer to forget about trying to align this to anything. There's no hard rule here, we're clearly in the domain of aesthetics :)

dictoon · 2018-03-07T15:22:17Z

src/appleseed/renderer/kernel/rendering/final/adaptivepixelrenderer.cpp

          , m_sample_renderer(factory->create(thread_index))
        {
-            if (m_params.m_diagnostics)
+            if (m_base_params.m_diagnostics)


It's quite problematic to have parameters in two places, m_base_params and m_params. I would suggest simply keeping m_base_params in the base class, and renaming it to m_params.

dictoon · 2018-03-07T15:22:55Z

src/appleseed/renderer/kernel/rendering/final/adaptivepixelrenderer.cpp

@@ -126,11 +124,13 @@ namespace

        void on_tile_begin(
            const Frame&                frame,
-            Tile&                       tile,
+            foundation::Tile&           tile,


You're in the .cpp file and there's a using namespace foundation statement so you can remove the qualification here (and everywhere in the file).

dictoon · 2018-03-07T15:23:27Z

src/appleseed/renderer/kernel/rendering/final/adaptivepixelrenderer.cpp

@@ -139,16 +139,19 @@ namespace
                    frame.aov_images().size(),
                    frame.get_filter()));

-            if (m_params.m_diagnostics)
-                m_diagnostics.reset(new Tile(tile.get_width(), tile.get_height(), 2, PixelFormatFloat));
+            if (m_base_params.m_diagnostics)


Wrap the body of multiline if statements with braces:

if (m_base_params.m_diagnostics) { ... }

dictoon · 2018-03-07T15:25:09Z

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

+                    tile.get_pixel(x, y, color);
+                    // 20% of luminance
+                    color.rgb().set(foundation::luminance(color.rgb()));
+                    color.rgb().set(color.r * 0.2f);


That looks a bit weird. Why not simply

color.rgb().set(0.2f * luminance(color.rgb()));

dictoon · 2018-03-07T15:28:16Z

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

@@ -107,4 +176,25 @@ void PixelRendererBase::signal_invalid_sample()
    ++m_invalid_sample_count;
 }

+//


Add another blank line here too.

dictoon

Thanks! A few very minor changes left and it's good to merge.

Impressed with the quality of your code!

dictoon · 2018-03-08T07:30:33Z

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

+    }
+
+    // Invalid samples diagnostic
+    if (tile_bbox.contains(pt) && m_params.m_diagnostics)


It would make sense to swap the conditions to avoid calling AABB::contains() if m_params.m_diagnostics is false.

dictoon · 2018-03-08T07:31:46Z

src/appleseed/renderer/kernel/rendering/final/uniformpixelrenderer.h

 {
  public:
    // Constructor.
    UniformPixelRendererFactory(
+        const Frame&                frame,


Need to forward declare Frame.

dictoon · 2018-03-08T07:32:44Z

src/appleseed/renderer/kernel/rendering/final/adaptivepixelrenderer.h

@@ -41,8 +41,7 @@
 #include <cstddef>

 // Forward declarations.
-namespace foundation    { class Dictionary; }
-namespace renderer      { class Frame; }


You need to restore the forward decl of Frame.

dictoon · 2018-03-08T07:33:35Z

src/appleseed/renderer/kernel/rendering/final/adaptivepixelrenderer.cpp

@@ -139,16 +142,19 @@ namespace
                    frame.aov_images().size(),
                    frame.get_filter()));

-            if (m_params.m_diagnostics)
-                m_diagnostics.reset(new Tile(tile.get_width(), tile.get_height(), 2, PixelFormatFloat));
+            if (PixelRendererBase::m_params.m_diagnostics)


Add braces around multiline body.

oktomus · 2018-03-08T09:06:11Z

Thanks, I'm happy to know that my code is of quality 😄

oktomus commented Mar 6, 2018

View reviewed changes

oktomus changed the title ~~Create a diagnostic AOV for invalid samples in adaptive sampling [WIP]~~ Create a diagnostic AOV for invalid samples in adaptive sampling Mar 6, 2018

oktomus changed the title ~~Create a diagnostic AOV for invalid samples in adaptive sampling~~ Create a diagnostic AOV for invalid samples Mar 7, 2018

dictoon requested changes Mar 7, 2018

View reviewed changes

dictoon reviewed Mar 7, 2018

View reviewed changes

dictoon reviewed Mar 8, 2018

View reviewed changes

Create an AOV for invalid samples (appleseedhq#1815)

14da087

oktomus force-pushed the invalidsamples_aov branch from e7df980 to 14da087 Compare March 8, 2018 10:14

dictoon merged commit 91e2fbd into appleseedhq:master Mar 8, 2018

oktomus deleted the invalidsamples_aov branch July 10, 2018 06:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a diagnostic AOV for invalid samples #1888

Create a diagnostic AOV for invalid samples #1888

oktomus commented Mar 6, 2018

oktomus Mar 6, 2018

dictoon left a comment

dictoon Mar 7, 2018

oktomus Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

oktomus Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

oktomus Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

oktomus commented Mar 7, 2018 •

edited

Loading

dictoon commented Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon Mar 7, 2018

dictoon left a comment

dictoon Mar 8, 2018

dictoon Mar 8, 2018

dictoon Mar 8, 2018

dictoon Mar 8, 2018

oktomus commented Mar 8, 2018


		// appleseed.foundation headers.
		#include "foundation/image/tile.h"

		@@ -60,6 +66,8 @@ class PixelRendererBase
		// Constructor.
		PixelRendererBase();

Create a diagnostic AOV for invalid samples #1888

Create a diagnostic AOV for invalid samples #1888

Conversation

oktomus commented Mar 6, 2018

Choose a reason for hiding this comment

dictoon left a 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

oktomus commented Mar 7, 2018 • edited Loading

dictoon commented Mar 7, 2018

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

dictoon left a 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

oktomus commented Mar 8, 2018

oktomus commented Mar 7, 2018 •

edited

Loading