Page MenuHome

Fix T86682: Scene strip not evaluated correctly
ClosedPublic

Authored by Richard Antalik (ISS) on Apr 21 2021, 12:56 PM.

Details

Summary

Dependency graph for target scene wasn't active, so camera object matrix
wasn't synchronized back to original object. This caused, that DOF
distance was calculated incorrectly.

I don't know whether graph is to be set to inactive after evaluation is
done.

Diff Detail

Repository
rB Blender
Branch
T86682 (branched from master)
Build Status
Buildable 14144
Build 14144: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) requested review of this revision.Apr 21 2021, 12:56 PM
Richard Antalik (ISS) created this revision.
Sergey Sharybin (sergey) requested changes to this revision.Apr 21 2021, 1:05 PM

The active dependency graph is the one which corresponds to the current editing context of artist. The rendering can not be setting their depsgraph as active, as it will conflict with what user is doing in the editors while rendering is happening on the background.

From the description it seems that some of the area is using original object instead of evaluated for the DOF. Rendering and drawing is always to use evaluated data, so to me it sounds like it is a bug in the place where the DOF is implemented.

The second thing is that the BKE_scene_ensure_depsgraph() is not a right choice in seq_render_scene_strip(): it creates depsgraph for viewport, so if you are using scene strip which has an object with subsurf modifier with different subdivision level the final F12 render will use viewport subdivision level. Additionally, the code evaluates depsgraph from rendering thread while it might be used by artists. That being said, this is a deeper problem is not directly related to the original DOF issue you're looking into.

This revision now requires changes to proceed.Apr 21 2021, 1:05 PM

The active dependency graph is the one which corresponds to the current editing context of artist. The rendering can not be setting their depsgraph as active, as it will conflict with what user is doing in the editors while rendering is happening on the background.

From the description it seems that some of the area is using original object instead of evaluated for the DOF. Rendering and drawing is always to use evaluated data, so to me it sounds like it is a bug in the place where the DOF is implemented.

Right, this sounds reasonable.
In example file from report, when you switch from VSE scene to target scene and bact to VSE scene, things will start to work, because depsgraph will remain active. That sounds like a bug?

The second thing is that the BKE_scene_ensure_depsgraph() is not a right choice in seq_render_scene_strip(): it creates depsgraph for viewport, so if you are using scene strip which has an object with subsurf modifier with different subdivision level the final F12 render will use viewport subdivision level. Additionally, the code evaluates depsgraph from rendering thread while it might be used by artists. That being said, this is a deeper problem is not directly related to the original DOF issue you're looking into.

I am not sure if it is correct, but BKE_scene_ensure_depsgraph() is used only for off-screen viewport renders, not for render job. That said, when render job is used, DOF is evaluated correctly (DEG_graph_build_for_render_pipeline() is used in this case).

How about this then (seems to work well):

diff --git a/source/blender/sequencer/intern/render.c b/source/blender/sequencer/intern/render.c
index 572fff0ad38..588629fbea0 100644
--- a/source/blender/sequencer/intern/render.c
+++ b/source/blender/sequencer/intern/render.c
@@ -1530,27 +1530,29 @@ static ImBuf *seq_render_scene_strip(const SeqRenderData *context,

     /* for old scene this can be uninitialized,
      * should probably be added to do_versions at some point if the functionality stays */
     if (context->scene->r.seq_prev_type == 0) {
       context->scene->r.seq_prev_type = 3 /* == OB_SOLID */;
     }

     /* opengl offscreen render */
     depsgraph = BKE_scene_ensure_depsgraph(context->bmain, scene, view_layer);
     BKE_scene_graph_update_for_newframe(depsgraph);
+    Object *camera_eval = DEG_get_evaluated_object(depsgraph, camera);
+
     ibuf = sequencer_view3d_fn(
         /* set for OpenGL render (NULL when scrubbing) */
         depsgraph,
         scene,
         &context->scene->display.shading,
         context->scene->r.seq_prev_type,
-        camera,
+        camera_eval,
         width,
         height,
         IB_rect,
         draw_flags,
         scene->r.alphamode,
         viewname,
         context->gpu_offscreen,
         err_out);
     if (ibuf == NULL) {
       fprintf(stderr, "seq_render_scene_strip failed to get opengl buffer: %s\n", err_out);

In example file from report, when you switch from VSE scene to target scene and bact to VSE scene, things will start to work, because depsgraph will remain active. That sounds like a bug?

More like a side-effect of using the wrong "domain" (as in, using original object instead of the evaluated one).

I am not sure if it is correct, but BKE_scene_ensure_depsgraph() is used only for off-screen viewport renders, not for render job.

Would be nice to verify that. But is better to happen outside of this patch.
Is something I was just noticing while looking into the code surrounding the patch.

How about this then (seems to work well):

If the scene and and seq are all original, then I think the change will work well for now.

Something for future is to make it so rendering happens using sequences which are known to be from an evaluated depsgraph. Will avoid call like DEG_get_evaluated_object. Generally the idea is that the depsgraph prepares data for drawing/rendering, and the draw manager/render engine/etc are visualizing what was prepared for them, without worrying about whether something is original or evaluated. In practice that would mean that somewhere at the top calls of seq_render_scene_strip() we'll be iterating sequences from scene which comes from depsgraph. But then again, this is something for the future (outside of this patch, because that'd be a bigger refactor).

P.S. On a second though think your new snippet with camera_eval is fine. Thing is, BKE_scene_ensure_depsgraph is supposed to be used for original scene, so doing depsgraph and querying evaluated camera does not introduce new design violation. So lets roll with your snippet!

This revision was not accepted when it landed; it landed in state Needs Revision.Apr 22 2021, 8:25 AM
This revision was automatically updated to reflect the committed changes.