Outliner: New context menu building design #101007

New Issue

Julian Eisel · 2022-09-12T14:48:32+02:00

Julian Eisel commented

2022-09-12 14:48:32 +02:00

Motivation
The way we build context menus in outliner_tools.cc is known to be quite a mess. Basic problems:

Bad Encapsulation: The entries typically operate on scene data. And usually the logic for these is implemented in the Outliner context menu code itself. This violates encapsulation, and the operations tend to have issues because they are badly maintained (ask @mont29 :) ). When mixing UI and scene data-management, responsibilities become unclear. Two separate departments need knowledge about each other to keep the features maintained.
Inconsistent: The way we build these menus is unlike any other menu building in Blender. Most entries don't use the regular operator system either. This breaks familiarity, and standard operator features like names, description and undo pushing have to be managed differently/manually.
Messy Code: The code is a bit chaotic and generally it's not as simple to add menu entries as it should be. Too much upfront digging is needed, you have to understand the unusual menu building system first.
User Visible Consequences: Some menu entries had a tooltip showing "(undocumented operator)", or the undo push name would differ from the operator name. Often menu entries were added that didn't make sense for the current selection, because hiding them requires special attention. With regular operators this is less likely to happen, since developers are used to implement them properly.

Proposed Design

TL;DR: Add batch editing operators outside of Outliner code, they can act on context set by the Outliner. Tree elements are asked to add context menu entries themselves.

Main idea is:

Rather than a central place to define the context menu (outliner_tools.cc), ask the active element to expand the context menu. For this, element types can override AbstractTreeElement::expandContextMenu().
Further general entries (e.g. the View and Area sub-menus) can be added in Python via OUTLINER_MT_context_menu. This is already the case in master.
Use proper operators for entries, defined in the relevant code modules, not the Outliner. Typically these operators would have to support batch operations to act on the whole Outliner selection.
The Outliner "broadcasts" information about its selection by setting context, which the operators can access.

Further Considerations
The context system has some limitations still:

Context can only hold RNA pointers. Data that isn't covered by RNA can't be "broadcast" this way currently.
Some operators rely on the Outliner hierarchy. For example library overrides use it to find out which data-blocks need to be overridden with a system override. How to "broadcast" this hierarchy via context (also see previous point)?
Querying lists of data (e.g. "selected_ids") can be inefficient. The Outliner context callback iterates over the entire tree to find the selection, and then allocates list elements for each.
Similarly, getting information about the selection can be inefficient. E.g. you have to request the entire list (that is then built as explained above), just to see if there are elements of interest selected. This is a problem for operator poll functions.

There are ideas to solve each, but they don't seem like a blocker for the design above. For now it's still fine to leave some menu entries as callbacks rather than operators, where context doesn't allow us to broadcast the necessary information. The actual logic should still be moved to the dedicated code modules. Performance impact is probably minor in practice.

Also:

It would be nice to have the context menu entries defined in Python. There could be a menu type for each element type (e.g. OUTLINER_MT_modifier_context_menu). But for as long as some entries may have to stick to a callback based approach, keeping things in C++ mostly seems better.
Some operators may invalidate the data displayed by the Outliner. So care needs to be taken to avoid use-after-free errors or similar. Hopefully this isn't an issue because the tree is rebuilt before any further access.

