Fix T66325: Animation Keyframe Undo/Redo Bug

The issue was caused by dependency graph always ignoring animation
update when it is first time constructed. This was a way to make it
preserve unkeyed changes on undo/redo. This, however, made it so
changes of animation data itself (such as deleting/moving keyframes)
did not trigger animation update by the dependency graph.

This worked prior to copy-on-write because animation recalc flags
were stored in the DNA and never re-set on file/undo load. This was
giving dependency graph a clue that animation is to be re-evaluated
when operator explicitly asked to (more precisely, when such operator
was undone/redone).

This change makes it so original ID's recalc flags are storing
recalc flags when ID is tagged for update as an response to user
input. This way re-building dependency graph can force animation
to be updated on redo.

Tricky part here is that ID's recalc flag is no longer to be zeroed
when loading undo step (which is the same as reading .blend file).
This is something what works differently comparing to legacy
dependency graph, which was zeroing object's recalc flags there but
not animation data's recalc flags.
Shouldn't be causing issues, since unkeyed changes are not preserved
upon opening a file anyway, at least to my knowledge.

Related reports which are to be taken into account and verified
they are not re-introduced when making changes in the area:

- T63111: Auto-Bake stuck at constant re-rendering
- T54296: Cycles viewport render stuck on constant re-render

Reviewers: campbellbarton, brecht

Reviewed By: campbellbarton, brecht

Maniphest Tasks: T66325

Differential Revision: https://developer.blender.org/D5316

3 files changed, 59 insertions, 9 deletions

