Fix T84586: missing Outliner redraws for certain NLA operators
ClosedPublic
Actions

Authored by Philipp Oeser (lichtwerk) on Jan 11 2021, 2:49 PM.

Details

Reviewers

Sybren A. Stüvel (sybren)
Wayde Moss (GuiltyGhost)
Julian Eisel (Severin)

Group Reviewers

User Interface
Animation & Rigging

Maniphest Tasks

T84586: Use-after-free when expanding hierarchy in Outliner after deleting NLA track

Commits

rBb4530deec478: Fix T84586: missing Outliner redraws for certain NLA operators

Summary

Outliner display under 'Animation' > 'NLA Tracks' was not updating in
the following cases:

adding strips
removing strips
duplicating strips (possibly to different track)
swapping strips
reordering tracks
changing strip order by translating
translating strips between tracks
renaming tracks

In the case of deleting strips/tracks, this was also resulting in a use-
after-free error in Outliner drawing code (this was reported specifically
in T84586).

Most of these operators already sent a ND_NLA|NA_EDITED notifier, but the
Outliner is not listening to these. Listening to NA_EDITED is also not
what we want since this also happens a lot in cases irrelevant to the
Outliner. Now be a bit more specific and send ND_NLA|NA_ADDED / ND_NLA|
NA_REMOVED or a new ND_NLA_ORDER (to distinguish from NA_EDITED
'only' - where a redraw is not neccessary) and listen to these from the
Outliner.

(note: places that were listening to ND_NLA|NA_EDITED before are also
listening to NA_ADDED or NA_REMOVED, so changing NA_EDITED should not be
a problem here)

(note 2: for cases like swapping tracks/strips order, NA_ADDED or
NA_REMOVED does not make sense, neither can we use NA_EDITED [since we
dont want to listen to this], so in this case an additional ND_NLA_ORDER
is now sent)

(note 3: in nla transform code, this is now always sent on confirm. There
are cases were the notifier would not be needed, but checking exactly all
cases were it actually would be needed seems overkill [history of D10073
has example code to check if strips moved between tracks])

Diff Detail

Repository: rB Blender

Event Timeline

Philipp Oeser (lichtwerk) created this revision.Jan 11 2021, 2:49 PM

Harbormaster completed remote builds in B12098: Diff 32611.Jan 11 2021, 2:49 PM

Philipp Oeser (lichtwerk) requested review of this revision.Jan 11 2021, 2:49 PM

Philipp Oeser (lichtwerk) added projects: User Interface, Animation & Rigging, Pipeline, Assets & I/O.Jan 11 2021, 2:55 PM

Philipp Oeser (lichtwerk) added reviewers: User Interface, Bastien Montagne (mont29), Sybren A. Stüvel (sybren).

Philipp Oeser (lichtwerk) added a subscriber: Robert Guetzkow (rjg).Jan 11 2021, 2:59 PM

Adding @Wayde Moss (GuiltyGhost) as reviewer, he knows the NLA better than I do. I also added the Animation & Rigging module as blocking reviewer as per our experimental review approach.

I didn't have a chance to look at it today. I am familiar with the NLA area but not so much its relation with the Outliner. I can look into it for the review but it seems more like a general outliner review and not so much specific to the NLA system? So don't let me slow you down.

This review is based on the user-end and less about proper implementation.

Moving strips between tracks (move_up/down operators and transform operator) doesn't update the outliner.
Reordering tracks does not update the outliner
Swapping (operator) strips doesn't reorder actions (really minor)
Snapping strips doesn't update the outliner (a new track is created in the case of single track stack with two strips [ selected | not selected ] and we snap the selected strip to the current frame, overlapping the non-selected strip)
duplication does now add an nla outliner-track but fails to add a new outliner-action

This revision now requires changes to proceed.Jan 13 2021, 7:40 PM

cover the remaining cases @Wayde Moss (GuiltyGhost) mentioned

Harbormaster completed remote builds in B12194: Diff 32782.Jan 14 2021, 5:22 PM

Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)Jan 14 2021, 5:24 PM

Previous issues appear to have been resolved, thanks.

One more case is that renaming tracks doesn't update in the outliner. Apologies for missing this case. After that, the patch looks good from user-side.

source/blender/editors/transform/transform_convert_nla.c
570	stripe

Ignore the missing case. It updates when mousing over the outliner.

In D10073#252742, @Wayde Moss (GuiltyGhost) wrote:

Ignore the missing case. It updates when mousing over the outliner.

Can cover that, too :) Gotta run, will do tomorrow

source/blender/editors/animation/anim_channels_edit.c
1583	This shouldn't send Outliner notifiers IMHO. Animation operators should send animation notifiers, it's the Outliners responsibility to listen to the notifiers correctly. Space notifiers should only be send if space data changes, not scene data. Same applies to the cases below of course.
source/blender/editors/space_nla/nla_edit.c
2323	Better don't do this in a loop, but at the end of the function. You can always send it, or use a `any_added` boolean.
source/blender/editors/transform/transform_convert_nla.c
567	Wouldn't keep this in.