Page MenuHome

Dependency graph API changes
ClosedPublic

Authored by Sergey Sharybin (sergey) on May 9 2019, 5:28 PM.

Details

Summary

Main goal here is to make it obvious and predictable about
what is going on.

Summary of changes.

  • Access to dependency graph is now only possible to a fully evaluated graph. This is now done via context.evaluated_depsgraph_get().

    The call will ensure both relations and datablocks are updated.

    This way we don't allow access to some known bad state of the graph, and also making explicit that getting update dependency graph is not cheap.
  • Access to evaluated ID is now possible via id.evaluated_get().

    It was already possible to get evaluated ID via dependency graph, but that was a bit confusing why access to original is done via ID and to evaluated via depsgraph.

    If datablock is not covered by dependency graph it will be returned as-is.
  • Similarly, request for original from an ID which is not evaluated will return ID as-is.
  • Removed scene.update().

    This is very expensive to update all the view layers.
  • Added depsgraph.update().

    Now when temporary changes to objects are to be done, this is to happen on original object and then dependency graph is to be updated.
  • Changed object.to_mesh() to behave the following way:
    • When is used for original object modifiers are ignored.

      For meshes this acts similar to mesh-copy, not very useful but allows to keep code paths similar (i.e. for exporter which has Apply Modifiers option it's only matter choosing between original and evaluated object, the to_mesh() part can stay the same).

      For curves this gives a mesh which is constructed from displist without taking own modifiers and modifiers of bevel/taper objects into account.

      For metaballs this gives empty mesh. Polygonization of metaball is not possible from a single object.
    • When is used for evaluated object modifiers are always applied.

      In fact, no evaluation is happening, the mesh is either copied as-is, or constructed from current state of curve cache.

      Arguments to apply modifiers and calculate original coordinates (ORCO, aka undeformed coordinates) are removed. The ORCO is to be calculated as part of dependency graph evaluation.

File used to regression-test (a packed Python script into .blend):

Patch to make addons tests to pass:

NOTE: I've included changes to FBX exporter, and those are addressing report T63689.
NOTE: All the enabled-by-default addons are to be ported still, but first want to have agreement on this part of changes.
NOTE: Also need to work on documentation for Python API, but, again, better be done after having agreement on this work.

Diff Detail

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

Event Timeline

Overall LGTM, did not see anything wrong in the BKE_mesh_new_from_object() (which is the biggest change here), and new code is sooo much well organized. :)

Noted some details (mostly typos in comments) below.

Also, FBX changes are not included here (probably because they are in a different repo?), wouldn’t mind having a look at them (I would expect the 'without modifier' case for non-mesh objects to be a bit annoying to handle now, since we'll have to temporarily disable all modifiers on curves etc. to support that, right?).

source/blender/blenkernel/BKE_context.h
315

yet, it will be ;)

source/blender/blenkernel/intern/mesh_convert.c
935–937

want them to be applied ;)

947–956

What is the point of this, since in only caller those get reset to NULL immediately?

1048

or lack of evaluated

1068

Just use Depsgraph *UNUSED(depsgraph) in function's parameters definition?

1103

an user.

source/blender/depsgraph/DEG_depsgraph_query.h
106

not out-of-main localized

source/blender/depsgraph/intern/depsgraph_query.cc
277

might create special datablocks which have all the

source/blender/makesrna/intern/rna_ID.c
1456

evaluated ID from

source/blender/makesrna/intern/rna_context.c
310–316

In theory, aren’t we supposed to put RNA functions into rna_xxx_api.c files? Not sure how strict we are on that policy currently (suspect it's quiet weak, given the amount of places not respecting it), just noting.

314

No final point in RNA descriptions. ;)

source/blender/makesrna/intern/rna_main_api.c
947–952

Descriptions here are to be updated to reflect new behavior I guess?

source/blender/makesrna/intern/rna_object_api.c
878–885

Update descriptions here too?

@Bastien Montagne (mont29), see attached depsgraph_api_changes.diff. Includes update for OBJ and FBX.

P.S. Should have chosen a better name for the file.

Sergey Sharybin (sergey) retitled this revision from Main goal here is to make it obvious and predictable about what is going on. to Dependency graph API changes.May 10 2019, 12:07 PM
Sergey Sharybin (sergey) edited the summary of this revision. (Show Details)
Sergey Sharybin (sergey) edited the summary of this revision. (Show Details)
source/blender/blenkernel/intern/mesh_convert.c
947–956

Purpose is to produce object which is totally ready for evaluation, even if BKE_displist_make_curveTypes_forRender() is used on it (similar to the old code).

The fact that we don't do display list conversion is more of detail. Because we might reconsider that later.

On another hand, if we don't do on-demand display list creation this entire code must not exist. The only reason for it is because BKE_mesh_from_nurbs_displist() is badly designed and written, and does a lot of things it must not be doing. Changing that is outside of the scope of this change.

