Page MenuHome

Fix T66325: Animation Keyframe Undo/Redo Bug
ClosedPublic

Authored by Sergey Sharybin (sergey) on Jul 22 2019, 5:16 PM.

Details

Summary

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

Diff Detail

Repository
rB Blender

Event Timeline

Don't have much to add here.
Seems generally fine and fixes the issue as intended.

Some notes:

  • Undo seems to work properly_ & doesn't break any undo tests.
  • Tests should be added to lib/tests/ui_simulate that validate these changes (just need to check rotation values of keyed channels are updated correctly).
  • Storing the undo state after evaluation will have some performance (from P1044).

    Although I don't think it can be done in a very elegant way. Would be interesting to for undoing animation on heavy scenes to see if it's giving any user-visible gains.
source/blender/depsgraph/intern/depsgraph_tag.cc
450

Why not make this a static function?

This revision is now accepted and ready to land.Jul 23 2019, 3:43 AM
source/blender/depsgraph/intern/depsgraph_tag.cc
450

It's in the anonymous namespace, which is effectively static function.

@Brecht Van Lommel (brecht), undo system is something where all the extra eyes are welcome ;) Do you think you'll have time soon-ish, or shall i just go ahead and commit this?

Brecht Van Lommel (brecht) requested changes to this revision.Thu, Jul 25, 5:55 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenloader/intern/readfile.c
9141–9145

Only do this for undo/redo then, and not general file reading.

Recalc flags are not compatible across versions, and for properly understanding refresh issues and debugging them it seems best not to have leftovers from a previous Blender session.

source/blender/depsgraph/intern/builder/deg_builder.cc
227

then were -> then were

source/blender/depsgraph/intern/depsgraph_tag.cc
458–459

This is not going into 2.80 I guess? So not sure why we need to be overly paranoid.

This revision now requires changes to proceed.Thu, Jul 25, 5:55 PM
source/blender/blenloader/intern/readfile.c
9141–9145

if (fd->memfile) { .. } tests for the undo case.

Addressed feedback from Brecht.

This revision is now accepted and ready to land.Fri, Jul 26, 2:15 PM