Page MenuHome
Differential D8616

Depsgraph: refactor tagging after time changes
ClosedPublic
Actions

Authored by Jacques Lucke (JacquesLucke) on Aug 18 2020, 5:58 PM.

Details

Reviewers
Sergey Sharybin (sergey)
Brecht Van Lommel (brecht)
Sybren A. Stüvel (sybren)
Commits
rB263148dbacc4: Depsgraph: refactor tagging after time changes
Summary

This reverts rB1693a5efe919: Fix T66378: Missing animation update when switching view layer and implements an alternative solution.

Benefits of this solution:

  • Does not require tagging all depsgraph when the frame changed.
  • Less code.
  • Fixes a problem when trying to evaluate a depsgraph on a different frame than is stored in the original scene.

Since a depsgraph already stores the time it was last evaluated at, there is no need to tag the depsgraph after time changes. Before evaluation, one just has to compare the last evaluated frame with the frame that is about to be evaluated.

I also removed the DEG_needs_eval function, because it was never used and is incorrect now. Whether a depsgraph needs evaluation depends on the frame that you want to evaluate it on. An alternative fix would be to pass a frame into DEG_needs_eval`, but it was easier to just remove it for now.

I don't expect any user visible changes from this patch.

Diff Detail

Repository
rB Blender

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Aug 18 2020, 5:58 PM
Jacques Lucke (JacquesLucke) created this revision.
Harbormaster completed remote builds in B9617: Diff 27881.Aug 18 2020, 5:58 PM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Aug 18 2020, 5:58 PM
Jacques Lucke (JacquesLucke) added reviewers: Sergey Sharybin (sergey), Brecht Van Lommel (brecht), Sybren A. Stüvel (sybren).Aug 18 2020, 6:01 PM
Sergey Sharybin (sergey) added a comment.Aug 19 2020, 9:40 AM

Before evaluation, one just has to compare the last evaluated frame with the frame that is about to be evaluated.

This isn't reliable: one might forward one frame and then go to the original frame. If the view layer is visible, unkeyed changes will be re-evaluated using animation system. But if the view layer is not visible (unless i'm missing something) there will be no time evaluation when you switch back to that view layer. There should be no difference in the end result regardless of whether view layer was visible or not when time was modified.

Jacques Lucke (JacquesLucke) updated this revision to Diff 27897.Aug 19 2020, 10:31 AM

That makes sense.

I added back the tagging of all depsgraphs. However, instead of storing
the tag in need_update_time, I just tag the TimeSourceNode for update.

For that purpose I added DEG_time_tag_update(bmain).

If, in some files, tagging the TimeSourceNode is too inefficient (because
it iterates over all nodes directly dependent on time), it seems to make
sense to add a tagged_for_update bool to the time source node and flush
it in deg_graph_flush_updates.

Harbormaster completed remote builds in B9630: Diff 27897.Aug 19 2020, 10:31 AM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Aug 19 2020, 10:31 AM
Sergey Sharybin (sergey) added a comment.Aug 19 2020, 10:41 AM

If, in some files, tagging the TimeSourceNode is too inefficient (because it iterates over all nodes directly dependent on time), it seems to make sense to add a tagged_for_update bool to the time source node and flush it in deg_graph_flush_updates.

Tagging is supposed to be cheap, and in production files there will be a lot of things dependent on time.
Is there anything what prevents to do this tagged_for_update flag now?

I'm also not sure why you still need to compare time.

Jacques Lucke (JacquesLucke) updated this revision to Diff 27900.Aug 19 2020, 12:23 PM
  • Store update tag directly in TimeSourceNode.
  • Defer tagging time dependent nodes until depsgraph is about to be evaluated.
  • Remove time comparison.
Harbormaster completed remote builds in B9632: Diff 27900.Aug 19 2020, 12:24 PM
Sergey Sharybin (sergey) added inline comments.Aug 19 2020, 12:36 PM
source/blender/depsgraph/intern/depsgraph_eval.cc
49–51

Comments need an update it seems.

Jacques Lucke (JacquesLucke) updated this revision to Diff 27904.Aug 19 2020, 12:43 PM
  • fix comment
Harbormaster completed remote builds in B9635: Diff 27904.Aug 19 2020, 12:44 PM
Sergey Sharybin (sergey) accepted this revision.Aug 19 2020, 4:44 PM

From reading seems fine.
But suggest to wait for a second pair of eyes to verify.

This revision is now accepted and ready to land.Aug 19 2020, 4:44 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Aug 20 2020, 1:07 PM

I think this is a much cleaner way of handling tagging for time updates, nice work.

source/blender/depsgraph/intern/depsgraph_eval.cc
50

"in the given frame" is a bit weird, because then a frame is seen as something that has a duration (or something can't be "in" it) rather than a point in time. As it is now, I read it as evaluating the entire interval [frame, frame+1).

51

I don't understand the 2nd sentence. Why would the caller be responsible for this, if this function can also figure out whether tagging is necessary or not?

The function seems to do two things:

  • update the depsgraph time, and
  • evaluate the depsgraph for the new time.

Probably deg_set_time_and_evaluate would be a better name for the function.

62

The naming seems to be a bit weird:

  • DEG_evaluate_on_refresh() does not call deg::deg_evaluate_on_refresh().
  • It does call deg_evaluate(), which removes the "on refresh" part of the thing.
  • DEG_evaluate_on_framechange() also calls deg_evaluate(), but
  • deg_evaluate() does call deg::deg_evaluate_on_refresh()

So the "on refresh" label blinks in and out of existence, without a clear indication as to why.

Also DEG_evaluate_on_refresh() updates the depsgraph time from the scene time, which to me makes sense in a "the frame changed" scenario. But, for that there is a different function DEG_evaluate_on_framechange() function, which actually only sets the time, regardless of what it is in the current scene. Much confusion.

Maybe not for this particular patch to solve, but confusing nonetheless.

source/blender/depsgraph/intern/depsgraph_tag.cc
775–776

Why does this code need to know that a timesource needs tagging with deg::DEG_UPDATE_SOURCE_TIME? Shouldn't this be a function Depsgraph::tag_time_source() instead? See the Law of Demeter.

source/blender/depsgraph/intern/eval/deg_eval_flush.cc
90

Which nodes?

91–100

This is only accessing properties of the TimeSourceNode, which is an indication that it actually should be a method of that class.

This revision now requires changes to proceed.Aug 20 2020, 1:07 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 27974.Aug 20 2020, 3:15 PM
  • Reorganize patch based on feedback.
Harbormaster completed remote builds in B9688: Diff 27974.Aug 20 2020, 3:16 PM
Sybren A. Stüvel (sybren) accepted this revision.Aug 20 2020, 4:19 PM
This revision is now accepted and ready to land.Aug 20 2020, 4:19 PM
Closed by commit rB263148dbacc4: Depsgraph: refactor tagging after time changes (authored by Jacques Lucke (JacquesLucke)). · Explain WhyAug 20 2020, 4:33 PM
This revision was automatically updated to reflect the committed changes.
Jacques Lucke (JacquesLucke) added a commit: rB263148dbacc4: Depsgraph: refactor tagging after time changes.

Revision Contents
Changeset List

PathSize
source/
blender/
depsgraph/
8 lines
intern/
5 lines
6 lines
37 lines
29 lines
eval/
11 lines
node/
4 lines
10 lines
editors/
screen/
2 lines
makesdna/
4 lines

Diff 27980

View Options

source/blender/depsgraph/DEG_depsgraph.h

Loading...
View Options

source/blender/depsgraph/intern/depsgraph.h

Loading...
View Options

source/blender/depsgraph/intern/depsgraph.cc

Loading...
View Options

source/blender/depsgraph/intern/depsgraph_eval.cc

Loading...
View Options

source/blender/depsgraph/intern/depsgraph_tag.cc

Loading...
View Options

source/blender/depsgraph/intern/eval/deg_eval_flush.cc

Loading...
View Options

source/blender/depsgraph/intern/node/deg_node_time.h

Loading...
View Options

source/blender/depsgraph/intern/node/deg_node_time.cc

Loading...
View Options

source/blender/editors/screen/screen_edit.c

Loading...
View Options

source/blender/makesdna/DNA_ID.h

Loading...