appleseedhq / appleseed

A modern open source rendering engine for animation and visual effects
https://appleseedhq.net/
MIT License
2.19k stars 329 forks source link

Fix Shift+F5 causing False Colors to be applied twice #2877

Closed laurelkeys closed 3 years ago

laurelkeys commented 4 years ago

The color map post-processing stage, applied when False Colors is enabled, is being called twice on Stop Rendering (Shift+F5).


This happens by on_rendering_abort() being called and emitting a signal_rendering_abort() at: https://github.com/appleseedhq/appleseed/blob/964c5a91d27760370f20b15f07088dc6a0064e36/src/appleseed/renderer/kernel/rendering/masterrenderer.cpp#L173

which also results in emitting signal_rendering_failed() on: https://github.com/appleseedhq/appleseed/blob/964c5a91d27760370f20b15f07088dc6a0064e36/src/appleseed.studio/mainwindow/rendering/renderingmanager.cpp#L113-L117

Thus, slot_rendering_end() runs twice.


I'm submitting a "conservative" fix, as it only prevents False Colors from being applied when a "failed" signal is emitted (other previous behavior stay the same).

However, it would be interesting to remove unnecessary computations that are being run twice. Creating a new signal_rendering_aborted() on renderingmanager.cpp could help splitting what should be run on "aborting" from "failing", e.g.:

- if (rendering_result.m_status != MasterRenderer::RenderingResult::Succeeded)
-     emit signal_rendering_failed();
+ switch (rendering_result.m_status)
+ {
+   case MasterRenderer::RenderingResult::Succeeded:
+     break;
+ 
+   case MasterRenderer::RenderingResult::Aborted:
+     emit signal_rendering_aborted();
+     break;
+ 
+   default:
+     emit signal_rendering_failed();
+     break;
+ }
laurelkeys commented 4 years ago

Thanks !

Creating a new signal_rendering_aborted() on renderingmanager.cpp could help splitting what should be run on "aborting" from "failing"

You mean a new signal_rendering_failed ? If so, I think it's a good idea.

Sorry, on_rendering_abort() emits signal_rendering_abort() (not signal_rendering_aborted() as I had previously written at the PR description, this isn't declared).

The MasterRendererThread inside renderingmanager.cpp has a single "failed" signal, which it emits both when m_status is Aborted and Failed (line 116 of the second code snippet in the description above).

So, adding an alternative for signal_rendering_failed to be called when the render is aborted (e.g. signal_rendering_aborted) would give us more information:

image