Details

Reviewers

Jacques Lucke (JacquesLucke)
Sybren A. Stüvel (sybren)
Sergey Sharybin (sergey)

Maniphest Tasks

T66913: Issue with Undoing after using next/previous keyframe shortcut.

Summary

Use the ID.recalc flag to detect when updates after frame-change is needed.
Since comparing the last calculated frame doesn't take undo into account (see code-comment for details).

ID_RECALC_AUDIO_SEEK has been renamed to ID_RECALC_FRAME_CHANGE since this is not only related to audio however internally this flag is still categorized in NodeType::AUDIO.

This fixes T66913 & T95246.

Note that this could go via the more conventional checks for ID.recalc in the depsgraph,
making it closer to ID_RECALC_TIME which was removed in rB263148dbacc4.

Diff Detail

Repository

rB Blender

Branch

TEMP-DEPSGRAPH-FRAME-CHANGE (branched from master)

Build Status

Buildable 20313
Build 20313: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.Jan 28 2022, 5:22 AM

Campbell Barton (campbellbarton) created this revision.

Harbormaster completed remote builds in B20206: Diff 47679.Jan 28 2022, 5:22 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Jan 28 2022, 5:34 AM

Campbell Barton (campbellbarton) added reviewers: Jacques Lucke (JacquesLucke), Sybren A. Stüvel (sybren), Sergey Sharybin (sergey).

Campbell Barton (campbellbarton) added projects: Restricted Project, Dependency Graph.

Campbell Barton (campbellbarton) added a task: T66913: Issue with Undoing after using next/previous keyframe shortcut..

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Improve comment wording.

Harbormaster completed remote builds in B20207: Diff 47680.Jan 28 2022, 5:40 AM

From what I can tell, the change seems reasonable. I just find it a bit weird to use ID_RECALC_AUDIO_SEEK. That could be quite confusing when debugging related issues. The other way around might make more sense if we really want to use the same flag for both.

Checking recalc flags prior time tagging seems a right solution to me.

The way how the new recalc flag is introduced I find hard to understand semantically.
The recalc flags are supposed to correspond to which part of ID did change. Exposing ID_RECALC_FRAME_CHANGE which is the same as ID_RECALC_AUDIO_SEEK is not clear to me. If you want to go a pseudonym route move it down under the Aggregate flags, use only for checks on runtime. section and rename to ID_RECALC_TIME_ALL. Alternatively rename the ID_RECALC_AUDIO_SEEK to ID_RECALC_FRAME_CHANGE. The latter one might be more clear for understanding.

This revision now requires changes to proceed.Jan 31 2022, 12:58 PM

Rename ID_RECALC_AUDIO_SEEK to ID_RECALC_FRAME_CHANGE
Move explanation for checking the flag into DEG_evaluate_on_refresh.

Harbormaster completed remote builds in B20311: Diff 47888.Feb 3 2022, 12:28 AM

Missed rename of string identifier when renaming AUDIO_SEEK -> FRAME_CHANGE

Harbormaster completed remote builds in B20312: Diff 47889.Feb 3 2022, 12:30 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Feb 3 2022, 12:37 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Remove unnecessary parens.

Harbormaster completed remote builds in B20313: Diff 47890.Feb 3 2022, 12:39 AM

From code side seems good.
Assuming you've tested undo of unkeyed animation changes and it works ;)

This revision is now accepted and ready to land.Feb 3 2022, 9:21 AM

Campbell Barton (campbellbarton) mentioned this in rB35aedd87e78d: Fix T66913: undo after frame-change doesn't refresh properly.Feb 3 2022, 11:39 AM

Campbell Barton (campbellbarton) closed this revision.Feb 3 2022, 12:16 PM

Fix T66913: undo after frame-change doesn't refresh properly
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 47890

source/blender/blenkernel/intern/scene.c

source/blender/depsgraph/intern/depsgraph_eval.cc

source/blender/depsgraph/intern/depsgraph_tag.cc

source/blender/editors/animation/anim_ops.c

source/blender/editors/screen/screen_edit.c

source/blender/editors/screen/screen_ops.c

source/blender/editors/space_clip/clip_ops.c

source/blender/editors/space_clip/tracking_ops.c

source/blender/editors/space_graph/graph_ops.c

source/blender/editors/space_image/image_ops.c

source/blender/editors/space_sequencer/sequencer_edit.c

source/blender/makesdna/DNA_ID.h

source/blender/makesrna/intern/rna_scene.c

source/blender/windowmanager/intern/wm_event_system.c

Fix T66913: undo after frame-change doesn't refresh properlyClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 47890

source/blender/blenkernel/intern/scene.c

source/blender/depsgraph/intern/depsgraph_eval.cc

source/blender/depsgraph/intern/depsgraph_tag.cc

source/blender/editors/animation/anim_ops.c

source/blender/editors/screen/screen_edit.c

source/blender/editors/screen/screen_ops.c

source/blender/editors/space_clip/clip_ops.c

source/blender/editors/space_clip/tracking_ops.c

source/blender/editors/space_graph/graph_ops.c

source/blender/editors/space_image/image_ops.c

source/blender/editors/space_sequencer/sequencer_edit.c

source/blender/makesdna/DNA_ID.h

source/blender/makesrna/intern/rna_scene.c

source/blender/windowmanager/intern/wm_event_system.c

Fix T66913: undo after frame-change doesn't refresh properly
ClosedPublic
Actions

Revision Contents
Changeset List