Non-animated property, stuck in previously keyframed values #62012

New Issue

Dalai Felinto · 2019-02-27T13:09:40+01:00

Dalai Felinto commented

2019-02-27 13:09:40 +01:00

Blender Version
Broken: 5984d541b45 (blender2.8 HEAD)
Worked: Not sure.

Short description of error
Object with animation data keeps going back to previously existing keyframed values for channels with no animation on it.

Exact steps for others to reproduce the error

Keyframe the initial cube LocScale (I > LocScale).
Mouse hover scale and "Delete Keyframes".
Change Scale X: to 4.
Playback.

Scale is set back to the previously keyframed value (1.0).

**Blender Version** Broken: 5984d541b45 (blender2.8 HEAD) Worked: Not sure. **Short description of error** Object with animation data keeps going back to previously existing keyframed values for channels with no animation on it. **Exact steps for others to reproduce the error** 1) Keyframe the initial cube LocScale (I > LocScale). 2) Mouse hover scale and "Delete Keyframes". 3) Change Scale X: to 4. 4) Playback. Scale is set back to the previously keyframed value (1.0).

Dalai Felinto commented

2019-02-27 13:09:40 +01:00

Added subscriber: @dfelinto

Sergey Sharybin was assigned by Dalai Felinto

2019-02-27 13:10:40 +01:00

Dalai Felinto commented

2019-02-27 13:10:40 +01:00

Added subscriber: @Sergey

Dalai Felinto commented

2019-02-27 13:10:40 +01:00

@Sergey a blind guess here, but I suspect this may have to do with the hacks in depsgraph to handle un-keyframed values.

Sergey Sharybin commented

2019-02-27 14:11:46 +01:00

@dfelinto ./blender --debug-depsgraph-tag, see that no tags are done when you delete keyframe.
This is nothing to do with "hacks", depsgraph has nothing to do with that. In fact, those hacks you're referring to are gone for months.

The actual issue is the fiasco of the optimization idea which was trying to deal with avoiding too many relations update when inserting keyframes. The correct logic should be:

Initial insertion of keyframe should update relation, since that is what introduces ANIMATION component of the cube.
Adding/removing keyframes should tag action for recalc. (ID_RECALC_ANIMATION should work. If not, ID_RECALC_COPY_ON_WRITE but with some TODO around id). This is what always should be happening, otherwise copied object will not be aware of changes in the interface.
Adding/removing drivers should be tagging ID itself, since it is ID who has its drivers (and not an action). Additionally, editing drivers should tag relations for update. Those are changing in 98% of edits.

Anyone should be able to fix such issues. It is just wrong if it's me who is the only one who fixes the issues all over the blender.
Otherwise you'd have to wait for until i'm done with another report which is triaged HIGH and is way more tricky.

@dfelinto `./blender --debug-depsgraph-tag`, see that no tags are done when you delete keyframe. This is nothing to do with "hacks", depsgraph has nothing to do with that. In fact, those hacks you're referring to are gone for months. The actual issue is the fiasco of the optimization idea which was trying to deal with avoiding too many relations update when inserting keyframes. The correct logic should be: - Initial insertion of keyframe should update relation, since that is what introduces ANIMATION component of the cube. - Adding/removing keyframes should tag action for recalc. (`ID_RECALC_ANIMATION` should work. If not, `ID_RECALC_COPY_ON_WRITE` but with some TODO around id). This is what always should be happening, otherwise copied object will not be aware of changes in the interface. - Adding/removing drivers should be tagging ID itself, since it is ID who has its drivers (and not an action). Additionally, editing drivers should tag relations for update. Those are changing in 98% of edits. Anyone should be able to fix such issues. It is just wrong if it's me who is the only one who fixes the issues all over the blender. Otherwise you'd have to wait for until i'm done with another report which is triaged HIGH and is way more tricky.

Sergey Sharybin was unassigned by Dalai Felinto

2019-02-27 14:24:07 +01:00

Dalai Felinto self-assigned this 2019-02-27 14:24:07 +01:00

Dalai Felinto commented

2019-02-27 14:24:07 +01:00

I was reporting this while using Blender, so didn't went into debugging steps.

Anyone should be able to fix such issues. It is just wrong if it's me who is the only one who fixes the issues all over the blender.

