[Bf-codereview] Threaded object update and EvaluationContext (issue 15340044)

[brechtvanlommel at gmail.com]

Some initial comments from first pass reading code, still need to dive
into scene.c, depsgraph.c and convertblender.c.


https://codereview.appspot.com/15340044/diff/1/b/source/blender/blenkernel/BKE_depsgraph.h
File b/source/blender/blenkernel/BKE_depsgraph.h (right):

https://codereview.appspot.com/15340044/diff/1/b/source/blender/blenkernel/BKE_depsgraph.h#newcode121
b/source/blender/blenkernel/BKE_depsgraph.h:121: +void
DAG_threaded_init(void);
Maybe leave out the "threaded" part of these function names?

https://codereview.appspot.com/15340044/diff/1/b/source/blender/blenkernel/BKE_scene.h
File b/source/blender/blenkernel/BKE_scene.h (right):

https://codereview.appspot.com/15340044/diff/1/b/source/blender/blenkernel/BKE_scene.h#newcode117
b/source/blender/blenkernel/BKE_scene.h:117: +/* TODO(sergey): Find a
better place for this. */
The dependency graph header file perhaps?

https://codereview.appspot.com/15340044/diff/1/b/source/blender/blenkernel/BKE_scene.h#newcode123
b/source/blender/blenkernel/BKE_scene.h:123: +void
BKE_scene_update_tagged_ex(struct EvaluationContext *evaluation_context,
struct Main *bmain, struct Scene *scene, bool use_threads);
When do you not want this to be threaded? Are there cases where this has
thread safety issues, or is it more done because you haven't tested
using threaded updates everywhere?

https://codereview.appspot.com/15340044/diff/1/b/source/blender/blenkernel/depsgraph_private.h
File b/source/blender/blenkernel/depsgraph_private.h (right):

https://codereview.appspot.com/15340044/diff/1/b/source/blender/blenkernel/depsgraph_private.h#newcode97
b/source/blender/blenkernel/depsgraph_private.h:97: +	uint32_t valency;
/* valency of the node is a number of parents which are not updated yet
This could be named better I think, valency is a pretty generic name and
doesn't really say anything about things being updated. Maybe
"num_pending_parents".

https://codereview.appspot.com/15340044/diff/1/b/source/blender/blenkernel/intern/constraint.c
File b/source/blender/blenkernel/intern/constraint.c (right):

https://codereview.appspot.com/15340044/diff/1/b/source/blender/blenkernel/intern/constraint.c#newcode3355
b/source/blender/blenkernel/intern/constraint.c:3355: +		 *
  render flag for cnstraints. before this use viewport dm as it
typo: cnstraints (same further in the file)

https://codereview.appspot.com/15340044/diff/1/b/source/blender/editors/render/render_internal.c
File b/source/blender/editors/render/render_internal.c (right):

https://codereview.appspot.com/15340044/diff/1/b/source/blender/editors/render/render_internal.c#newcode52
b/source/blender/editors/render/render_internal.c:52: +#include
"BKE_curve.h"
Adding these includes seems kind of unnecessary for just a whitespace
change? :)

https://codereview.appspot.com/15340044/diff/1/b/source/blender/editors/screen/screen_edit.c
File b/source/blender/editors/screen/screen_edit.c (right):

https://codereview.appspot.com/15340044/diff/1/b/source/blender/editors/screen/screen_edit.c#newcode1441
b/source/blender/editors/screen/screen_edit.c:1441:
+		BKE_scene_set_background(bmain, sc->scene);
It makes sense to do this update here, I would say the main todo in this
case is that it would be better if we could detect if anything actually
changes that requires a depsgraph rebuild, but probably not so important
to solve now.

https://codereview.appspot.com/15340044/diff/1/b/source/blender/makesrna/intern/rna_scene_api.c
File b/source/blender/makesrna/intern/rna_scene_api.c (right):

https://codereview.appspot.com/15340044/diff/1/b/source/blender/makesrna/intern/rna_scene_api.c#newcode60
b/source/blender/makesrna/intern/rna_scene_api.c:60: +	/* TODO(sergey):
Use context from G.main? */
This one is a bit complicated, for cycles (and other external render
engines) motion blur we need to be able to do this update for render,
and not just for the viewport. Some possible solutions, one is to
provide an option to do this for render, or we could add a function to
RenderEngine that should be used instead of this one and uses the
evaluation context owned by the render, or we expose the concept of an
evaluation context to the API somehow.

I think we aren't quite ready yet to expose this to the API too much, so
perhaps this needs to be postponed a bit until more of the depsgraph
work is getting implemented and not part of this patch.

https://codereview.appspot.com/15340044/