Fix False Colors not being applied to all tiles #2896
Conversation
| @@ -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(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 callon_rendering_abortbefore it, I don't believe this is something that could lead to problems) - For
on_rendering_successonly,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"?
There was a problem hiding this comment.
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 🤣
This is an alternative solution to the bug "False Colors not being applied to all tiles".
The problem is the following:
1. We emit a signal for the end of the render **before** applying post-process.
The signal is emitted using `renderer_controller.on_rendering_success()` in `MasterRenderer::do_render()`.
2. Because of that, appleseed.studio apply False Colors before post-process.
3. False Colors gets applied on a copy of the frame, to keep the original render.
4. So if there is a post-process + False Colors, False Colors get replaced by the post-process.
One solution is to call `on_rendering_success` after applygin post-process, which order-correctly the effects:
1. Post-process is applied first
2. Then False Colors is applied on the post-processed frame
3. It works !
The problem with that solution, is that we don't call `on_rendering_success` and `on_rendering_abort` from `do_render` anymore, but `on_rendering_begin()` still is. Trigerring the signals from differents places in the code make it less easy to maintain and understand.
I think we should keep the structure in `do_render()` to make sure interactive rendering + final rendering always work the same way.
Another solution, the one in this commit, is to apply the post-process right after render. Then, no matter when you emit the signal, the frame will always be ready and will never get modified after the signal is being emitted.
|
92df33e is an alternative solution to the bug "False Colors not being applied to all tiles". The problem is the following:
One solution is to call
The problem with that solution, is that we don't call I think we should keep the structure in Another solution, the one in this commit, is to apply the post-process right after render. Then, no matter when you emit the signal, the frame will always be ready and will never get modified after the signal is being emitted. @laurelkeys What do you think about this approach ? If you agree, can you try to break it while testing it ? |
LGTM! Just tested it and it's working as expected 🙂 |
This PR addresses the same issue as #2880, namely,
False Colorsaren't correctly applied to all tiles in a final render when the frame has some post-processing stage.For a further explanation of the problem, see: https://discord.com/channels/430063582777049089/707230180233707591/729948630668148766