source/blender/makesrna/intern/rna_context.c
310–316

But there is no such a file yet, and i don't see this to be used a lot. More like if there is a huge amount of function calls then _api file is created.

source/blender/makesrna/intern/rna_main_api.c
947–952

Updated the function description.

Dependency graph argument is actually annoying one here.

I feel liker we can get rid of it if we are happy with BKE_mesh_new_from_object(). Dependency graph is only used to check whether mball is a motherball or not. But think we can replace that check with check whether displist exists for the object. I don't see how properly evaluated motherball will not have displist, and can't see why non-motherballs will have a displist.

Brecht Van Lommel (brecht) requested changes to this revision.May 10 2019, 1:45 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/makesrna/intern/rna_context.c
311

I think the convention is for API functions to start with the verb? So this would be get_evaluated_depsgraph().

I could even be called evaluate_depsgraph() to make it clear that this does some work, but I'm not sure.

313–314

This description should make it clear that it's an expensive operation, and could give some guidance on what to use it for. Suggested description:

Get the dependency graph for the current scene and view layer, to access to data-blocks with animation and modifiers applied. If any data-blocks have been edited, the dependency graph will be updated. This invalidates all references to evaluated data-blocks from the dependency graph.
source/blender/makesrna/intern/rna_depsgraph.c
649

Suggested description:

Re-evaluate any modified data-blocks, for example for animation or modifiers. This invalidates all references to evaluated data-blocks from this dependency graph.

Relations being up to date seems more like an implementation detail that API users do not need to be aware of.

This revision now requires changes to proceed.May 10 2019, 1:45 PM

Ah, did not realized that diff covered FBX and OBJ… In fact, it is missing (for FBX) what I was saying, that is, with current new code when exporting without modifiers, you won't get anything for non-mesh objects. It should, for that case, do similar trick as what is done with armature modifier, that is something like that (also fixed mistake in object on which you where calling to_mesh()):