**Motivation** The way we build context menus in `outliner_tools.cc` is known to be quite a mess. Basic problems: - **Bad Encapsulation:** The entries typically operate on scene data. And usually the logic for these is implemented in the Outliner context menu code itself. This violates encapsulation, and the operations tend to have issues because they are badly maintained (ask @mont29 :) ). When mixing UI and scene data-management, responsibilities become unclear. Two separate departments need knowledge about each other to keep the features maintained. - **Inconsistent:** The way we build these menus is unlike any other menu building in Blender. Most entries don't use the regular operator system either. This breaks familiarity, and standard operator features like names, description and undo pushing have to be managed differently/manually. - **Messy Code:** The code is a bit chaotic and generally it's not as simple to add menu entries as it should be. Too much upfront digging is needed, you have to understand the unusual menu building system first. - **User Visible Consequences**: Some menu entries had a tooltip showing `"(undocumented operator)"`, or the undo push name would differ from the operator name. Often menu entries were added that didn't make sense for the current selection, because hiding them requires special attention. With regular operators this is less likely to happen, since developers are used to implement them properly. **Proposed Design** *TL;DR: Add batch editing operators outside of Outliner code, they can act on context set by the Outliner. Tree elements are asked to add context menu entries themselves.* Main idea is: - Rather than a central place to define the context menu (`outliner_tools.cc`), ask the active element to expand the context menu. For this, element types can override `AbstractTreeElement::expandContextMenu()`. - Further general entries (e.g. the *View* and *Area* sub-menus) can be added in Python via `OUTLINER_MT_context_menu`. This is already the case in master. - Use proper operators for entries, defined in the relevant code modules, not the Outliner. Typically these operators would have to support batch operations to act on the whole Outliner selection. - The Outliner "broadcasts" information about its selection by setting context, which the operators can access. **Further Considerations** The context system has some limitations still: - Context can only hold RNA pointers. Data that isn't covered by RNA can't be "broadcast" this way currently. - Some operators rely on the Outliner hierarchy. For example library overrides use it to find out which data-blocks need to be overridden with a system override. How to "broadcast" this hierarchy via context (also see previous point)? - Querying lists of data (e.g. `"selected_ids"`) can be inefficient. The Outliner context callback iterates over the entire tree to find the selection, and then allocates list elements for each. - Similarly, getting information about the selection can be inefficient. E.g. you have to request the entire list (that is then built as explained above), just to see if there are elements of interest selected. This is a problem for operator poll functions. There are ideas to solve each, but they don't seem like a blocker for the design above. For now it's still fine to leave some menu entries as callbacks rather than operators, where context doesn't allow us to broadcast the necessary information. The actual logic should still be moved to the dedicated code modules. Performance impact is probably minor in practice. Also: - It would be nice to have the context menu entries defined in Python. There could be a menu type for each element type (e.g. `OUTLINER_MT_modifier_context_menu`). But for as long as some entries may have to stick to a callback based approach, keeping things in C++ mostly seems better. - Some operators may invalidate the data displayed by the Outliner. So care needs to be taken to avoid use-after-free errors or similar. Hopefully this isn't an issue because the tree is rebuilt before any further access.

❤️ 1

Julian Eisel commented

2022-09-12 14:48:32 +02:00

Added subscribers: @mont29, @JulianEisel

Brecht Van Lommel commented

2022-09-12 16:05:48 +02:00

Added subscriber: @brecht

Brecht Van Lommel commented

2022-09-12 16:05:48 +02:00

I think all menu entries being proper operators makes sense. I'm less sure about asking the active element to expand the context menu, and broadcasting information through context.

The purpose of context is that operators can run in different editors. If the information is outliner specific (hierarchy etc), it's an unnecessary layer of abstraction only adding complexity, and I would just let operators access outliner data structures directly.

Letting the elements fill the menu is unclear to me, not sure how that works when you have multiple selected elements, mixed types. There might be some logic there that's much more easily expressed in a central function to fill the menu. Maybe it's not an issue, but I vaguely remember some logic that would prioritize e.g. objects over other data types in mixed selections. If it can be made to work then the extensibility is nice though.

I think all menu entries being proper operators makes sense. I'm less sure about asking the active element to expand the context menu, and broadcasting information through context. The purpose of context is that operators can run in different editors. If the information is outliner specific (hierarchy etc), it's an unnecessary layer of abstraction only adding complexity, and I would just let operators access outliner data structures directly. Letting the elements fill the menu is unclear to me, not sure how that works when you have multiple selected elements, mixed types. There might be some logic there that's much more easily expressed in a central function to fill the menu. Maybe it's not an issue, but I vaguely remember some logic that would prioritize e.g. objects over other data types in mixed selections. If it can be made to work then the extensibility is nice though.

Julian Eisel commented

2022-09-12 16:20:50 +02:00

