Use regular AOVs to export diagnostic AOVs #2121

oktomus · 2018-07-16T10:29:56Z

Copy diagnostic AOVs into real AOVs
Remove enable_diagnostics option and check whether the use has set the wanted AOV
Don't create extra aovs but directly write into regular aovs from the pixel renderers
? Write directly in the AOV
Post process in the AOV and not in the renderer
? Use write() to detect invalid samples
Check in both regular and internal aovs instead of aov_images -> not required

fix #2068

est77 · 2018-07-16T19:28:30Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+            {
+                const string aov_name = frame.aov_images().get_name(i);
+
+                if (aov_name == m_diagnostic_name)


Do the linear search only the first time it is needed. The image does not change while the AOVAccumulator is alive and accumulators are not shared between threads.

const Image* m_image = nullptr;
if (!m_image) { do the search here... }

est77 · 2018-07-16T19:34:31Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+
+        size_t get_channel_count() const override
+        {
+            return 4;


Do we need 4 channels?
Some of the previous AOVs use only one (you can save them using 1 channel called "Y") and the pixel variation is RGB (no need for A).

Yes I will improve that when implementing per-aov post process

est77 · 2018-07-18T19:07:46Z

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

-                    else signal_invalid_sample();
+                    else
+                    {
+                        signal_invalid_sample();


Remove the braces, leave the code as it was before.
Do the same in the other signal_invalid_sample() call a few lines after.

est77 · 2018-07-18T19:19:03Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+    // Diagnostic AOV accumulator.
+    //
+
+    class DiagnosticAOVAccumulator


We are not using this class, no?

it is used by PixelVariation and PixelSample

dictoon · 2018-07-20T05:29:45Z

src/appleseed/renderer/modeling/aov/pixeltimeaov.cpp

@@ -218,16 +218,14 @@ namespace
            m_image->clear(Color<float, 1>(0.0f));
        }

-        void post_process_image() override
+        void post_process_image(const AABB2u& bbox) override


You must inclde foundation/math/aabb.h in all files that now require it.

dictoon · 2018-07-20T05:30:40Z

src/appleseed/renderer/modeling/aov/aov.h

@@ -90,7 +91,7 @@ class APPLESEED_DLLSYMBOL AOV
    virtual void clear_image();

    // Apply any post processing needed to the AOV image.
-    virtual void post_process_image();
+    virtual void post_process_image(const foundation::AABB2u& bbox);


I believe bbox should be called crop_window for clarity.

dictoon · 2018-07-20T05:31:14Z

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

-{
-  public:
-    static foundation::Dictionary get_params_metadata();
+    size_t m_invalid_pixel_count;


Make sure to remove header files that are no longer required.

dictoon · 2018-07-20T05:32:11Z

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

@@ -79,7 +79,7 @@ namespace
            ISampleRendererFactory*     factory,
            const ParamArray&           params,
            const size_t                thread_index)
-          : PixelRendererBase(frame, thread_index, params)
+          : PixelRendererBase()


You no longer need to explicitly invoke the constructor here.

oktomus · 2018-07-20T07:45:15Z

@dictoon I will address your review in the adaptive PR :)

dictoon · 2018-07-20T12:39:36Z

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

+
+                if (m_sample_aov_tile)
+                {
+                    value[0] = trackers[0].get_size();


You need a static_cast<> here as in general this will loose precision.

dictoon · 2018-07-20T12:41:00Z

src/appleseed/renderer/kernel/rendering/generic/generictilerenderer.cpp

            // Inform the AOV accumulators that we are about to render a tile.
            m_aov_accumulators.on_tile_begin(
                frame,
                tile_x,
                tile_y,
                m_pixel_renderer->get_max_samples_per_pixel());

+            // Inform the pixel renderer that we are about to render a tile.
+            m_pixel_renderer->on_tile_begin(


Why did you move this?

I don't remember, I think it was for the first implementation of DiagnosticAOV. I'm going to check if it's still required and why if so.

not required anymore.

dictoon · 2018-07-20T12:43:42Z

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


-    // todo: mark pixel as faulty in the diagnostic map.


If I understand correctly this todo is done because we now have a dedicated Invalid Samples AOV?

dictoon · 2018-07-20T12:45:49Z

src/appleseed/renderer/modeling/aov/aovfactoryregistrar.cpp

    register_factory(auto_release_ptr<FactoryType>(new NormalAOVFactory()));
+    register_factory(auto_release_ptr<FactoryType>(new PixelSampleAOVFactory()));


I feel that this AOV name is ambiguous, I would suggest PixelSampleCountAOV[Factory].

dictoon · 2018-07-20T12:46:12Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+
+    //
+    // Invalid Sample AOV accumulator.
+    //


Remove this line.

You want me to remove the whole block ?

dictoon · 2018-07-20T12:46:30Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+        {
+        }
+    };
+


Always precede block comments by two blank lines.

dictoon · 2018-07-20T12:46:53Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+    {
+      public:
+        explicit InvalidSampleAOVAccumulator(
+            Image& image)


Move argument to precedent line, there's no line length problem here.

dictoon · 2018-07-20T12:47:06Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+        }
+
+        void on_tile_begin(
+                const Frame&                frame,


Fix indentation.

dictoon · 2018-07-20T12:47:32Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+        }
+
+        void on_tile_end(
+                const Frame&                frame,


Fix indentation.

dictoon · 2018-07-20T12:47:52Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+
+        void on_pixel_end(const Vector2i& pi) override
+        {
+            // Store a hint corresping to the sample state in the tile.


dictoon · 2018-07-20T12:48:45Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+
+            m_tile_origin_x = static_cast<int>(tile_x * props.m_tile_width);
+            m_tile_origin_y = static_cast<int>(tile_y * props.m_tile_height);
+            m_tile_end_x = static_cast<int>(m_tile_origin_x + m_invalid_sample_tile->get_width() - 1);


Traditionally "end" indices are exclusive, i.e. you don't subtract 1 here, and you change <= to < when checking if pixels fall inside the tile. Simpler math and increased consistency.

dictoon · 2018-07-20T12:50:08Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+
+    const uint8 NoState = 0;
+    const uint8 InvalidSample = 1;
+    const uint8 CorrectSample = 2;


CorrectSample -> ValidSample (because "valid" is the opposite of "invalid", while "correct" is the opposite of "incorrect").

dictoon · 2018-07-20T12:50:58Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+      : public AOV
+    {
+      public:
+        explicit DiagnosticAOV(const char* name, const ParamArray& params)


Remove explicit.

Why is this AOV named while the others aren't?

Because other AOVs inherit from this one.

Why removing explicit ?

Because it's not useful for constructors with more than one argument... Or at least that was true until C++11: https://stackoverflow.com/a/39122237/393756

It seems that it makes sense starting with C++11 to mark all constructors as explicit... Today I learn!

You can leave explicit then. If you already remove it then no big deal, it's fine too.

dictoon · 2018-07-20T12:52:09Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+
+            Color3f color;
+
+            for (size_t j = bbox.min.y; j <= bbox.max.y; ++j)


It would be more logical to use x and y to iterate.

dictoon · 2018-07-20T12:52:58Z

src/appleseed/renderer/modeling/aov/diagnosticaov.cpp

+    return
+        Dictionary()
+            .insert("name", Invalid_Sample_Model)
+            .insert("label", "Invalid Sample");


"Invalid Samples".

The AOV needs to be renamed too.

Use regular AOVs to export diagnostic AOVs

784fcc1

oktomus requested a review from est77 July 16, 2018 10:29

oktomus added 3 commits July 16, 2018 14:51

Merge together diagnostic aovs

cf49d4a

Write directly in the AOVs from the adaptive pixel renderer

ddcbae4

Remove all diagnostics option

cabc4fa

est77 requested changes Jul 16, 2018

View reviewed changes

dictoon changed the title ~~Use regular AOVs to export diagnostic AOVs~~ [WIP] Use regular AOVs to export diagnostic AOVs Jul 17, 2018

oktomus added 4 commits July 17, 2018 14:23

Create a real AOV for invalid samples

d7c800f

Remove extra aovs from the frame

62fdb3b

Remove alpha channel from invalid sample AOV

d5878c8

Post process AOVs

8c9859a

oktomus changed the title ~~[WIP] Use regular AOVs to export diagnostic AOVs~~ Use regular AOVs to export diagnostic AOVs Jul 17, 2018

oktomus changed the title ~~Use regular AOVs to export diagnostic AOVs~~ [WIP] Use regular AOVs to export diagnostic AOVs Jul 17, 2018

Prevent diagnostic AOVs from overlapping crop area

0a67f7c

oktomus changed the title ~~[WIP] Use regular AOVs to export diagnostic AOVs~~ Use regular AOVs to export diagnostic AOVs Jul 17, 2018

dictoon added the PR | Squash This PR must be squashed when merged. label Jul 18, 2018

oktomus added 3 commits July 18, 2018 09:57

Warn the user about invalid samples in the pixel renderer

4d13962

Don't check for key when removing a value in the project updater

9cc1c7e

Remove unused variable

f71da2f

est77 requested changes Jul 18, 2018

View reviewed changes

Minor tweak

3716392

est77 approved these changes Jul 19, 2018

View reviewed changes

est77 merged commit 9e3980f into appleseedhq:master Jul 19, 2018

dictoon reviewed Jul 20, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use regular AOVs to export diagnostic AOVs #2121

Use regular AOVs to export diagnostic AOVs #2121

oktomus commented Jul 16, 2018 •

edited

Loading

est77 Jul 16, 2018

est77 Jul 16, 2018

oktomus Jul 16, 2018

est77 Jul 18, 2018

est77 Jul 18, 2018

oktomus Jul 19, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

oktomus commented Jul 20, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

oktomus Jul 23, 2018

oktomus Jul 23, 2018

dictoon Jul 20, 2018

oktomus Jul 23, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

oktomus Jul 23, 2018

dictoon Jul 20, 2018 •

edited

Loading

dictoon Jul 20, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

oktomus Jul 23, 2018

oktomus Jul 23, 2018

dictoon Jul 25, 2018

dictoon Jul 20, 2018

dictoon Jul 20, 2018

		register_factory(auto_release_ptr<FactoryType>(new NormalAOVFactory()));
		register_factory(auto_release_ptr<FactoryType>(new PixelSampleAOVFactory()));


		Color3f color;

		for (size_t j = bbox.min.y; j <= bbox.max.y; ++j)

Use regular AOVs to export diagnostic AOVs #2121

Use regular AOVs to export diagnostic AOVs #2121

Conversation

oktomus commented Jul 16, 2018 • edited Loading

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 Jul 20, 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

Choose a reason for hiding this comment

dictoon Jul 20, 2018 • edited Loading

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 Jul 16, 2018 •

edited

Loading

dictoon Jul 20, 2018 •

edited

Loading