Page MenuHome

Outliner: Add properties editor sync on selection
ClosedPublic

Authored by Nathan Craddock (Zachman) on Aug 19 2020, 8:06 PM.
Tags
Tokens
"Love" token, awarded by gilberto_rodrigues."Love" token, awarded by chironamo."Love" token, awarded by Slowwkidd."Love" token, awarded by pablovazquez."Love" token, awarded by franMarz."Love" token, awarded by billreynish."Love" token, awarded by Geronimooo.

Details

Summary

When outliner datablocks are selected, switch the properties editors in the current workspace to the correct tab for that datablock.

Includes logic to ensure the tab doesn't switch to an unavailable tab when a datablock is pinned in a properties editor.

There is a heuristic to only change the properties editor tabs for editors bordering the outliner. This uses the same shared edge checking as the area join operator.

When clicking modifiers, constraints, and shader effects in the outliner, the corresponding panel will be opened in all properties editors.

Diff Detail

Repository
rB Blender

Event Timeline

Nathan Craddock (Zachman) requested review of this revision.Aug 19 2020, 8:06 PM
Nathan Craddock (Zachman) created this revision.

The code side looks mostly fine again. But I do see some design issues:

  • If we apply this we loose the ability to change modes in the Outliner. So we shouldn't commit this without D8641 as well. However there are open technical concerns raised about this (asked Campbell to note them in the differential).
  • I'm unsure if the design of changing all Properties in the current window will work well in practice. I would like to at least try out the alternative of trying to use connected editors only (ie with a shared edge).
  • This current design won't work for floating windows. So if you shift+drag the Properties editor corner to duplicate it into a new window and close the non-floating one, changing the Outliner selection won't change the floating Properties. Maybe you should iterate over all visible layouts of the current workspace until you found one with the properties visible?

Also, these are separate changes, which I don't minding reviewing together. But of course they should be committed separately.

source/blender/editors/space_buttons/buttons_context.c
754–756

The active space type is typically checked via ScrArea.spacetype.

source/blender/makesdna/DNA_outliner_types.h
112–113 ↗(On Diff #27933)

These are grease pencil only, right? In that case I'd prefer something like TSE_GPENCIL_EFFECT

I'm fine with having the GPencil modifier and shader effects committed to master directly (although there are a few tweaks I'd suggest, see the inline comments).
These changes are straight forward and make sense to me.

source/blender/editors/space_outliner/outliner_tree.c
565 ↗(On Diff #27933)

There's LISTBASE_FOREACH_INDEX now that you can use here.

  • Fix: Use proper method of finding active space type

Just in case you missed it-- looks like outliner_item_mode_toggle is unused with this patch applied.

Nathan Craddock (Zachman) marked an inline comment as done.

Rebase on master after committing gpencil modifier & shaders in the tree

Nathan Craddock (Zachman) marked an inline comment as done.Sep 20 2020, 1:06 AM
Nathan Craddock (Zachman) added a project: Restricted Project.
Nathan Craddock (Zachman) edited the summary of this revision. (Show Details)
  • Only update for shared edges
  • Rebase on master

I'm curious if this could still be added to 2.91. I know that bcon2 allows for smaller new features, and D8641: Outliner: Add mode toggling column was partially motivated by properties syncing. It feels incomplete to me to have moved mode toggling to the left column but leaving the datablocks without syncing in 2.91.

  • If we apply this we loose the ability to change modes in the Outliner. So we shouldn't commit this without D8641 as well. However there are open technical concerns raised about this (asked Campbell to note them in the differential).

No longer an issue :)

  • I'm unsure if the design of changing all Properties in the current window will work well in practice. I would like to at least try out the alternative of trying to use connected editors only (ie with a shared edge).

I just committed a fix to test this behavior. I think it works well. I'm using the same function that is use to determine if areas can be joined, exposed as an ED_area function.

  • This current design won't work for floating windows. So if you shift+drag the Properties editor corner to duplicate it into a new window and close the non-floating one, changing the Outliner selection won't change the floating Properties. Maybe you should iterate over all visible layouts of the current workspace until you found one with the properties visible?

Maybe I'm misunderstanding, but I see a bit of conflict between this and the previous suggestion. Does it make sense to sync floating properties editors when we do extra work to only sync bordering properties editors?

I recorded a short video showing the shared edges behavior.

Nathan Craddock (Zachman) edited the summary of this revision. (Show Details)

Looks quite good, simple and intuitive too : )

source/blender/editors/space_buttons/buttons_context.c
755–756

There's an odd combination of continue and indentation here.

I would suggest something like this:

LISTBASE_FOREACH (ScrArea *, area, &screen->areabase) {
  if (area->spacetype != SPACE_PROPERTIES) {
    continue;
  }
  if (!ED_area_has_shared_border(active_area, area)) {
    continue;
  }
  ...
source/blender/editors/space_outliner/outliner_select.c
1160–1161

Commented out code should be removed? Or inside an #if 0 with a comment for why it isn't used.

Nathan Craddock (Zachman) marked 2 inline comments as done.
  • Cleanup: Remove comment
  • Cleanup: Continue and indentation

Code is fine, just suggesting to minor tweaks.

source/blender/editors/space_outliner/outliner_select.c
1084

There are all these different calls to ED_buttons_set_context() and the only thing that changes is one parameter.

I'd just set a context for each case and at the end of he function do:

if (ptr.data) {
  ED_buttons_set_context(C, &ptr, context);
}

Note that you'll have to initialize ptr = {0} for that.

1150–1157

These low level operations bother me. Could we just have a BKE_modifier_show_ui() (and similar for the other cases)?

This revision is now accepted and ready to land.Wed, Oct 28, 4:37 PM
source/blender/editors/space_outliner/outliner_select.c
1150–1157

Note that it would look better with the UI_EXPAND_ROOT_PANEL you suggested in D7997.

Nathan Craddock (Zachman) marked an inline comment as done.Wed, Oct 28, 5:25 PM
Nathan Craddock (Zachman) added inline comments.
source/blender/editors/space_outliner/outliner_select.c
1150–1157

That sounds good to me. I think BKE_modifier_panel_expand() is more clear as a name

Nathan Craddock (Zachman) marked an inline comment as done.Wed, Oct 28, 5:55 PM