In #101007#1415770, @brecht wrote:
The purpose of context is that operators can run in different editors. If the information is outliner specific (hierarchy etc), it's an unnecessary layer of abstraction only adding complexity, and I would just let operators access outliner data structures directly.

Right the situation gets tricky with hierarchies and such. I see some options, but I wouldn't even mind outliner specific operators here, for as long as these are just thin wrappers calling into BKE code or such. My main concern is to get data-management code (even things like user counting) out of the Outliner, which should be "just a UI".

For other cases like batch deleting objects, I see no reason to keep this in the Outliner code at all. Broadcasting via context would be simple here and not require further abstractions.

Letting the elements fill the menu is unclear to me, not sure how that works when you have multiple selected elements, mixed types. There might be some logic there that's much more easily expressed in a central function to fill the menu. Maybe it's not an issue, but I vaguely remember some logic that would prioritize e.g. objects over other data types in mixed selections. If it can be made to work then the extensibility is nice though.

I think this used to be the case, but for a while we've only been building the context menu from the active element now. In fact I've just cleaned up get_element_operation_type(), which seems to have been designed to support this, but wasn't used that way anymore (which I checked carefully): 8f6a38ed7e.

> In #101007#1415770, @brecht wrote: > The purpose of context is that operators can run in different editors. If the information is outliner specific (hierarchy etc), it's an unnecessary layer of abstraction only adding complexity, and I would just let operators access outliner data structures directly. Right the situation gets tricky with hierarchies and such. I see some options, but I wouldn't even mind outliner specific operators here, for as long as these are just thin wrappers calling into BKE code or such. My main concern is to get data-management code (even things like user counting) out of the Outliner, which should be "just a UI". For other cases like batch deleting objects, I see no reason to keep this in the Outliner code at all. Broadcasting via context would be simple here and not require further abstractions. > Letting the elements fill the menu is unclear to me, not sure how that works when you have multiple selected elements, mixed types. There might be some logic there that's much more easily expressed in a central function to fill the menu. Maybe it's not an issue, but I vaguely remember some logic that would prioritize e.g. objects over other data types in mixed selections. If it can be made to work then the extensibility is nice though. I think this used to be the case, but for a while we've only been building the context menu from the active element now. In fact I've just cleaned up `get_element_operation_type()`, which seems to have been designed to support this, but wasn't used that way anymore (which I checked carefully): 8f6a38ed7e.

Duarte Farrajota Ramos commented

2022-09-12 16:21:20 +02:00

Added subscriber: @DuarteRamos

Julian Eisel commented

2022-09-12 16:35:07 +02:00

Right the situation gets tricky with hierarchies and such.

Just to note, for library overrides it was/is quite a problem that only the Outliner has a good enough understanding of the necessary relations. I think it's worth thinking about ways to better define scene hierarchies throughout in Blender, for a whole bunch of reasons. Of course this is a bigger discussion, just noting as another way this could go. All in all I'm not too concerned, I'm glad about any gradual improvement here, and I wasn't planning to spend much time on these cases.

> Right the situation gets tricky with hierarchies and such. Just to note, for library overrides it was/is quite a problem that only the Outliner has a good enough understanding of the necessary relations. I think it's worth thinking about ways to better define scene hierarchies throughout in Blender, for a whole bunch of reasons. Of course this is a bigger discussion, just noting as another way this could go. All in all I'm not too concerned, I'm glad about any gradual improvement here, and I wasn't planning to spend much time on these cases.

Duarte Farrajota Ramos commented

2022-09-12 17:06:43 +02:00

Would the "operatorification" of the outliner menus finally open up the possibility of more things working on selection rather than active object?

I'm thinking of things like (visibility or selectability) toggles working on all selection rather than just clicked item, or the possibility of keyframing toggles renderability toggles for multiple objects.

Would the "operatorification" of the outliner menus finally open up the possibility of more things working on selection rather than active object? I'm thinking of things like (visibility or selectability) toggles working on all selection rather than just clicked item, or the possibility of keyframing toggles renderability toggles for multiple objects.

Philipp Oeser removed the

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Outliner: New context menu building design #101007