Page MenuHome

Update depsgraph before drawing the render layers
ClosedPublic

Authored by Dalai Felinto (dfelinto) on Feb 20 2018, 8:52 PM.

Diff Detail

Repository
rB Blender

Event Timeline

Fix warning on rna_render.c

Sergey Sharybin (sergey) requested changes to this revision.Feb 21 2018, 9:35 AM
Sergey Sharybin (sergey) added inline comments.
source/blender/depsgraph/intern/depsgraph_eval.cc
103

This findstring with offsets everywhere is becoming. Can we finally have an utility function for that?

I would also propose the following:

  1. Set depsgraph's scene and view_layer at the allocation time:
  2. Get rid of deg_ensure_scene_view_layer() in depsgraph_tag,.cc
  3. Remove view_layer from EvaluationContext.

All in all, see P616 (need to make it compilable).

This way you are much less likely to re-use depsgraph built for view layer A and try to evaluate it for view layer B.

Currently system is far too fragile in terms of having things running out of sync really quick.

source/blender/draw/intern/draw_manager.c
3671–3676

RE_GetEvalCtx() is to be removed. You can not have both per-view_layer and global per-Render contexts and keep things manageable and understandable.

It is only used to get object mode with this patch, and you must not initialize evaluation context to just get object mode.

3684–3686

This is another thing i totally dislike. Global static state. What could possibly go wrong.

3731–3732

This is really error-prone. What if we extend EvaluationContext further?

source/blender/render/intern/source/render_result.c
1487

Depsgraph must ever be NULL.

This revision now requires changes to proceed.Feb 21 2018, 9:35 AM
  • From review: Don't touch re->eval_ctx here

Note, we still have re->eval_ctx, which we can get rid of. But for now
(for the scope of this patch), I'm not touching it.

  • From review, depsgraph should always be valid for render layers
  • From review: Don't touch re->eval_ctx here
  • From review, depsgraph should always be valid for render layers
  • Move render depsgraph to where it belongs: RenderLayer
Move render depsgraph to where it belongs: RenderLayer

This fixes compositing for Eevee as well as lay the ground work to fix Cycles
objects iteration in blender 2.8.

Note: Work in progress, the following is not working:
* Cycles
** see "#ifdef TODO" in blender_session.cpp

* Sequencer
** pipeline.c calls BKE_sequencer_new_render_data() early on, and assumes an eval_ctx from that moment on

* Point cache texture (RE_sample_material_init)
** needs eval_ctx :/

What is not handled here: Get rid of view layer in EvaluationContext.
We should store ViewLayer in RenderLayer. The patch is massive as it is though.

Getting rid of re->eval_ctx/depsgraph is a bit messy. Part of it it is because a lot of legacy code (BI) relies on it, so for these parts my focus was on getting things building.
I'm not sure how to best handle the Cycles changes though (cc @Brecht Van Lommel (brecht)). I think we will need to refactor the logic there. Right now Sync happens only once for the entire rendering, which is a no go.

If it is up to me I wouldn't mind leaving Cycles broken and get it fixed afterwards. It's not like Cycles is 100% in 2.8 anyways (my fault likely). But I know this won't be a popular decision. I'm open for suggestions.

I also committed this patch to the temp-render-depsgraph branch if anyones wants to do some changes directly in the code.

I could do the Cycles changes, but the design is unclear to me. The simple method with least memory usage I would imagine to be something like this:

foreach(render_layer)
    depsgraph = create_depsgraph(render_layer)
    sync(depsgraph)
    free(depsgraph)
    render()

But it can be useful to keep that depsgraph around longer, for incremental updates for animation renders or re-renders. And in this patch the depsgraph is stored in the render result, so I guess it's intended to have a longer lifetime than just rendering one frame of one render layer? Or not?

There's different render results:

  1. The one owned by a Render and shown in the special Render Result image datablock.
  2. Temporary per-tile render results that can be created through the render API, that get merged into that main render result part of Render.
  3. Render results moved into render slots.

In DRW_render_to_image the depsgraph is stored in the temporary render result, so it's not persistent. This also seems incompatible with Cycles (and the render API in general) since it creates one render result per tile, not just a single big one like Eevee rendering. Even for Eevee rendering tiles could become useful at some point to render very large resolutions.

So what is the intended lifetime of this depsgraph, and why is it part of render result?

This also seems incompatible with Cycles (and the render API in general) since it creates one render result per tile, not just a single big one like Eevee rendering.

Never really understood why exactly it's doing this, but ok. I think it's already possible to know that render result is created for a tile, and in that case you don't create contexts/depsgraphs.

So what is the intended lifetime of this depsgraph, and why is it part of render result?

Every render engine is supposed to have own depsgraph, with own evaluated state of the scene. This was the whole idea of solving viewport/render data conflicts. Further, depsgraph is built for a single view layer, since it can not be evaluated for multiple viewlayers due to possible overrides.

