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.

Sign up for GitHub

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

Fix False Colors not being applied to all tiles #2896

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+25 −25
Open

Fix False Colors not being applied to all tiles #2896

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45fa332
Move render status signals from do_render() up to render()
laurelkeys Oct 18, 2020
92df33e
Alternative: post process the frame just after rendering
oktomus Oct 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions src/appleseed/renderer/kernel/rendering/masterrenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,22 +211,28 @@ struct MasterRenderer::Impl

try
{
// Render.
result.m_status =
do_render(
IRendererController& controller =
m_serial_renderer_controller != nullptr
? *m_serial_renderer_controller
: renderer_controller);
: renderer_controller;

// Render.
result.m_status = do_render(controller);

// Retrieve frame's render info. Note that the frame entity may have been replaced during rendering.
ParamArray& render_info = m_project.get_frame()->render_info();

// Insert rendering time into frame's render info.
render_info.insert("render_time", m_project.get_rendering_timer().get_seconds());

// Don't proceed further if rendering failed.
// Don't proceed further if rendering failed or aborted.
if (result.m_status != RenderingResult::Succeeded)
{
if (result.m_status == RenderingResult::Aborted)
controller.on_rendering_abort();

return result;
}

// Post-process.
RenderingTimer stopwatch;
Expand All @@ -237,6 +243,8 @@ struct MasterRenderer::Impl
// Insert post-processing time into frame's render info.
render_info.insert("post_processing_time", stopwatch.get_seconds());
RENDERER_LOG_INFO("post-processing time: %s.", pretty_time(stopwatch.get_seconds()).c_str());

controller.on_rendering_success();
Copy link
Member

Choose a reason for hiding this comment

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

There is quite a lot of clean-up happening in initialize_and_render_frame. I'm concerned that calling on_rendering_success and on_rendering_abort here will lead to side effects.

The controller is an abstraction made to be able to render with appleseed from studio, blender, max, cli and such. It is very easy to introduce a bug into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I see, these are the differences it makes to "bubble up" these calls:

  • The render time will be added to the frame's render info before calling on_rendering_* (while we could call on_rendering_abort before it, I don't believe this is something that could lead to problems)
  • For on_rendering_success only, postprocess() will be called before emitting it, and so will the post-processing time be added to render info

Calling on_rendering_success before post-processing the frame (as it's currently happening) seems erroneous.. and it's what actually leads to the bug. Maybe a frame's "lifetime events" should be split up into "rendering success" and something like "post process success"?

Copy link
Member

Choose a reason for hiding this comment

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

Calling on_rendering_success before post-processing the frame (as it's currently happening) seems erroneous.. and it's what actually leads to the bug. Maybe a frame's "lifetime events" should be split up into "rendering success" and something like "post process success"?

Yes, we definitely need more precision there. But it's a code not easy to edit as it impact all plugins and studio and the cli 🤣

}
catch (const std::bad_alloc&)
{
Expand Down Expand Up @@ -306,11 +314,9 @@ struct MasterRenderer::Impl
switch (status)
{
case IRendererController::TerminateRendering:
renderer_controller.on_rendering_success();
return RenderingResult::Succeeded;

case IRendererController::AbortRendering:
renderer_controller.on_rendering_abort();
return RenderingResult::Aborted;

case IRendererController::ReinitializeRendering:
Expand Down