You are right. I should probably have left the report as medium or normal but unassigned until had the time to see how who this issue belong to.

Anyone should be able to fix such issues. It is just wrong if it's me who is the only one who fixes the issues all over the blender.

Right again, and I will look at it later this week then. Either way it is tagged as high, so other devs are always welcome to pick it before I get the time to do it myself.

I was reporting this while using Blender, so didn't went into debugging steps. > Anyone should be able to fix such issues. It is just wrong if it's me who is the only one who fixes the issues all over the blender. You are right. I should probably have left the report as medium or normal but unassigned until had the time to see how who this issue belong to. > Anyone should be able to fix such issues. It is just wrong if it's me who is the only one who fixes the issues all over the blender. Right again, and I will look at it later this week then. Either way it is tagged as high, so other devs are always welcome to pick it before I get the time to do it myself.

Sergey Sharybin commented

2019-02-27 14:35:01 +01:00

I was reporting this while using Blender, so didn't went into debugging steps.

Well, i did that for you ;) Didn't take much time. So no worries here. Finding a place where it is to be corrected in keyframing.c (i would imagine) will take more time :/

> I was reporting this while using Blender, so didn't went into debugging steps. Well, i did that for you ;) Didn't take much time. So no worries here. Finding a place where it is to be corrected in `keyframing.c` (i would imagine) will take more time :/

Brecht Van Lommel commented

2019-02-27 20:11:54 +01:00

Added subscriber: @brecht

Brecht Van Lommel commented

2019-02-27 20:11:54 +01:00

#60086 and #61169 are possible duplicates.

Joshua Leung commented

2019-02-28 12:38:54 +01:00

Added subscriber: @JoshuaLeung

Joshua Leung commented

2019-02-28 12:38:54 +01:00

Yes, the ones in #60086 are pretty much the same issue (incorrect update tagging). IIRC, the logic for deciding when to tag is a bit borked right now - COW tagging only happens along with relations rebuild/update, and only when F-Curves get added/deleted, or something like that.

#61169 is something different (i.e. when multiple values are edited, but only one property gets keyframed, all other modified but unkeyed lose their unkeyed values)

Yes, the ones in #60086 are pretty much the same issue (incorrect update tagging). IIRC, the logic for deciding when to tag is a bit borked right now - COW tagging only happens along with relations rebuild/update, and only when F-Curves get added/deleted, or something like that. #61169 is something different (i.e. when multiple values are edited, but only one property gets keyframed, all other modified but unkeyed lose their unkeyed values)

Sergey Sharybin commented

2019-02-28 17:25:29 +01:00

Here is what is appears to be a correct behavior to me P919: Fix #62012

diff --git a/source/blender/editors/animation/keyframing.c b/source/blender/editors/animation/keyframing.c
index 5e3880e2253..1922b876d1e 100644
--- a/source/blender/editors/animation/keyframing.c
+++ b/source/blender/editors/animation/keyframing.c
@@ -1365,6 +1365,20 @@ static bool delete_keyframe_fcurve(AnimData *adt, FCurve *fcu, float cfra)
 	return false;
 }
 
+static void deg_tag_after_keyframe_delete(Main *bmain, ID *id, AnimData *adt)
+{
+	if (adt->action == NULL) {
+		/* In the case last f-curve wes removed need to inform dependency graph
+		 * about relations update, since it needs to get rid of animation operation
+		 * for this datablock. */
+		DEG_id_tag_update_ex(bmain, id, ID_RECALC_COPY_ON_WRITE);
+		DEG_relations_tag_update(bmain);
+	}
+	else {
+		DEG_id_tag_update_ex(bmain, &adt->action->id, ID_RECALC_COPY_ON_WRITE);
+	}
+}
+
 short delete_keyframe(Main *bmain, ReportList *reports, ID *id, bAction *act,
                       const char group- [ ], const char rna_path- [ ], int array_index, float cfra,
                       eInsertKeyFlags UNUSED(flag))
@@ -1441,12 +1455,8 @@ short delete_keyframe(Main *bmain, ReportList *reports, ID *id, bAction *act,
 		ret += delete_keyframe_fcurve(adt, fcu, cfra);
 
 	}
