While it is true that depsgraph will not do id user counter, I am not sure if there any other cases when this is the case.

Could imagine that main database free also doesn't do ID users. And this is actually all good: optimization on closing file:

@Bastien Montagne (mont29), think you would know best here.

seq_free_animdata should really be using BKE_animdata_fix_paths_remove, it's not handling drivers and NLA now.

Yes, it's a different issue than this but nice to make the code both simpler and more correct.

Regarding the flags to check. I think we want to only remove animation if it's a datablock part of the main database that is actually saved to the .blend. And we only want to do it when that datablock is getting removed, not when it is merely freed. So that would mean it's:

if (!(id->tag & LIB_TAG_NO_MAIN) && do_id_user)

But maybe the !(id->tag & LIB_TAG_NO_MAIN) part is implied by do_id_user?

@Bastien Montagne (mont29)?

In D7264#174977, @Brecht Van Lommel (brecht) wrote:

Yes, it's a different issue than this but nice to make the code both simpler and more correct.

Regarding the flags to check. I think we want to only remove animation if it's a datablock part of the main database that is actually saved to the .blend.

I guess it wouldn't be a problem now, but I don't really see reason why to do this only for datablocks stored in main. I think it would be quite weak to build logic based on that flag (unless we already do)

And we only want to do it when that datablock is getting removed, not when it is merely freed.

This could be true if Sequence was datablock and owned AnimData. But now we want to remove animation even if we delete strip. No datablock is removed in this case.

So that would mean it's:

if (!(id->tag & LIB_TAG_NO_MAIN) && do_id_user)

But maybe the !(id->tag & LIB_TAG_NO_MAIN) part is implied by do_id_user?

@Bastien Montagne (mont29)?

It doesn't look that it is implied, it is pretty much hard-coded at 2 places - BKE_scene_copy and scene_free_data
do_id_user really means, that usercount should be changed. So if curve has only one user, it should be removed from list if BKE_sequence_free_ex is called with do_id_user = true

First of all, i would like to actually understand what is the problem here, "Changes introduced in commit be2e41c397ba caused, that BKE_sequence_free_ex() removed fcurve pointers belonging to strips from scene CoW datablock." is... vague, at best.

Relying on do_id_user for that is certainly not correct, default generic ID freeing code path e.g. will call that function without it.

The only change I can find in rBbe2e41c397ba is that call to BKE_animdata_free() is now made at the end, after specific IDType freeing callback has been run, while previously that call was typically done earlier, in Scene case, before the sequencer data would be freed.

So think if we want to restore previous situation, simplest solution is to move, in BKE_id_free_ex, the cqll to BKE_libblock_free_data before the call to BKE_libblock_free_datablock.

But again, I find it very unsettling that order matters here, and would very much to understand better the root of the issue - why is it a problem when freeing CoW Scene data-block, that scene's animdata are not freed when we handle sequencer data?

And changes in seq_free_animdata should really be handled separately...

In D7264#176919, @Bastien Montagne (mont29) wrote:

First of all, i would like to actually understand what is the problem here, "Changes introduced in commit be2e41c397ba caused, that BKE_sequence_free_ex() removed fcurve pointers belonging to strips from scene CoW datablock." is... vague, at best.

I will try to rephrase:

seq_free_animdata() removes fcurve pointers belonging to strips from AnimData CoW datablock during BKE_scene_graph_update_for_newframe. This causes problems with updating animation.
This worked before rBbe2e41c397ba, because AnimData was freed by BKE_animdata_free() before seq_free_animdata() was executed, so it had no data to operate on and returned on precondition if (scene->adt == NULL || scene->adt->action == NULL)

Is that clearer?

Relying on do_id_user for that is certainly not correct, default generic ID freeing code path e.g. will call that function without it.

I could agree that it is not correct, but I will need some flag, that indicates that data are not actually removed "forever", but they are only updated instead.
I don't know if there is similar case to look at how it should be done.

And changes in seq_free_animdata should really be handled separately...

Ok I can make separate patch for this

Think I understand a bit better yes.

but then... How in Hell can we allow a BKE function (assumed to be somewhat low-level), which has a rather specific, limited scope even, to go mess around a completely different datablock's data? This should be completely forbidden...

TBH I would entirely remove the call to seq_free_animdata from BKE_sequence_free_ex. in fact, the former should not even be a BKE function, but and ED one I think. And it should be explicitly called by (ED or RNA) code actually deleting a sequence. This would be the best (as in, cleanest and soundest) thing to do imho.

Alternatively, a less involved solution would be to add an extra do_clean_animdata or so flag to BKE_sequence_free_ex, but again, I would really rather keep that kind of very high-level logic outside of the BKE area.

PS: AnimData is not a datablock, Action is.

In D7264#176944, @Bastien Montagne (mont29) wrote:

Think I understand a bit better yes.

but then... How in Hell can we allow a BKE function (assumed to be somewhat low-level), which has a rather specific, limited scope even, to go mess around a completely different datablock's data? This should be completely forbidden...

TBH I would entirely remove the call to seq_free_animdata from BKE_sequence_free_ex. in fact, the former should not even be a BKE function, but and ED one I think. And it should be explicitly called by (ED or RNA) code actually deleting a sequence. This would be the best (as in, cleanest and soundest) thing to do imho.

Hm indeed this sounds like a good solution.

Alternatively, a less involved solution would be to add an extra do_clean_animdata or so flag to BKE_sequence_free_ex, but again, I would really rather keep that kind of very high-level logic outside of the BKE area.

PS: AnimData is not a datablock, Action is.

Sorry, I thought it is, didn't check.. will change and update description

I guess this is an incomplete patch, since it changes the function signature of BKE_sequence_free but there's no other files affected?

I think an approach like this is reasonable, even as a proper solution.

I suggest to have a BKE_sequence_delete() and BKE_sequence_free() in the API. Then those can both call the same internal function, just with a different argument to delete the animation data or not.