So lifetime of depsgraphs used for F12 is somewhere between user hitting F12 and render job being finished. Now, to avoid any hash lookups or anything it seems logical to put depsgraph to a RenderLayer, since they correspond 1:1 to ViewLayers.

Guess the whole RenderResult with all its RenderLayers are stored after render is finished. That's indeed something what is not intended. Solutions could be:

  1. Free depsgraphs after render is finished (unless persistent data is requested?)
  2. Have some sort of "wrapper" around RenderLayers, which will have actual render layer and its evaluation context / depsgraph.
  3. Don't try to optimize CPU ticks, have a hash inside of Render. With some utility functions it shouldn't be that bad.

I do see a concern about memory usage though. However, proposed foreach() snipped would need to be done in the pipeline, so we don't require render engines to worry about synchronization/graph construction (otherwise we will end up with lots of places where this code would need to be kept in sync)..

intern/cycles/blender/blender_session.cpp
144

This i'm not sure why changes are needed.

Your chose worst ever way to communicate what needs to be done here. Use #if 0 and a human-readable comment about what the exact issue here, especially if you expect others to pick up and finish the work.

221

Same as above.

1321

Once again.

source/blender/blenkernel/intern/scene.c
1452

TODOs should include the string TODO in all caps, followed by the name, e-mail address, bug ID, or other identifier of the person or issue with the best context about the problem referenced by the TODO. The main purpose is to have a consistent TODO that can be searched to find out how to get more details upon request. A TODO is not a commitment that the person referenced will fix the problem. Thus when you create a TODO with a name, it is almost always your name that is given.

In short: you can not list me there since i can not give any details about what you had in mind when was typing this comment. TODO(dfelinto) is the proper way to go. It doesn't mean that you should solve the issue, it only means you have the knowledge about what is to be done.

1457–1458

Current design is that those functions are updating given depsgraph within a given context.

Technically, inputs are: scene, depsgraph, evaluation mode. And probably object mode as well nowadays.

Mode is set via eval_ctx->mode, the rest is set from depsgraph and scene.

While this might not be clear from API usage, this is how it is currently. Changing this in one single place leads you to big synchronization issues, and causes chicken-egg ambiguity.

1469

Same as above.

source/blender/depsgraph/intern/depsgraph_eval.cc
107–111

Initialize fields in the order of them in the struct.

source/blender/draw/intern/draw_manager.c
3725–3726

As i understood from yesterday, it is pipeline who calls draw manager. And it is up to pipeline to keep scene updated for the proper frame.

Basically, i do not see any justification for scene update to be here. This causes double updates of the scene, which is a complete waste.

source/blender/render/intern/source/convertblender.c
79

Can we remove BI and stop wasting time on maintaining it?

Dalai Felinto (dfelinto) marked 3 inline comments as done.Feb 22 2018, 12:45 PM
Dalai Felinto (dfelinto) updated this revision to Diff 10054.
  • From review: Initialize struct based on struct elements declaration order
  • From review: Add compreensible comments on Cycles
  • From review: Avoid chicken-egg situation with EvaluationContext/Depsgraph
  • Cleanup: Always have viewlayer when updating scene graph

Patch (and branch) updated with some of the suggestions by Sergey. My biggest pending question is regarding the creation of the render result and where do we want it.

If we leave as it is we need to at least create shallow render layers with the depsgraph/eval_ctx early on, to be updated/built during do_render_all_options. Otherwise we create the entire render result in pipeline.c, as oppose to what we do now for Cycles/Draw Manager.

source/blender/blenkernel/intern/scene.c
1452

You were the one literally suggesting to add a TODO to create an util function to handle update of new frame for all render layers at once. But sure thing (I'm actually removing this TODO, it's not really an important one).

source/blender/draw/intern/draw_manager.c
3725–3726

I see your point. That means we can't rely on creating the RenderResult inside draw manager nor inside Cycles for that matter.

Or optionally we create shallow RenderLayers inside the re->result and make sure "merging/creating render result keeps pointing to the correct depsgraph/eval_ctx around". That would also help with the issue of depsgraph/eval_ctx for the "tiles" render result to be always full valid. To be honest we may need this incorporated in the tile code in a way or another anyways.

But back to the point. Is there a good reason for result result to be created inside Cycles instead of as part of the pipeline.c?

source/blender/render/intern/source/convertblender.c
79

+1 I would love to see it gone.

source/blender/render/intern/source/render_texture.c
3819

Note to self, tackle this beauty.

Never really understood why exactly it's doing this, but ok. I think it's already possible to know that render result is created for a tile, and in that case you don't create contexts/depsgraphs.

It's mostly to make Save Buffers work, where the main render result only exists on disk so it can't be written to directly.

