Details

Reviewers

Sybren A. Stüvel (sybren)

Group Reviewers

Animation & Rigging

Maniphest Tasks

T72629: Library Override: Support editing of animation data in overriden data-blocks.

Commits

rBc0bd240ad0a1: LibOverride: Add initial support for adding new NLA tracks.

Summary

Also makes NLA tracks and strips overridable.

User can either edit existing strips in existing NLA tracks (but not add or remove them), and/or add new NLA tracks after those comming from the linked data.

Most of the work was as usual checking operators and adding protections against illegal operations in override context.

Note that since we can only rely on indices to deal with local added tracks, we forbid any local track being before any linked/original track.

Diff Detail

Repository: rB Blender

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Nov 20 2020, 11:46 AM

Bastien Montagne (mont29) created this revision.

Harbormaster completed remote builds in B11357: Diff 31239.Nov 20 2020, 11:46 AM

Minor cleanup.

Note that this is similar to what we already do with constraints or modifiers, main difference is that for tracks we do not rely on their names, only their indices, since the former are not unique.

@Pablo Fournier (pablico) @Demeter Dzadik (Mets) would be cool if one of you guys could also do some quick test of this patch before it goes to master. ;)

Minor cleanup.

Harbormaster completed remote builds in B11358: Diff 31240.Nov 20 2020, 11:56 AM

Harbormaster completed remote builds in B11359: Diff 31241.

RedMser (RedMser) added a subscriber: RedMser (RedMser).Nov 21 2020, 11:18 PM

Bastien Montagne (mont29) added a task: T72629: Library Override: Support editing of animation data in overriden data-blocks..Nov 25 2020, 5:30 PM

Pipeliner (Pipeliner) awarded a token.Nov 30 2020, 5:26 PM

I'm not familiar with library overrides API-wise or user-wise but since I'm one of the NLA helper programmers, I'll try to help out. After a bit of testing, there are a few issues:

You can reorder tracks past the linked tracks. When you move a new local track to the bottom, making it the first track, then save and reload the file: the local track seems to be treated as if it's a linked track along with the original linked ones. I can no longer add a new strip to the supposed-to-be-local track, for example.
It looks like you prevent strips from being moved into linked tracks. But linked tracks themselves can still be reordered directly (leading to the above issue again)
When strips are duplicated, they are moved into the next upper track when there's space, even if its a linked track.
In the linking blend file: add local track (A), save. Then go to the linked blend file and add a local track (B), save. Then go back to the linking blend file, then B track is the last track instead of directly after the linked tracks. Track A is between B and the initial set of linked tracks. I'm not sure if this is intentional or not. It doesn't seem intuitive though.
This is likely more of an NLA RNA get/set function issue: when you change a linked strip's action sampling frame_start/end value, then save and reload: the strip's frame_start/end values change (note: not the action's sampling frame bounds) and the action's scale is changed.

I can confirm that this patch fixes T72629.

I'm missing some things in the patch description. What can be library-overridden, and what is not supported? How are things supposed to work?

source/blender/blenkernel/intern/nla.c
296	This sets the `NLATRACK_OVERRIDELIBRARY_LOCAL` on all new tracks. This is inconsistent with the description of the flag, which states "NLA track is added to an override ID".
source/blender/editors/animation/anim_channels_edit.c
1083	This combination of `ID_IS_OVERRIDE_LIBRARY(ale->id)` and `(nlt->flag & NLATRACK_OVERRIDELIBRARY_LOCAL) == 0` occurs multiple times. To me this indicates that this combination expresses some concept that should be a separate function. It can then have the proper name & documentation, to make it clear what happens and how these two conditions relate to each other.
source/blender/editors/space_nla/nla_edit.c
1111	There are many implicit references in this comment: "this case" "it" "this operator" What do they mean?
1111	Why is "almost always" enough to allow something? What of the other cases?
source/blender/editors/transform/transform_convert_nla.c
465	trips → strips
465	What does "we cannot move strips across tracks that come from the linked data" mean for users? Let's say you have this: Track 1: locally editable Track 2: from linked data, so not editable Track 3: locally editable Does that mean you cannot move a strip from track 1 to track 3, because it would go across track 2?
source/blender/makesdna/DNA_anim_types.h
879	That → This.
880	Why is `1<<16` used and not the successor of the last flag, `1<<11`?
source/blender/makesrna/intern/rna_animation.c
756–758	This comment doesn't make much sense to me. I don't remember anything, because I don't know anything about library overrides. Somebody who does remember probably doesn't need the comment. The fact that things are stored the correct way shouldn't need explanation. Apart from those points, the comment still doesn't mean that much to me, but that's probably because I'm not familiar with LibOverride terminology like "anchor".
778	Don't include commented-out code.
source/blender/makesrna/intern/rna_nla.c
884	inserting

This revision now requires changes to proceed.Dec 1 2020, 3:39 PM

In D9611#240589, @Wayde Moss (GuiltyGhost) wrote:

I'm not familiar with library overrides API-wise or user-wise but since I'm one of the NLA helper programmers, I'll try to help out. After a bit of testing, there are a few issues:

You can reorder tracks past the linked tracks. When you move a new local track to the bottom, making it the first track, then save and reload the file: the local track seems to be treated as if it's a linked track along with the original linked ones. I can no longer add a new strip to the supposed-to-be-local track, for example.

It looks like you prevent strips from being moved into linked tracks. But linked tracks themselves can still be reordered directly (leading to the above issue again)

When strips are duplicated, they are moved into the next upper track when there's space, even if its a linked track.

In the linking blend file: add local track (A), save. Then go to the linked blend file and add a local track (B), save. Then go back to the linking blend file, then B track is the last track instead of directly after the linked tracks. Track A is between B and the initial set of linked tracks. I'm not sure if this is intentional or not. It doesn't seem intuitive though.

This is likely more of an NLA RNA get/set function issue: when you change a linked strip's action sampling frame_start/end value, then save and reload: the strip's frame_start/end values change (note: not the action's sampling frame bounds) and the action's scale is changed.

Point 1 and 2 are direct consequences of NLA track names not being ensured as unique. Therefore we can only use indices to keep track of inserted local strips...

Point 3 is fixed in updated patch.

Point 4 is expected behavior, but indeed not necessarily ideal in current situation.

Point 5 is indeed because we have an RNA setters dependency (action_frame_end -> frame_end -> action_scale e.g., many more scenarii are possible here). I have no good solution tbh, those kind of things are just bad in liboverride context. Only thing I can think of is making some of those not overridable (probably the action_ properties)?

Point 1,2 and 4 are now addressed by forbidding any local tracks to exist before any tracks from linked data. %This is not so ideal solution, but only viable one unless we get unique NLA track names.

source/blender/blenkernel/intern/nla.c
296	This flag is irrelevant in non-override cases, adding a check whether we are in an override case would add extra boiler code that would clutter readability and make change noisier, for no actual benefit.
source/blender/editors/animation/anim_channels_edit.c
1083	I would rather not. At least not for now. Ideally this whole 'local versus linked' items status of RNA collections (ListBase in C usually) could be abstracted behind a generic API, but doing that properly and in a not too much invasive way is not easy, and definitively out of the scope of this patch. In the mean while, I'd rather keep things simple and explicit, and not hide this behind a one-liner function that would really just clutter readability of what actually happens imho. For now other places (constraints and modifiers) do it that way as well, prefer to keep consistency here.
source/blender/editors/space_nla/nla_edit.c
1111	People can add custom shortcut or execute this operator directly... Transform is only triggered if it is invoked.
1111	Isn't it obvious that they all refer to the same thing (this operator)? ... Will rephrase it to make it more clear then.
source/blender/editors/transform/transform_convert_nla.c
465	Does that mean you cannot move a strip from track 1 to track 3, because it would go across track 2? Yes. But does not matter anymore, since we now forbid any local track being before any 'linked' ones, so just means you cannot add/move a strip into a linked track, and cannot move a strip from a linked track into any other one.
source/blender/makesdna/DNA_anim_types.h
880	To leave space to logically group potential future bitflags (exactly just like `DISABLED` does not follow `PROTECTED`).
source/blender/makesrna/intern/rna_animation.c
756–758	This is valid comment regarding internals of how override operations are stored, which may not remain true forever (see T82160: LibOverrides - Refactor how diffing of RNA collections is handled, and extend diffing possibilities.). This is override stuff, not related to NLA area at all. This code will be disabled for now anyway, until NLA tracks get proper unique names at least. But will still be there (do not want to remove that).
778	This is done for debugging purpose and is intended to be kept for now (have them in other similar 'insert in collection' cases, like modifiers...)

updates from review.

Harbormaster completed remote builds in B11490: Diff 31495.Dec 1 2020, 4:49 PM

Bastien Montagne (mont29) edited the summary of this revision. (Show Details)Dec 1 2020, 4:50 PM

Note that am not really asking for a review of override code itself (in RNA parts), more for validation of needed changes to NLA tools and operators...

In D9611#240959, @Bastien Montagne (mont29) wrote:

Note that am not really asking for a review of override code itself (in RNA parts), more for validation of needed changes to NLA tools and operators...

If you don't want reviewers to look at certain areas of a patch, please let them know in the description of that patch, and not after a review has been done already. Now I'm getting a "you shouldn't have looked at that" vibe, which isn't really nice.

source/blender/blenkernel/BKE_nla.h
65	For parameters that are passed by value, `const` isn't necessary in a function declaration. See the Clang-Tidy readability-avoid-const-params-in-decls rule for more info.
source/blender/blenkernel/intern/nla.c
296	I'm not saying this use is wrong. I'm saying this use is inconsistent with the description of the flag. Consistency of explanation of a flag and actual use of the flag does not reduce readability. I don't see how you could argue this.
source/blender/editors/animation/anim_channels_edit.c
1083	Placing some code into a well-named function that has well-defined and well-explained behaviour is NOT the same as "hiding" it. I'm also not arguing for some generic API that can be used everywhere. I would find a function `nla_is_readonly_liboverride(owner_id, nla_track)` so much easier to read than the current code. Functions should strive to work on a single abstraction level only. When reordering NLA tracks, I don't want to know all the details of how the liboverride system works. This is basic code abstraction 101.
1176–1187	This seems to assume that non-local NLA tracks are always listed first, and local ones later. Is this indeed the case? If so, it's not obvious to me what code guarantees this, so maybe you could add a comment that points to it?
source/blender/editors/space_nla/nla_edit.c
1111	How are those cases supposed to ensure the data is still valid?

Sybren A. Stüvel (sybren) requested changes to this revision.Dec 1 2020, 5:54 PM

This revision now requires changes to proceed.Dec 1 2020, 5:54 PM

Updates from review.

In D9611#240975, @Sybren A. Stüvel (sybren) wrote:

In D9611#240959, @Bastien Montagne (mont29) wrote:

Note that am not really asking for a review of override code itself (in RNA parts), more for validation of needed changes to NLA tools and operators...

If you don't want reviewers to look at certain areas of a patch, please let them know in the description of that patch, and not after a review has been done already. Now I'm getting a "you shouldn't have looked at that" vibe, which isn't really nice.

I should have yes, and that's why I added it as comment when I realized this was not obvious. ;)

source/blender/blenkernel/BKE_nla.h
65	Thanks, didn't know that!
source/blender/blenkernel/intern/nla.c
296	have you seen that I updated the flag description already, stating that this flag is only meaningful in override case?
source/blender/editors/animation/anim_channels_edit.c
1083	Guess we'll have to agree to disagree here then...
1176–1187	Whole editing code ensures that now (see my changes from yesterday), as well as the 'apply override rules' for tracks RNA collection. Will add that to comment.
source/blender/editors/space_nla/nla_edit.c
1111	They are not... Not sure how to do it, and not sure it's worth the effort since user should not typically be able to do that. Also, this does not really generate 'invalid' data, it only means once re-read the result will not match what was saved. Alternative I can think of right now it to forbid that op on strips from non-local tracks in override case, which would be much worse UX-wise imho (as in, forbidding by far most common use case to work nicely, just to prevent potential non-critical issues in some very unlikely cases)?

Harbormaster completed remote builds in B11511: Diff 31524.Dec 2 2020, 10:37 AM

Sybren A. Stüvel (sybren) added inline comments.Dec 7 2020, 12:31 PM

source/blender/editors/space_nla/nla_edit.c
1111	In that case, 👍 on how this is implemented now.
source/blender/makesdna/DNA_anim_types.h
880	👍

Updated from review.

Mainly adding helper function to check for local vs. linked NLA tracks in override case.

Harbormaster completed remote builds in B11595: Diff 31699.Dec 7 2020, 5:24 PM

The BKE_nlatrack_is_local_in_liboverride() function melts my brain a bit. There are so many negations going on, and then the majority of the calls to that function require yet another negation of the returned result.

\note This check is only valid for a liboverride data-block, it always return \a true otherwise.

I'm not a fan of functions that are described as invalid in a certain situation but still are used in that situation and treated as valid. This on top of the triple negations is just too confusing.

Why not keep the original logic as you had and just turn it into a function?

This revision now requires changes to proceed.Dec 7 2020, 5:47 PM

Update from review

Harbormaster completed remote builds in B11610: Diff 31726.Dec 8 2020, 9:54 AM

Thanks for the update.

This revision is now accepted and ready to land.Dec 8 2020, 10:16 AM

Closed by commit rBc0bd240ad0a1: LibOverride: Add initial support for adding new NLA tracks. (authored by Bastien Montagne (mont29)). · Explain WhyDec 8 2020, 10:56 AM

This revision was automatically updated to reflect the committed changes.

Bastien Montagne (mont29) added a commit: rBc0bd240ad0a1: LibOverride: Add initial support for adding new NLA tracks..

Wayde Moss (GuiltyGhost) mentioned this in D10102: NLA: Strip Post-transform Horizontal Shuffle.Jan 13 2021, 7:47 AM

Wayde Moss (GuiltyGhost) mentioned this in D10103: NLA: Strip Post-transform Vertical Shuffle and Auto-Grow Track List.Jan 13 2021, 8:17 AM

LibOverride: Add initial support for adding new NLA tracks.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 31727

source/blender/blenkernel/BKE_nla.h

source/blender/blenkernel/intern/ipo.c

source/blender/blenkernel/intern/nla.c

source/blender/editors/animation/anim_channels_edit.c

source/blender/editors/object/object_add.c

source/blender/editors/space_action/action_data.c

source/blender/editors/space_nla/nla_channels.c

source/blender/editors/space_nla/nla_edit.c

source/blender/editors/transform/transform_convert.c

source/blender/editors/transform/transform_convert_nla.c

source/blender/makesdna/DNA_anim_types.h

source/blender/makesrna/intern/rna_access_compare_override.c

source/blender/makesrna/intern/rna_animation.c

source/blender/makesrna/intern/rna_nla.c

source/blender/makesrna/intern/rna_rna.c

source/blender/makesrna/intern/rna_space.c

LibOverride: Add initial support for adding new NLA tracks.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 31727

source/blender/blenkernel/BKE_nla.h

source/blender/blenkernel/intern/ipo.c

source/blender/blenkernel/intern/nla.c

source/blender/editors/animation/anim_channels_edit.c

source/blender/editors/object/object_add.c

source/blender/editors/space_action/action_data.c

source/blender/editors/space_nla/nla_channels.c

source/blender/editors/space_nla/nla_edit.c

source/blender/editors/transform/transform_convert.c

source/blender/editors/transform/transform_convert_nla.c

source/blender/makesdna/DNA_anim_types.h

source/blender/makesrna/intern/rna_access_compare_override.c

source/blender/makesrna/intern/rna_animation.c

source/blender/makesrna/intern/rna_nla.c

source/blender/makesrna/intern/rna_rna.c

source/blender/makesrna/intern/rna_space.c

LibOverride: Add initial support for adding new NLA tracks.
ClosedPublic
Actions

Revision Contents
Changeset List