[Bf-committers] Dependency graph filtering

Sergey Sharybin sergey.vfx at gmail.com
Thu Aug 23 11:04:33 CEST 2018


Hi,

While it's cool and everything to have faster motion paths, the
implementation way i find completely wrong, with big intrinsic design
flaws. Here goes just a brief overview of things which bothers me most.

- Committing without any code review?

Surely, all this filtering API is coming from an original author of the new
dependency graph. But that "new" dependency graph was re-written quite a
lot by both Lukas and myself. And it is me who is spending at least 50% of
time maintaining the dependency graph.

I do not find it cool at all committing things without even talking to me,
without discussing ways to achieve particular goal, and without asking even
whether i am interested in having a review on the code.

- Why is it even a thing for "filtering" ?

The goal is: have a dependency graph which is enough to evaluate datablocks
which you're interested in at some specific task.

Most reliably and shortest way to do so, is to invoke dependency graph
builder starting from that datablock. Nowadays it's almost as simple as
calling `build_id()` for node and relations builder. Surely, you'll need to
take collections and overrides into account, but that is also just few
extra calls. This way you:

a) Do not need to implement all those weird and wonderful traversals
b) Make it possible to create such a dependency graph from a thread, or
create multiple dependency graphs at the same time (the foreach API
explicitly says it is not safe from threading point of view). Just to state
the obvious: we do need to support building dependency graphs (including
their "minimal" versions) from multiple threads.
c) You guarantee that dependency graph is fully consistent, and fully
matches situation when one manually simplifies scene to the same state.

Next big issue with filtering is that it requires original dependency graph
to have up-to-date relations. This means, before you can filter, you have
to ensure relations are up to date. Filtering API doesn't even do any
sanity checks in this regard.
I see an argument "motion paths do have dependency graph up to date
already". But then why is this filtering in depsgraph/ ? Anything what goes
there must be generic and reusable and reliable. Current implementation is
none of that.

And last but not least, all the comments about per-operation or
per-component filtering are not compatible with the current expectations of
what dependency graph is supposed to deliver: you are not supposed to have
partially evaluated datablocks after calling DEG_evaluate_on family of
functions. None of the areas should even care whether dependency graph is
"filtered" or not, there must be no cases where you can't access evaluated
object properties based on the nature of dependency graph.

- What is `DAG_EVAL_BACKGROUND` ?

While i see that potentially some of the areas might want to do something
special for such kind of evaluation, but currently this breaks
visibility/simplification settings checks: those are expecting either
viewport or render evaluation. If new evaluation modes are added, all users
of mode are to be checked. You can't just add new evaluation modes and
expect all the areas properly pick it up.

- Keep areas unaware of where dependency graph came from.

This is very red-flagging when comment says things like "don't pass
filtered depsgraph here". I do not see any reason why filtering will not be
able to narrow dependency down even further. If it's unable to do so, this
means dependency graph it produces is wrong.

- Naming.

Stop calling everything a `target`. This is ambiguous word, which is in
most cases it is obviously inverted. Don't modify naming in the existing
code you didn't write, especially if the author of that code is still
around and does close to 100% maintenance in the area.

Stop changing ownership of a variables. This is very confusing when up to
some point `depsgraph` is owned by a scene, but after calling `filter()` it
is owned by the function itself (which forces you to have all the obscure
things like `free_depsgraph` (which should really at least prefixed
`need_`)).

- Dead code.

Never commit it. It adds a lot of confusion, and is almost never gets
pushed forward. Things like `detail_level` never gets implemented or used
for years, and during those years they only cause confusion about what
should one pass there.

- Moving forward.

I would really strongly advice removing this filtering API. This is really
a dead end. Implement BUILDING of dependency graph from a datablock. If you
want to be more generic, allow building dependency graph from a list of
datablocks.

-- 
With best regards, Sergey Sharybin


More information about the Bf-committers mailing list