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 _______________________________________________ Bf-committers mailing list Bf-committers@blender.org https://lists.blender.org/mailman/listinfo/bf-committers