Guess the whole RenderResult with all its RenderLayers are stored after render is finished. That's indeed something what is not intended. Solutions could be:

  1. Free depsgraphs after render is finished (unless persistent data is requested?)
  2. Have some sort of "wrapper" around RenderLayers, which will have actual render layer and its evaluation context / depsgraph.
  3. Don't try to optimize CPU ticks, have a hash inside of Render. With some utility functions it shouldn't be that bad.

I think 3) is the right solution, store the depsgraphs in a list/hash in Render. For the persistent data case at least, for others might not even be a need to store it there.

I do see a concern about memory usage though. However, proposed foreach() snipped would need to be done in the pipeline, so we don't require render engines to worry about synchronization/graph construction (otherwise we will end up with lots of places where this code would need to be kept in sync)..

I'm fine with moving the loop over render layers out of the engines, and just passing the depsgraph to the render_to_image() method again.

If not I guess we need something like depsgraph = engine.begin_depsgraph(render_layer) and engine.end_depsgraph(depsgraph) API functions.

The consensus I gathered so far:

  • Create render result in the pipeline early on (I need to see how that affects animation, but for still frames this is straight forward).
  • Call RenderEngineType->render_to_image() for each render layer individually. The arguments we pass now (Engine and depsgraph) should be enough.

Note both Eevee and Cycles are handled through the same pipeline here. Eevee happens to just use the draw manager generic DRW_render_to_image() as its render_to_image().

  • Handle depsgraph update also outside Cycles/Draw Manager, we can even create depsgraph, and free it in between layers if memory is an issue.

The pending question would be whether to store the depsgraph in a list/hash in Render, or directly in the render layers. And how long we want the depsgraphs to live.

For this patch I would just store the depsgraph pointer on the stack, just create/destroy it around render_to_image(). Or maybe in Render if it's inconvenient to pass around the pointer to every function, but it can still have the same lifetime. Then the next step would be to keep it for (single render layer only?) animation renders, updating it incrementally for each frame.

And then later it could also take into account the R_PERSISTENT_DATA option and get cached in Render, for fast re-renders. In that case it will need to track any changes in the scene: hook into depsgraph updates (DEG_id_tag_update and friends), and get destroyed when the scene or view layer is deleted. That seems out of scope for this patch.

I see no reason to ever store the depsgraph in RenderResult or RenderLayer.

  • Depsgraph: Expose query to get view layer to RNA
  • From review: Move depsgraph away from render layer and change render loop
Create depsgraph as part of the render loop and pass a render layer for the
render_to_image. Optionally we could create a single layered render_result
but why would we?

Depsgraph is created from scratch prior to rendering each frame.
So I got rid of most of the partial update calls we had during the render
pipeline.

Also, it's past the time to remove Blender Internal.

Note 1: Cycles doesn't build.

Note 2: Cycles seems to rely on an "empty" render function, where all the
required settings are passed to the update function instead. I would like
to have this cleared out because draw manager is assuming everything is
passed to render_to_image, but we need to have an API that makes sense
to all cases.

It's closer but still not quite right I think.

We can probably get rid of the update() method for render engines. The reason it existed originally was so that the scene and UI could be locked during synchronization, and then become interactive again during render(). If accessing the depsgraph is totally thread safe that is not needed anymore.

source/blender/render/intern/source/external_engine.c
744

It's the engine that should be calling RE_engine_begin_result, for per tile render results. What should be passed to the engine is the view layer, not the render layer.

The purpose of Save Buffer is to save memory by having only a subset of render in memory, and what this does is allocate the entire image in one go, so that can't work.

762–764

The iteration over views has been moved out of the engine as well now. Almost all scene data can be shared between views so we don't want that stuff to be freed between views. The engine could have logic to keep around scene data between calls to render_to_image() for different views, it's probably good to do that anyway for things like image textures between render layers.

Still, it could be useful for engines to do the iteration themselves. Progressively rendering stereo views at the same time could be nice, and for features like adaptive subdivision we should be taking into account all camera views for coherent results. So I'm not entirely sure having the iteration over views done for the engine is good.

  • Move render ViewLayer iterator outside render_result_new
  • From review: render_to_image should not get RenderLayer
  • Fix Sequencer for new depsgraph/render changes
  • Use lean/shallow EvaluationContext for point density texture

Note: Although Eevee rendering is working (in sequencer), preview is not because it is not working in 2.8 even.

  • Bug: crash when previewing material for eevee (factory startup, show material tab in the Properties Editor).
BLI_assert failed: /home/dfelinto/src/blender/blender/source/blender/render/intern/source/render_result.c:1444, RE_RenderViewGetById(), at 'res->views.first'

Full backtrace: P626

This revision was not accepted when it landed; it landed in state Needs Review.Feb 27 2018, 10:26 PM
This revision was automatically updated to reflect the committed changes.