Save extra AOVs #1865

oktomus · 2018-02-27T12:10:54Z

We are now able to:

Successfully save extra AVOS as exr
Save extra AOVs as png if possible

Fix for #1858

I am not sure if you are ok regarding changes on Frame::create_extra_aov_image.
I haven't been able to test futher with png because I have a linking issue (possible version mismatch) which cause that pngptr is null in PNGImageFileWriter::write.

I was also thinking of a different approach:
Create new AVO classes for data images and color images. Is this possible ?

est77 · 2018-02-27T12:38:32Z

Thanks for the PR!
I'll check it today after work.

dictoon · 2018-02-27T16:13:39Z

Thanks! Small remark: it's spelled AOV for Arbitrary Output Variables, not AVO. Could you fix the commit title please?

oktomus · 2018-02-27T16:45:53Z

Wooups, I know and still I made the mistake a lot

est77

Thanks for the PR.

I am not sure about saving extra AOVs to PNGs and I think the way it is done is not the best one.
I prefer that we remove that and save them as float EXRs only as a first step.

Also, with the changes in the PR, extra AOVs are always saved when saving AOVs even if the user does not want them.
I think we should add a parameter to the frame to allow users to enable / disable saving of the extra AOVs. In most cases, users will not want to save them (by default, we should not save them).

est77 · 2018-02-27T21:22:02Z

src/appleseed/renderer/modeling/frame/frame.cpp

+        const string extension = boost_file_path.extension().string();
+
+        // Extra AOVs
+        for (size_t i = 0, e = aov_images().size(); i < e; ++i)


This fragment of code is repeated. Refactor into a function in an unnamed namespace if possible,
if not, refactor to a private method of the frame.

est77 · 2018-02-27T21:24:26Z

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

@@ -91,8 +91,8 @@ namespace
        {
            if (m_params.m_diagnostics)
            {
-                m_variation_aov_index = frame.create_extra_aov_image("variation");
-                m_samples_aov_index = frame.create_extra_aov_image("samples");
+                m_variation_aov_index = frame.create_extra_aov_image("variation", PixelFormatHalf);


With these changes, the range and precision of the images used by the adaptive pixel renderer is much lower than before. This is going to affect the results of the adaptive sampling. I think we want to keep them as floats.

I chanded the format because the tile_end maps values to colors.

est77 · 2018-02-27T21:27:19Z

src/appleseed/renderer/modeling/frame/frame.cpp

-size_t Frame::create_extra_aov_image(const char* name) const
+size_t Frame::create_extra_aov_image(
+    const char* name,
+    const PixelFormat pixel_format) const


Personally, I prefer to undo this change and remove the option of saving extra aov images to pngs.
The images in the extra aov images are always non-color images and in my opinion, it doesn't make much sense to save them as pngs.

oktomus · 2018-02-27T23:12:36Z

Thanks for the review @est77, I will commit asap :)

est77 · 2018-02-28T21:26:30Z

src/appleseed/renderer/modeling/frame/frame.cpp

@@ -646,6 +649,16 @@ namespace
            }
            else if (extension == ".png")
            {
+                // Test if the image contains appropriate data


We don't need this code anymore. You can remove it.

Yes, I forgot that

est77 · 2018-02-28T21:27:44Z

src/appleseed/renderer/modeling/frame/frame.cpp

@@ -720,7 +765,7 @@ bool Frame::write_aov_images(const char* file_path) const

    bool success = true;

-    if (!aovs().empty())
+    if (!aovs().empty() || !aov_images().empty())


Remove the !aov_images().empty() check

Can you explain me why ?

est77 · 2018-02-28T21:32:14Z

src/appleseed/renderer/modeling/frame/frame.cpp

@@ -687,6 +700,38 @@ namespace

        return true;
    }