-	/* In the case last f-curve wes removed need to inform dependency graph
-	 * about relations update, since it needs to get rid of animation operation
-	 * for this datablock. */
-	if (ret && adt->action == NULL) {
-		DEG_id_tag_update_ex(bmain, id, ID_RECALC_COPY_ON_WRITE);
-		DEG_relations_tag_update(bmain);
+	if (ret) {
+		deg_tag_after_keyframe_delete(bmain, id, adt);
 	}
 	/* return success/failure */
 	return ret;
@@ -1537,12 +1547,8 @@ static short clear_keyframe(Main *bmain, ReportList *reports, ID *id, bAction *a
 		/* return success */
 		ret++;
 	}
-	/* In the case last f-curve wes removed need to inform dependency graph
-	 * about relations update, since it needs to get rid of animation operation
-	 * for this datablock. */
-	if (ret && adt->action == NULL) {
-		DEG_id_tag_update_ex(bmain, id, ID_RECALC_COPY_ON_WRITE);
-		DEG_relations_tag_update(bmain);
+	if (ret) {
+		deg_tag_after_keyframe_delete(bmain, id, adt);
 	}
 	/* return success/failure */
 	return ret;

Here is what is appears to be a correct behavior to me [P919: Fix #62012](https://archive.blender.org/developer/P919.txt) ``` diff --git a/source/blender/editors/animation/keyframing.c b/source/blender/editors/animation/keyframing.c index 5e3880e2253..1922b876d1e 100644 --- a/source/blender/editors/animation/keyframing.c +++ b/source/blender/editors/animation/keyframing.c @@ -1365,6 +1365,20 @@ static bool delete_keyframe_fcurve(AnimData *adt, FCurve *fcu, float cfra) return false; } +static void deg_tag_after_keyframe_delete(Main *bmain, ID *id, AnimData *adt) +{ + if (adt->action == NULL) { + /* In the case last f-curve wes removed need to inform dependency graph + * about relations update, since it needs to get rid of animation operation + * for this datablock. */ + DEG_id_tag_update_ex(bmain, id, ID_RECALC_COPY_ON_WRITE); + DEG_relations_tag_update(bmain); + } + else { + DEG_id_tag_update_ex(bmain, &adt->action->id, ID_RECALC_COPY_ON_WRITE); + } +} + short delete_keyframe(Main *bmain, ReportList *reports, ID *id, bAction *act, const char group- [ ], const char rna_path- [ ], int array_index, float cfra, eInsertKeyFlags UNUSED(flag)) @@ -1441,12 +1455,8 @@ short delete_keyframe(Main *bmain, ReportList *reports, ID *id, bAction *act, ret += delete_keyframe_fcurve(adt, fcu, cfra); } - /* In the case last f-curve wes removed need to inform dependency graph - * about relations update, since it needs to get rid of animation operation - * for this datablock. */ - if (ret && adt->action == NULL) { - DEG_id_tag_update_ex(bmain, id, ID_RECALC_COPY_ON_WRITE); - DEG_relations_tag_update(bmain); + if (ret) { + deg_tag_after_keyframe_delete(bmain, id, adt); } /* return success/failure */ return ret; @@ -1537,12 +1547,8 @@ static short clear_keyframe(Main *bmain, ReportList *reports, ID *id, bAction *a /* return success */ ret++; } - /* In the case last f-curve wes removed need to inform dependency graph - * about relations update, since it needs to get rid of animation operation - * for this datablock. */ - if (ret && adt->action == NULL) { - DEG_id_tag_update_ex(bmain, id, ID_RECALC_COPY_ON_WRITE); - DEG_relations_tag_update(bmain); + if (ret) { + deg_tag_after_keyframe_delete(bmain, id, adt); } /* return success/failure */ return ret; ```

Brecht Van Lommel commented

2019-02-28 18:02:24 +01:00

Looks good to me.

blender-admin commented

2019-02-28 18:08:22 +01:00

This issue was referenced by 2215541967

This issue was referenced by 2215541967f6a476b9c0189d10c591cbc83879b3

Sergey Sharybin commented

2019-02-28 18:09:30 +01:00

Changed status from 'Open' to: 'Resolved'

Sergey Sharybin closed this issue

2019-02-28 18:09:30 +01:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Non-animated property, stuck in previously keyframed values #62012