Fix false colors not being applied to all tiles on a final render

[laurelkeys]

This PR fixes the following bug (which happens when we have post-processing stages, and False Colors is enabled):

Edit: I undid the changes I mention below because they don't fit well with #2885 (though, I intend to add them in a later PR, once we discuss what'd lead to the best "user experience").

Also, these changes make post-processing effects always run when a rendering is stopped (e.g. Shift+F5).
If this behavior is desirable, #2878 can be closed. Otherwise, we can keeps effects only being applied to final renderings with:

@@ -226,7 +226,7 @@ struct MasterRenderer::Impl
             render_info.insert("render_time", m_project.get_rendering_timer().get_seconds());

             // Don't proceed further if rendering failed.
-            if (result.m_status == RenderingResult::Failed)
+            if (result.m_status != RenderingResult::Succeeded)
             {
                 controller.on_rendering_abort();
                 return result;
@@ -241,12 +241,7 @@ struct MasterRenderer::Impl
             // Insert post-processing time into frame's render info.
             render_info.insert("post_processing_time", stopwatch.get_seconds());

-            switch (result.m_status)
-            {
-              case RenderingResult::Succeeded: controller.on_rendering_success(); break;
-              case RenderingResult::Aborted: controller.on_rendering_abort(); break;
-              assert_otherwise;
-            }
+            controller.on_rendering_success();
         }

[oktomus]

Thanks !

I'm just concerned about the fact that we trigger on_rendering_abort for both the Aborted and Failed states. Have you heavily tested appleseed.studio ? Also, be sure to open and try different projects when testing to find any corner case.

[laurelkeys]

I'm just concerned about the fact that we trigger on_rendering_abort for both the Aborted and Failed states.

on_rendering_abort is used for both:

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

Lines 58 to 59 in 964c5a9

	// This method is called after rendering has failed or was aborted.
	virtual void on_rendering_abort() = 0;

However, since the value of m_status is set by do_render() it'll either be Aborted or Suceedded (it does not return Failed). Thus, the if (result.m_status == RenderingResult::Failed) could simply be removed.

But, if do_render() is later changed to return a Failed state we won't be treating this case (or we could add it to the switch statement, though it probably wouldn't make sense to call postprocess() if the rendering failed).

I'm not sure, this if statement was trying to be "future proof", so it may be better to remove it. What do you think?

[laurelkeys]

Have you heavily tested appleseed.studio ? Also, be sure to open and try different projects when testing to find any corner case.

I have tested it with a couple of different combinations of post-processing stages (Color Map, Render Stamp, Vignette) on quite a few images (small, large, non-square).

Also tried setting different color maps for False Colors and everything appeared to be working 😅

[oktomus]

Have you heavily tested appleseed.studio ? Also, be sure to open and try different projects when testing to find any corner case.

I have tested it with a couple of different combinations of post-processing stages (Color Map, Render Stamp, Vignette) on quite a few images (small, large, non-square).

Also tried setting different color maps for False Colors and everything appeared to be working 😅

It's really important to test different scenes on top of the default "Cornell Box" scene.

[oktomus]

The bug needs to be confirmed to continue to exist once #2877 is merged.

[laurelkeys]

The bug needs to be confirmed to continue to exist once #2877 is merged.

It's still present after commit 0acfc05.

PR #2896 adds the remaining changes to fix the issue, so I'm closing this one.