+
+    bool write_extra_aovs(
+        const ImageStack & images,


Remove space between the types and the '&' and align parameter names vertically.

est77 · 2018-02-28T21:38:22Z

Thanks for the changes.

There is a problem that I missed yesterday.
In write_extra_aovs(), we save all the images in the image stack:

for (size_t i = 0, e = images.size(); i < e; ++i)

but many AOVs also create images on the image stack.
As a result, we will save them twice, once when the AOVs are written and a second time in write_extra_aovs.

Maybe the easiest way to avoid saving some images twice is to keep a vector of the image indices created by Frame::create_extra_aov_image and in write_extra_aovs, write only those images.

oktomus · 2018-02-28T22:15:01Z

Thanks for the review, I will do that :)

oktomus · 2018-03-01T12:33:21Z

src/appleseed/renderer/modeling/frame/frame.cpp

@@ -720,27 +763,34 @@ bool Frame::write_aov_images(const char* file_path) const

    bool success = true;

-    if (!aovs().empty())


I do not check for the size anymore because if I do, I would have to check aovs().empty() and aov_images().empty(). Otherwise extra AOVs wouldn't be saved if there isn't any main AOV.

Maybe I'm wrong, there is always an AOV, beauty, right ? @est77

beauty is not an AOV in appleseed. There's an image in the frame to store it.
The idea of the check was to avoid computing the directory and other paths if there were no AOVs to save.
I think it is ok to do it outside of the loop now.

est77 · 2018-03-01T21:34:24Z

src/appleseed/renderer/modeling/frame/frame.cpp


    // Images.
    unique_ptr<Image>       m_image;
    unique_ptr<ImageStack>  m_aov_images;
    AOVContainer            m_aovs;
    AOVContainer            m_internal_aovs;
    DenoiserAOV*            m_denoiser_aov;
+	vector<size_t>          m_extra_aovs;


align the type and the var member name with the previous lines.

est77 · 2018-03-01T21:35:16Z

src/appleseed/renderer/modeling/frame/frame.cpp

