Page MenuHome

Set names of cut sequences properly, so that the duplicated animation data is attached to the right sequence
AbandonedPublic

Authored by Guillaume M (mathers) on Jan 10 2019, 12:09 AM.

Details

Summary

This patch solves bug T60194.

It reverts changes made by commit bb98e83b99e63348e0396a5ffe5bb2a20ff1607a by @Bastien Montagne (mont29), but it does not seem to cause a regression on bug T55668. Even with the patch applied I was unable to reproduce this bug.

Diff Detail

Repository
rB Blender

Event Timeline

After a bit more testing, it looks like this patch indeed solves major bug T60194, but also introduces minor bug T60194.

I don't see how to solve T60370, if anyone sees the issue, can you submit a new revision to this patch, that solved T60194 without introducing T60194 ?

Is that new introduced bug, that curves are copied, but not functioning?

That may be, because BKE_animdata_fix_paths_rename may be getting wrong paths

in seq_dupli

		if (dupe_flag & SEQ_DUPE_ANIM) {
			BKE_sequencer_dupe_animdata(scene_dst, seq->name + 2, seqn->name + 2);
}

signature is void BKE_sequencer_dupe_animdata(Scene *scene, const char *name_src, const char *name_dst)

in our case src is dst - doesn't make sense. Can not test this at the moment - just guessing

In the newly introduced bug, the curves are initially not functioning, but "spring into action" as soon as any kind of animation data is changed in the strip (even a cancelled move - hitting G on a keyframe and hitting ESC) .

Changing the animation data must refresh something, I'll try to look into that over the weekend.

but "spring into action" as soon as any kind of animation data is changed in the strip (even a cancelled move - hitting G on a keyframe and hitting ESC)

This operator is quite complex. Can you verify, if this happens, when you change length of strip?

I wonder if do_sequence_frame_change_update(scene, seq) will trigger this. It shouldn't as it's also called by cut OP

I removed that flag because it was doing double 'unique name' check, since a global one is done as final step in sequencer_cut_exec itself. But I guess this messed up animation, which also relies on names (RNA paths).

If you triple-checked that T55668 does not pop up back, then think the patch is OK (though not ideal, but…).

Hmmpf, should have refreshed the page before posting it seems :P

Also things are more complex than it seems here, rBbb98e83b99 is indeed fully broken when it comes to animation, since it does not do any renaming in fcurves/drivers… And it's actually too late to perform it for animdata, there is no way anymore to tell which curve applies to which strip.

So think rBbb98e83b99 should be reverted, and a better solution working in-place (as original one, but actually working as expected) should be implemented instead. can look into that, unless @Guillaume M (mathers) you are still interested to investigate?

That's fine by me, you can revert rBbb98e83b99 and submit a new revision that solves T55668 without introducing T60194.

Let me know when it's done and I'll test it again !

OK, will close that patch then, thanks.

I have looked into this.

Seems that curves were copied properly and RNA paths are also applied properly.

Animdata evaluation is done by depsgraph.
Problem with not evaluating newly created curve is, that depsgraph wasn't notified about this change and depsgraph is working with old copy of scene.

To be fair I don't speak depsgraph, so I don't know how to notify it.
I tried to notify WM by adding WM_event_add_notifier(C, NC_ANIMATION | ND_ANIMCHAN | NA_EDITED, NULL); to cut OP, but no success.

I hooked up depsgraph CoW update, and animdata are still not evaluated.
this was done by including DEG_id_tag_update_ex(CTX_data_main(C), &scene->id, 0); before end of cut OP

after copying adt, foreach_libblock_remap_callback modifies adt pointer to another location with adt struct with uncopied second f-curve.

I would probably try to see if there are other objects, that will duplicate animdata and trace, how they do it / see if it works at all.