1diff --git a/io_scene_fbx/export_fbx_bin.py b/io_scene_fbx/export_fbx_bin.py
2index cbcc8675..e17adac2 100644
3--- a/io_scene_fbx/export_fbx_bin.py
4+++ b/io_scene_fbx/export_fbx_bin.py
5@@ -2205,10 +2205,21 @@ def fbx_data_from_scene(scene, depsgraph, settings):
6 mod.show_viewport = False
7 if mod.show_render or mod.show_viewport:
8 use_org_data = False
9+ if ob.type in BLENDER_OTHER_OBJECT_TYPES and not settings.use_mesh_modifiers:
10+ # If we want to export non-mesh geometry without modifiers, we have to disable them temporarily,
11+ # and still get mesh from evaluated object (getting mesh from orig non-mesh object returns nothing).
12+ for mod in ob.modifiers:
13+ tmp_mods.append((mod, mod.show_render, mod.show_viewport))
14+ mod.show_render = False
15+ mod.show_viewport = False
16+ # If modifiers has been altered, we need to update dependency graph.
17+ if tmp_mods:
18+ depsgraph.update()
19 if not use_org_data:
20- tmp_me = ob.to_mesh(
21- depsgraph,
22- apply_modifiers=settings.use_mesh_modifiers)
23+ # We always want to convert from evaluated data at this point
24+ # (only case where we do not want that, which is mesh object without exporting modifiers,
25+ # will never lead to that point).
26+ tmp_me = ob.evaluated_get(depsgraph).to_mesh(depsgraph)
27 data_meshes[ob_obj] = (get_blenderID_key(tmp_me), tmp_me, True)
28 # Re-enable temporary disabled modifiers.
29 for mod, show_render, show_viewport in tmp_mods:
30@@ -3089,7 +3100,8 @@ def save(operator, context,
31 ctx_objects = context.view_layer.objects
32 kwargs_mod["context_objects"] = ctx_objects
33
34- ret = save_single(operator, context.scene, context.depsgraph, filepath, **kwargs_mod)
35+ depsgraph = context.evaluated_depsgraph_get()
36+ ret = save_single(operator, context.scene, depsgraph, filepath, **kwargs_mod)
37 else:
38 # XXX We need a way to generate a depsgraph for inactive view_layers first...
39 # XXX Also, what to do in case of batch-exporting scenes, when there is more than one view layer?
40diff --git a/io_scene_obj/export_obj.py b/io_scene_obj/export_obj.py
41index 34a279f4..08074089 100644
42--- a/io_scene_obj/export_obj.py
43+++ b/io_scene_obj/export_obj.py
44@@ -347,8 +347,10 @@ def write_file(filepath, objects, depsgraph, scene,
45 continue
46 # END NURBS
47
48+ ob_for_convert = ob.evaluated_get(depsgraph) if EXPORT_APPLY_MODIFIERS else ob.original_get()
49+
50 try:
51- me = ob.to_mesh(depsgraph, EXPORT_APPLY_MODIFIERS)
52+ me = ob_for_convert.to_mesh(depsgraph)
53 except RuntimeError:
54 me = None
55
56@@ -678,7 +680,7 @@ def _write(context, filepath,
57 base_name, ext = os.path.splitext(filepath)
58 context_name = [base_name, '', '', ext] # Base name, scene name, frame number, extension
59
60- depsgraph = context.depsgraph
61+ depsgraph = context.evaluated_depsgraph_get()
62 scene = context.scene
63
64 # Exit edit mode before exporting, so current object states are exported properly.

source/blender/blenkernel/intern/mesh_convert.c
947–956

OK, see the point, indeed BKE_mesh_from_nurbs_displist() is a horrible piece of code…

source/blender/makesrna/intern/rna_main_api.c
947–952

Would be nice to get rid of that depsgrah argument if possible, yes…

Also, I wouldn’t mind a note about how you can pass original or evaluated object, at least (and ideally what's the difference between both cases). Reason is, those end up directly in API doc, so while having more comprehensive description of the process to get mesh from objects elsewhere is fine, I think it is worth having basics covered directly in func doc? Something like that maybe:

Add a new mesh created from given object (original undeformed geometry if object is original, and final evaluated geometry, with all modifiers etc., if object is evaluated)

Slightly longer description are not a problem here, those won't show in UI, only in API doc.

source/blender/makesrna/intern/rna_object_api.c
879

the current state ;)

source/blender/makesrna/intern/rna_context.c
311

I don’t think so? That would be opposite of our C 'API' conventions. And in rna_object_api.c e.g. all accessor functions end with _get()/_set()

Think the one starting with a verb are older ones?

source/blender/makesrna/intern/rna_context.c
311

Eugh. In C i think we've got verb as a suffix. A bit confusing, but fine changing it in Python API.

evaluate_depsgraph() indeed sounds in a way that implies there is work to be done. But it doesn't imply that it gives you dependency graph. Is it more like evaluate_and_get_depsgraph()? Kinda too verbose.

313–314

This invalidates all references to evaluated data-blocks from the dependency graph.

Not really correct. References to sub-data is invalidated. References to the datablock itself are not.

While this seems like implementation details, there are a lot of places which are dependent on this, and being aware of this fact will speed up things like baking type of operations: one can look up evaluated datablock once, then keep updating dependency graph without looking up datablock again.

I couldn't spot issues from reading the code, only some naming things. With some basic testing I couldn't find anything broken.

source/blender/makesrna/intern/rna_context.c
311

Ok, it seems we are already inconsistent in this case, it's a mix. I'm fine with having evaluated_depsgraph_get() then.

313–314

If an object becomes invisible, is the reference still valid?

I was thinking it's safer to just consider everything invalidated since the distinction is difficult to explain. But maybe we need to try anyway.

source/blender/makesrna/intern/rna_context.c
313–314

That is a good point.

So maybe all the details belongs to a wiki page like "How to get maximum performance from scripts when you know what you are doing"?

Just feel like we should share such knowledge.

source/blender/makesrna/intern/rna_context.c
313–314

Yes, keeping the details to the Python API docs seems fine.

Summary of changes:

  • Removed dependency graph argument from API functions.
  • Running object.to_mesh() on an original curve will give tessellated result but without modifiers applied.

Probably still need to adjust description of API functions.

Sergey Sharybin (sergey) updated this revision to Diff 15315.

Minor spelling fix

  • Updated against latest master
  • Update tooltip for object.to_mesh()
  • Added examples for documentation

Looks good, just comments an grammar.

doc/python_api/examples/bpy.types.Depsgraph.1.py
6–7

from original one -> from an original ID

8–9

Such access -> This

26

for -> from

27

amd -> and

29–31

dependency graph -> the dependency graph
there are changes made -> changes were made
danling -> dangling

45

has -> have

doc/python_api/examples/bpy.types.Depsgraph.2.py
19

original object -> the original object

21

has -> have an

26

This seems -> It seems

Spelling fixes.

But then me still not knowing English, why is it

All ID types have an `original` field

but

All ID types have `evaluated_get()`

Should it be "an" for the evaluated_get() as well?

LGTM, only minor points in review.

doc/python_api/examples/bpy.types.Depsgraph.4.py
18–19

Use .. note:: prefix for RST notes.

36

Convention is to use static set, {...} instead of tuple.

source/blender/blenkernel/intern/mesh_convert.c
1102

databae ?

source/blender/makesrna/intern/rna_main_api.c
950

double space.

More spelling fixes.

This revision is now accepted and ready to land.May 15 2019, 2:10 PM
This revision was automatically updated to reflect the committed changes.