@@ -280,9 +284,12 @@ const AOVContainer& Frame::internal_aovs() const

 size_t Frame::create_extra_aov_image(const char* name) const
 {
-    const size_t index = aov_images().get_index(name);
+    size_t index = aov_images().get_index(name);


index can be const. We don't modify it in the method.

est77 · 2018-03-01T21:35:43Z

src/appleseed/renderer/modeling/frame/frame.cpp

+
+    bool write_extra_aovs(
+        const ImageStack&       images,
+            const vector<size_t>&   aov_indices,


align with the rest of the arguments.

est77 · 2018-03-01T21:36:06Z

src/appleseed/renderer/modeling/frame/frame.cpp

+
+        for (size_t i = 0, e = aov_indices.size(); i < e; ++i)
+        {
+            size_t image_index = aov_indices[i];


est77 · 2018-03-01T21:43:34Z

Thanks for the changes!
Only a few very small details and we can merge.

oktomus · 2018-03-01T23:41:19Z

Thanks for the review, I'm really sorry for all these align issues.

dictoon · 2018-03-03T10:00:26Z

Very nice work @oktomus! Appreciated.

dictoon · 2018-03-03T10:01:13Z

@est77 When merging this, make sure to squash the PR into a single commit, and to remove the individual commit titles from the commit body (which GitHub fills automatically).

dictoon · 2018-03-03T09:58:26Z

src/appleseed/renderer/modeling/frame/frame.cpp

-            if (!write_image(aov_file_path.c_str(), aov->get_image(), aov))
-                success = false;
-        }
+    if (impl->m_save_extra_aovs && !aov_images().empty())


Why checking !aov_images().empty()? Isn't this test redundant?

dictoon · 2018-03-03T09:58:53Z

src/appleseed/renderer/modeling/frame/frame.cpp

+    {
+        bool success = true;
+
+        if (extension != ".exr")


Is extension guaranteed to always be a lower case?

dictoon · 2018-03-03T09:59:08Z

src/appleseed/renderer/modeling/frame/frame.cpp

+            const size_t image_index = impl->m_extra_aovs[i];
+            assert(image_index < aov_images().size());
+
+            const Image & image = aov_images().get_image(image_index);


Remove space before &.

est77 · 2018-03-03T10:05:50Z

Done!

dictoon changed the title ~~Save extra avos~~ Save extra AOVs Feb 27, 2018

Save extra aovs

e5463a4

oktomus force-pushed the save_extra_aovs branch from 31c4abe to e5463a4 Compare February 27, 2018 16:56

est77 requested changes Feb 27, 2018

View reviewed changes

oktomus added 4 commits February 28, 2018 12:25

Remove pixel format parameter when adding extra frame aovs

bc18489

Add a function to save extra aovs in anonymous namespace

24a40c0

Add a prameter on the frame to save extra aovs

426ea5e

Save extra aovs when required

99fc7d9

est77 requested changes Feb 28, 2018

View reviewed changes

oktomus added 5 commits March 1, 2018 13:00

Remove useless check when saving png image

46a1df7

Remove extra aovs size check

c951f8a

Align function parameters

539e5ee

Store a list of extra aovs

e6657ff

Fix typo

862d9ef

oktomus commented Mar 1, 2018

View reviewed changes

Fix multipart_exr when saving extra AOVs

41eabb2

est77 reviewed Mar 1, 2018

View reviewed changes

Fill review

973aee1

dictoon reviewed Mar 3, 2018

View reviewed changes

est77 merged commit 5d40768 into appleseedhq:master Mar 3, 2018

oktomus added a commit to oktomus/appleseed that referenced this pull request Mar 4, 2018

Minor fix on save extra AOVs (originally appleseedhq#1865)

e1f36c1

oktomus mentioned this pull request Mar 4, 2018

Minor fix on save extra AOVs (originally #1865) #1880

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save extra AOVs #1865

Save extra AOVs #1865

oktomus commented Feb 27, 2018 •

edited

Loading

est77 commented Feb 27, 2018

dictoon commented Feb 27, 2018 •

edited

Loading

oktomus commented Feb 27, 2018

est77 left a comment

est77 Feb 27, 2018

est77 Feb 27, 2018

oktomus Feb 27, 2018

est77 Feb 27, 2018

oktomus commented Feb 27, 2018

est77 Feb 28, 2018

oktomus Feb 28, 2018

est77 Feb 28, 2018

oktomus Feb 28, 2018

est77 Feb 28, 2018

est77 commented Feb 28, 2018

oktomus commented Feb 28, 2018

oktomus Mar 1, 2018 •

edited

Loading

oktomus Mar 1, 2018

est77 Mar 1, 2018 •

edited

Loading

est77 Mar 1, 2018

est77 Mar 1, 2018

est77 Mar 1, 2018

est77 Mar 1, 2018

est77 commented Mar 1, 2018

oktomus commented Mar 1, 2018

dictoon commented Mar 3, 2018

dictoon commented Mar 3, 2018

dictoon Mar 3, 2018

dictoon Mar 3, 2018

dictoon Mar 3, 2018

est77 commented Mar 3, 2018

		@@ -720,27 +763,34 @@ bool Frame::write_aov_images(const char* file_path) const

		bool success = true;

		if (!aovs().empty())

Save extra AOVs #1865

Save extra AOVs #1865

Conversation

oktomus commented Feb 27, 2018 • edited Loading

est77 commented Feb 27, 2018

dictoon commented Feb 27, 2018 • edited Loading

oktomus commented Feb 27, 2018

est77 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 Feb 27, 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

est77 commented Feb 28, 2018

oktomus commented Feb 28, 2018

oktomus Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

est77 Mar 1, 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

est77 commented Mar 1, 2018

oktomus commented Mar 1, 2018

dictoon commented Mar 3, 2018

dictoon commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

est77 commented Mar 3, 2018

oktomus commented Feb 27, 2018 •

edited

Loading

dictoon commented Feb 27, 2018 •

edited

Loading

oktomus Mar 1, 2018 •

edited

Loading

est77 Mar 1, 2018 •

edited

Loading