Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save extra AOVs #1865

Merged
merged 12 commits into from
Mar 3, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

m_samples_aov_index = frame.create_extra_aov_image("samples", PixelFormatHalf);

if ((thread_index == 0) && (m_variation_aov_index == size_t(~0) || m_samples_aov_index == size_t(~0)))
{
Expand Down
58 changes: 55 additions & 3 deletions src/appleseed/renderer/modeling/frame/frame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -278,11 +278,13 @@ const AOVContainer& Frame::internal_aovs() const
return impl->m_internal_aovs;
}

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
const size_t index = aov_images().get_index(name);
if (index == size_t(~0) && aov_images().size() < MaxAOVCount)
return aov_images().append(name, 4, PixelFormatFloat);
return aov_images().append(name, 4, pixel_format);

return index;
}
Expand Down Expand Up @@ -646,6 +648,16 @@ namespace
}
else if (extension == ".png")
{
// Test if the image contains appropriate data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I forgot that

if (image.properties().m_pixel_format != PixelFormatHalf &&
image.properties().m_channel_count != 4)
{
RENDERER_LOG_ERROR(
"failed to write image file %s: data images can only be saved as exr.",
file_path);
return false;
}

write_png_image(
bf_file_path,
image,
Expand Down Expand Up @@ -720,7 +732,7 @@ bool Frame::write_aov_images(const char* file_path) const

bool success = true;

if (!aovs().empty())
Copy link
Member Author

@oktomus oktomus Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@est77 est77 Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!aovs().empty() || !aov_images().empty())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the !aov_images().empty() check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain me why ?

{
const bf::path boost_file_path(file_path);
const bf::path directory = boost_file_path.parent_path();
Expand All @@ -741,6 +753,22 @@ bool Frame::write_aov_images(const char* file_path) const
if (!write_image(aov_file_path.c_str(), aov->get_image(), aov))
success = false;
}

// Extra AOVs
for (size_t i = 0, e = aov_images().size(); i < e; ++i)
{
const Image & image = aov_images().get_image(i);

// Compute AOV image file path.
const string aov_name = aov_images().get_name(i);
const string safe_aov_name = make_safe_filename(aov_name);
const string aov_file_name = base_file_name + "." + safe_aov_name + extension;
const string aov_file_path = (directory / aov_file_name).string();

// Write AOV image.
if (!write_image(aov_file_path.c_str(), image))
success = false;
}
}

return success;
Expand Down Expand Up @@ -776,6 +804,30 @@ bool Frame::write_main_and_aov_images() const
}
}

if (!aov_images().empty())
{
const bf::path boost_file_path(filepath);
const bf::path directory = boost_file_path.parent_path();
const string base_file_name = boost_file_path.stem().string();
const string extension = boost_file_path.extension().string();

// Extra AOVs
for (size_t i = 0, e = aov_images().size(); i < e; ++i)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
const Image & image = aov_images().get_image(i);

// Compute AOV image file path.
const string aov_name = aov_images().get_name(i);
const string safe_aov_name = make_safe_filename(aov_name);
const string aov_file_name = base_file_name + "." + safe_aov_name + extension;
const string aov_file_path = (directory / aov_file_name).string();

// Write AOV image.
if (!write_image(aov_file_path.c_str(), image))
success = false;
}
}

return success;
}

Expand Down
5 changes: 4 additions & 1 deletion src/appleseed/renderer/modeling/frame/frame.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
// appleseed.foundation headers.
#include "foundation/image/canvasproperties.h"
#include "foundation/image/colorspace.h"
#include "foundation/image/pixel.h"
#include "foundation/math/aabb.h"
#include "foundation/math/filter.h"
#include "foundation/math/vector.h"
Expand Down Expand Up @@ -101,7 +102,9 @@ class APPLESEED_DLLSYMBOL Frame
const AOVContainer& aovs() const;

// Create an extra AOV image if it does not exist.
size_t create_extra_aov_image(const char* name) const;
size_t create_extra_aov_image(
const char* name,
const foundation::PixelFormat pixel_format = foundation::PixelFormatFloat) const;

// Return the reconstruction filter used by the main image and the AOV images.
const foundation::Filter2f& get_filter() const;
Expand Down