Responding to selection changes

Dear fellow Blender developers,

TL;DR: syncing selection between editors is broken, and needs a redesign. Please help.

Intro to Selection Sync

Currently Blender responds to certain changes in selection, and updates the selection state of other things. For example:

  • Select a Bone in pose mode → the corresponding Animation Channel Group is selected.
  • Select an Animation Channel Group in the Dope Sheet → the corresponding Bone is selected.
  • Select a Shader Node → its animation channels are selected.

In principle this is all fine, as it helps animators to quickly find relevant animation channels based on their selection in the 3D viewport, node editor, etc.

The current problem

Selection syncs are handled via the window manager notification system, and in my opinion in an unreliable and badly designed way. It causes reports like these:

As far as I’ve been able to figure out from the code, this is what’s happening. I’ve included my comments about what’s happening. I’ve marked the loss of contextual information with a :x:.

What Happens My interpretation
User selects a bone This is fine
A notification is sent to the window manager, indicating that bone selection changed :x: This has no information about which Object changed
This notification is received by the Action editor ok
Action editor sets a runtime flag that indicates “channels need syncing” :x: This looses information, from the specific “bone selection changed” to the generic “sync necessary”
Action editor tags the area for refresh via the very generic ED_area_tag_refresh() function by now, all context is gone, and only that one flag is all that’s left
The window manager handles the refresh tags, and calls the Action editor’s refresh function ok
Action editor’s refresh function looks at the flag, and when set, effectively deselects all FCurves, then selectes curves that refer to something (bone, shader node) that is selected This is problematic, as it synces too much (shader node channel selection is synced too)
Action editor’s refresh function clears the flag, and calls ED_area_tag_redraw() on itself This shouldn’t be necessary

From a glance, things don’t look too bad:

  • By using the notification system, the part of the code that changes the bone selection doesn’t need to know about the selection synchronisation. It just needs to send a generic Window Manager notification and the rest is taken care of automatically.
  • By using the “tag for refresh” functionality, handling the notifications themselves is very cheap. This makes it possible send many notifications, while still causing only one “loop over all the visible animation channels and sync their selection” step.

However, from “My Interpretation” above, you can see that there are various issues with the current code (for example: the existence of pose animation channels causes node animation channels to become unselectable).

My proposal

My proposal to address the above issues is:

  • (A): Create functions for specific things for which we want to sync selections. These should take a pointer to whatever was changed (Object*, bNodeTree*), and should be limited to the FCurves in that Object/Node Tree’s action, so that the selection state of unrelated animation data is unaffected.
  • (B): Code that wants to modify the selection state of bones/nodes, should call dedicated functions for this (instead of modifying the DNA directly).
  • The (B) functions shouldn’t directly call the (A) functions, as selecting many bones in a for-loop should be kept efficient. They could set some runtime flag “flush selection state to animation” on the Object or Node Tree (or on the ID or AnimData struct to keep it generic).

After all the selection changes are done, we need some way to call the relevant (A) functions. This could be done in response to window manager notifications similar to what’s done now. With the proposed tags, the current Action Editor refresh functions could be reused, looking at the owner of the animation data to see if they’re tagged.

Alternatively, the (B) functions could be made in such a way that they keep track of the objects that require syncing, and that list could at some point be used by the window manager notification system. This would remove the “refreshing also alters selection state in DNA” quirk that we have now, at the expense of having to find a place to do the relevant bookkeping.

What do other devs think about this?

1 Like

I am all up for moving away from notifier system. From quick Friday evening reading, the proposal is sound.

Not sure if we should try to defer synchronization up to the next usage point. Don’t think current notifier-based approach defers anything in this regard? Maybe current notifier approach avoids some calculations if there are no animation editors open. But that’s probably not the case in average usecase.

Anyway. define API, use the API instead of direct DNA access. Deferring can be then implemented (if needed) as a detail of API implementation.

2 Likes

In general the notifier system is the wrong solution for many things that it is used for, and this is one of them.

I don’t mind too much to have a specialized system for this, but if we are looking for something more generic I would do it like this:

  • Datablocks with modified selection state should be tagged with ID_RECALC_SELECT.
  • This system should be decoupled from the dependency graph. Right now DEG_id_tag_update does a lot of work immediately. But tagging modifications of original datablocks could be done very quickly by setting a bitflag, and then later the dependency graphs can pick them up and do the appropriate operations.
  • Editors would then also be able to have a callback to listen to such changes on original datablocks (which is distinct from listing to changes of evaluated datablocks of a particular depsgraph).
  • For efficiency, you’d want to be able to quickly check which types of datablocks are modified and which types of tags there were, rather than always looping over all of them.
  • For example the undo system can also benefit from this. It is now somewhat tied to the dependency graph even for original datablocks, which makes little sense.
3 Likes

Besides notifiers not being ideal, I’m not actually sure how changing to a different system will address the listed bug reports. If you know which specific objects or bones changed selection, you could sync only those, but what problem does that solve?

what problem does that solve?

It’ll allow me to actually improve the functionality of selection sync, as I don’t think I should be trying to change the behaviour without understanding and addressing the fundamental flaws in the current design first.

For one, it allows for a proper solution to T62463 Skeleton rig with keyframes prevents selection of Shader Nodetree channels in Dope Sheet and Graph Editor . My workaround to get at least the channels to unblock is to delete some sync code that could otherwise be useful.

It depends a bit on what is considered to be the fundamental flaw. It’s not obvious to me why selecting a channel would cause a scene data -> animation editor sync in the first place (which is what I assume is causing this).

What I see as a fundamental flaw is that an operation that I expect to be idempotent (refreshing some area of the UI) is used to perform an action that is not.

As far as I know, the selection sync is often used by animators to quickly find the relevant channels in the animation editor. Note that selection sync is only active when “Only Selected” is disabled, i.e. when more animation data is shown than just that of the selected object/bone/node/etc.

For these two bugs, I think some design is also needed, as well as/in addition to/ improving the syncing system under the hood:

and

It seems this one happens from a ‘two way’ issue - it’s by design, in that selection in the 3D view selects channels, but selection in the dope sheet editor (if show only selected is off) selects bones. I think the latter isn’t as useful - I wonder if it would be better if restricting it to cases where you actively click on the channelbox rather than the keys/graphs themselves.