diff --git a/source/blender/blenloader/intern/readfile.c b/source/blender/blenloader/intern/readfile.c
index 44039ad59ee..acca42b6562 100644
--- a/source/blender/blenloader/intern/readfile.c
+++ b/source/blender/blenloader/intern/readfile.c
@@ -9137,7 +9137,18 @@ static BHead *read_libblock(FileData *fd, Main *main, BHead *bhead, const int ta
   id->icon_id = 0;
   id->newid = NULL; /* Needed because .blend may have been saved with crap value here... */
   id->orig_id = NULL;
-  id->recalc = 0;
+
+  /* NOTE: It is important to not clear the recalc flags for undo/redo.
+   * Preserving recalc flags on redo/undo is the only way to make dependency graph detect
+   * that animation is to be evaluated on undo/redo. If this is not enforced by the recalc
+   * flags dependency graph does not do animation update to avoid loss of unkeyed changes.,
+   * which conflicts with undo/redo of changes to animation data itself.
+   *
+   * But for regular file load we clear the flag, since the flags might have been changed sinde
+   * the version the file has been saved with. */
+  if (!fd->memfile) {
+    id->recalc = 0;
+  }
 
   /* this case cannot be direct_linked: it's just the ID part */
   if (bhead->code == ID_LINK_PLACEHOLDER) {
diff --git a/source/blender/depsgraph/intern/builder/deg_builder.cc b/source/blender/depsgraph/intern/builder/deg_builder.cc
index 36c6d246097..4cbdd169980 100644
--- a/source/blender/depsgraph/intern/builder/deg_builder.cc
+++ b/source/blender/depsgraph/intern/builder/deg_builder.cc
@@ -204,7 +204,7 @@ void deg_graph_build_finalize(Main *bmain, Depsgraph *graph)
   /* Re-tag IDs for update if it was tagged before the relations
    * update tag. */
   for (IDNode *id_node : graph->id_nodes) {
-    ID *id = id_node->id_orig;
+    ID *id_orig = id_node->id_orig;
     id_node->finalize_build(graph);
     int flag = 0;
     /* Tag rebuild if special evaluation flags changed. */
@@ -219,10 +219,13 @@ void deg_graph_build_finalize(Main *bmain, Depsgraph *graph)
       flag |= ID_RECALC_COPY_ON_WRITE;
       /* This means ID is being added to the dependency graph first
        * time, which is similar to "ob-visible-change" */
-      if (GS(id->name) == ID_OB) {
+      if (GS(id_orig->name) == ID_OB) {
         flag |= ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY;
       }
     }
+    /* Restore recalc flags from original ID, which could possibly contain recalc flags set by
+     * an operator and then were carried on by the undo system. */
+    flag |= id_orig->recalc;
     if (flag != 0) {
       graph_id_tag_update(bmain, graph, id_node->id_orig, flag, DEG_UPDATE_SOURCE_RELATIONS);
     }
diff --git a/source/blender/depsgraph/intern/depsgraph_tag.cc b/source/blender/depsgraph/intern/depsgraph_tag.cc
index f9105b53536..729a0929dd5 100644
--- a/source/blender/depsgraph/intern/depsgraph_tag.cc
+++ b/source/blender/depsgraph/intern/depsgraph_tag.cc
@@ -447,6 +447,24 @@ const char *update_source_as_string(eUpdateSource source)
   return "UNKNOWN";
 }
 
+int deg_recalc_flags_for_legacy_zero()
+{
+  return ID_RECALC_ALL & ~(ID_RECALC_PSYS_ALL | ID_RECALC_ANIMATION);
+}
+
+int deg_recalc_flags_effective(Depsgraph *graph, int flags)
+{
+  if (graph != NULL) {
+    if (!graph->is_active) {
+      return 0;
+    }
+  }
+  if (flags == 0) {
+    return deg_recalc_flags_for_legacy_zero();
+  }
+  return flags;
+}
+
 /* Special tag function which tags all components which needs to be tagged
  * for update flag=0.
  *
@@ -462,7 +480,7 @@ void deg_graph_node_tag_zero(Main *bmain,
   }
   ID *id = id_node->id_orig;
   /* TODO(sergey): Which recalc flags to set here? */
-  id_node->id_cow->recalc |= ID_RECALC_ALL & ~(ID_RECALC_PSYS_ALL | ID_RECALC_ANIMATION);
+  id_node->id_cow->recalc |= deg_recalc_flags_for_legacy_zero();
   GHASH_FOREACH_BEGIN (ComponentNode *, comp_node, id_node->components) {
     if (comp_node->type == NodeType::ANIMATION) {
       continue;
@@ -607,6 +625,16 @@ void graph_id_tag_update(
   if (id_node != NULL) {
     id_node->id_cow->recalc |= flag;
   }
+  /* When ID is tagged for update based on an user edits store the recalc flags in the original ID.
+   * This way IDs in the undo steps will have this flag preserved, making it possible to restore
+   * all needed tags when new dependency graph is created on redo.
+   * This is the only way to ensure modifications to animation data (such as keyframes i.e.)
+   * properly triggers animation update for the newely constructed dependency graph on redo (while
+   * usually newly created dependency graph skips animation update to avoid loss of unkeyed
+   * changes). */
+  if (update_source == DEG_UPDATE_SOURCE_USER_EDIT) {
+    id->recalc |= deg_recalc_flags_effective(graph, flag);
+  }
   int current_flag = flag;
   while (current_flag != 0) {
     IDRecalcFlag tag = (IDRecalcFlag)(1 << bitscan_forward_clear_i(&current_flag));
@@ -779,6 +807,16 @@ void DEG_ids_check_recalc(
   DEG::deg_editors_scene_update(&update_ctx, updated);
 }
 
+static void deg_graph_clear_id_recalc_flags(ID *id)
+{
+  id->recalc &= ~ID_RECALC_ALL;
+  bNodeTree *ntree = ntreeFromID(id);
+  /* Clear embedded node trees too. */
+  if (ntree) {
+    ntree->id.recalc &= ~ID_RECALC_ALL;
+  }
+}
+
 static void deg_graph_clear_id_node_func(void *__restrict data_v,
                                          const int i,
                                          const ParallelRangeTLS *__restrict /*tls*/)
@@ -790,12 +828,10 @@ static void deg_graph_clear_id_node_func(void *__restrict data_v,
   DEG::IDNode *id_node = deg_graph->id_nodes[i];
 
   id_node->is_user_modified = false;
-  id_node->id_cow->recalc &= ~ID_RECALC_ALL;
 
-  /* Clear embedded node trees too. */
-  bNodeTree *ntree_cow = ntreeFromID(id_node->id_cow);
-  if (ntree_cow) {
-    ntree_cow->id.recalc &= ~ID_RECALC_ALL;
+  deg_graph_clear_id_recalc_flags(id_node->id_cow);
+  if (deg_graph->is_active) {
+    deg_graph_clear_id_recalc_flags(id_node->id_orig);
   }
 }