Page MenuHome

Outliner: Data stack drag and drop operator
ClosedPublic

Authored by Nathan Craddock (Zachman) on Aug 19 2020, 10:58 PM.

Details

Summary

Add a generic operator for modifier, constraint, and shader effect drag
and drop in the outliner.

The following operations are allowed:

  • Reordering within an object.
  • Copying a single modifier/constraint/effect to an other object.
  • Copying (linking) an object's modifiers/constraints/effects to another.

This is useful/needed for a number of reasons. (using modifier to represent all three types of data)

  • We show modifiers, constraints, and shader effects in the outliner with minimal useful behaviors. You can see at a glance the modifiers on an object/bone, and can delete or disable it. Adding the ability to reorder in the outliner gives another purpose for showing this data.
  • AFAIK it's currently impossible to copy a single modifier between objects. Using the outliner for this is intuitive.
  • This also applies to linking. This can be done through the object menus, but it's also nice to use the outliner to easily link all modifiers to another object.

Diff Detail

Repository
rB Blender

Event Timeline

Nathan Craddock (Zachman) requested review of this revision.Aug 19 2020, 10:58 PM
Nathan Craddock (Zachman) created this revision.
Nathan Craddock (Zachman) retitled this revision from Outliner: Add UI stack drag and drop operator to Outliner: UI stack drag and drop operator.
Nathan Craddock (Zachman) edited the summary of this revision. (Show Details)
Julian Eisel (Severin) requested changes to this revision.Aug 26 2020, 12:10 AM

First pass, things look mostly fine. A few smaller tweaks to be done here and there, but no bigger issue.
One oversight though: It should not be possible to add modifiers to linked object.

source/blender/editors/space_outliner/outliner_dragdrop.c
96 ↗(On Diff #27939)

Minor suggestion: I'd suggest using sizeof(*drop_data) here, which means the type will always follows the one of the variable.

270–279 ↗(On Diff #27939)

This loop is repeated 3 times, with only minor differences. You could write a utility function for this.
I also don't see why this is using a while loop.

848 ↗(On Diff #27939)

Use a switch here, so it's obvious on first sight that this logic only handles the different types (there could be other checks in the `else if'-blocks below, you have to read them to see if that's the case).

849–851 ↗(On Diff #27939)

I'd add these function calls to ED_object_link_modifiers(). Same for the ones below.

896 ↗(On Diff #27939)

Same here, better use a switch.

924–927 ↗(On Diff #27939)

Why is this lower-level logic here? I'd expect there would be functions for this, likely on ED_ level.

1012 ↗(On Diff #27939)

I find UI stack a bit misleading, since were dealing with scene data, not UI. Maybe call it data-stack then? Not too discriptive either, but at least not misleading I think.

This revision now requires changes to proceed.Aug 26 2020, 12:10 AM
Nathan Craddock (Zachman) marked 4 inline comments as done.
  • Cleanup: Use switches rather than if-else branches
  • Cleanup: Remove compile warning
  • Cleanup: Move code to function
  • Fix: Prevent dropping on linked objects

When moving code to ED_* functions, should I follow the naming conventions there? For example, you suggested ED_object_link_modifiers() but wouldn't ED_object_modifier_link() be more consistent with the other names?

I fixed dropping on linked objects. Should the same be prevented for bone constrains in linked Armatures? I'm not familiar enough with the linking workflow.

Nathan Craddock (Zachman) marked an inline comment as done.
  • Cleanup: Rename UI Stack to Data Stack
Nathan Craddock (Zachman) retitled this revision from Outliner: UI stack drag and drop operator to Outliner: Data stack drag and drop operator.Aug 27 2020, 6:10 AM
Nathan Craddock (Zachman) edited the summary of this revision. (Show Details)
Nathan Craddock (Zachman) marked an inline comment as done.
  • Cleanup: Move shaderfx copy in to function
Julian Eisel (Severin) requested changes to this revision.Aug 27 2020, 4:39 PM

When moving code to ED_* functions, should I follow the naming conventions there? For example, you suggested ED_object_link_modifiers() but wouldn't ED_object_modifier_link() be more consistent with the other names?

Ah yeah, I was just mixing up some functions in my mind there. It should be ED_object_link_modifiers() I guess (note the plural).

I fixed dropping on linked objects. Should the same be prevented for bone constrains in linked Armatures? I'm not familiar enough with the linking workflow.

Since the armature has an object as parent, the check you added should cover this. There might still be a way to drag a constraint to a linked armature somehow that is not from a linked object, although I didn't manage to do that.
But to play save, you could use the TreeStoreElem.id for the ID_IS_LINKED() check, rather than just the object if there is one. Of course you have to check if the pointer is actually a real ID one.

source/blender/editors/space_outliner/outliner_dragdrop.c
726–729 ↗(On Diff #28224)

This function does much more than polling, it actually specifies the operation and input data for the dropping. I'd rather avoid that.

Could this logic be moved into an own function to determine operation and input data? The poll could then use that to check if the drop is valid, and set the TSE_DRAG_ flag. The drop-invoke could call it again to retrieve the needed data for the actual operation.
Executing this logic twice is not great, but a totally acceptable trade-off.

Note that collection_drop_init() does the same actually.

874–878 ↗(On Diff #28224)

What I meant with my earlier comment is that I think all these things should be handled in ED_ functions. The Outliner shouldn't know which notifiers have to be sent or which depsgraph tags have to be set if an operation is executed. The ED code should do that.
Same goes for the cases below of course. Even if that differs from what the Outliner code does in other cases.

Note that you don't need the context for that, you can call WM_main_add_notifier(). The context is only needed to get the window for specific screen-level handling.

This revision now requires changes to proceed.Aug 27 2020, 4:39 PM
  • Move functions to ED_*
  • Cleanup: Rename link to copy for modifiers
  • Move more code to ED functions
Nathan Craddock (Zachman) marked an inline comment as done.
  • Cleanup: Split up poll function
  • Ensure grease pencil data isn't copied to other objects

I believe I have addressed all the issues you mentioned except for a few I have questions on:

  1. Moving code to ED_ functions. I have finished doing this except for reordering data. For the reordering logic, ED_ functions already exist now because of the recent work on modifier drag&drop. These functions don't call the notifiers which is why I do the notifiers in the outliner code. For example, ED_object_shaderfx_move_to_index. In these cases it seems strange to wrap an ED function in an identical function, just with notifiers. What's the recommended approach here?
  1. Cleanup of outliner_collection_from_tree_element_and_parents and similar functions. You mentioned creating a utility function to replace the 3 similar functions here. I agree that would be great. I admittedly haven't given it a ton of thought, but my best idea here requires passing in a function for identifying the desired data. So we would still need 3 functions. Is that amount of complexity preferable to the three similar functions? Perhaps there is an easier solution, I'll keep looking.
  1. Data validation I added some more validation code to prevent adding grease pencil modifiers to meshes and other similar cases. One I haven't handled yet is preventing copying the two IK constraints from a bone to an object. Again, I'll see if I can find a reasonable place to put this check, but I would appreciate some input.
  • Move constraint copy to ED

I believe I have addressed all the issues you mentioned except for a few I have questions on:

  1. Moving code to ED_ functions. I have finished doing this except for reordering data. For the reordering logic, ED_ functions already exist now because of the recent work on modifier drag&drop. These functions don't call the notifiers which is why I do the notifiers in the outliner code. For example, ED_object_shaderfx_move_to_index. In these cases it seems strange to wrap an ED function in an identical function, just with notifiers. What's the recommended approach here?

I'd just move the DEG and notifier calls to the ED_ functions. Editor level functions are high-level, I think it's reasonable to expect them to do the necessary update tagging for the operations they just did.

  1. Cleanup of outliner_collection_from_tree_element_and_parents and similar functions. You mentioned creating a utility function to replace the 3 similar functions here. I agree that would be great. I admittedly haven't given it a ton of thought, but my best idea here requires passing in a function for identifying the desired data. So we would still need 3 functions. Is that amount of complexity preferable to the three similar functions? Perhaps there is an easier solution, I'll keep looking.

There are a few ways we could do this, but I think just passing a callback that checks if an item meets the criteria is easiest to understand. I don't mind much having three small functions, but having almost the same iterator logic duplicated multiple times is nice to avoid.

  1. Data validation I added some more validation code to prevent adding grease pencil modifiers to meshes and other similar cases. One I haven't handled yet is preventing copying the two IK constraints from a bone to an object. Again, I'll see if I can find a reasonable place to put this check, but I would appreciate some input.

I would simply disallow dragging bone constraints to objects. Only allow dragging object constraints to objects, and bone constraints to bones. Even if they are of the same type, you don't know if a constraint changes internal logic based on if it's added to an object or to a bone. And if then a bone constraint is copied to an object or vise versa, things might break.

source/blender/editors/space_outliner/outliner_dragdrop.c
697 ↗(On Diff #28296)

I'd call this pchan_parent, otherwise it's confusing/misleading since it's not actually the bone.

749 ↗(On Diff #28296)

Same here, would suggest pchan_te?

Julian Eisel (Severin) requested changes to this revision.Thu, Sep 10, 4:47 PM
This revision now requires changes to proceed.Thu, Sep 10, 4:47 PM
Nathan Craddock (Zachman) marked 2 inline comments as done.
  • Cleanup: Use pchan rather than bone in names
  • Cleanup: Combine parent element search utility functions
  • Prevent data from being copied between objects and bones
  • Move notifiers and DEG calls to ED functions for reorder
  • Rebase on master
Nathan Craddock (Zachman) marked 2 inline comments as done.Tue, Sep 15, 7:38 PM
Julian Eisel (Severin) added inline comments.
source/blender/editors/space_outliner/outliner_dragdrop.c
193 ↗(On Diff #28894)

Couldn't you just use a non-NULL return value as condition? Don't see a need for both, the bool and the argument return value.

This revision is now accepted and ready to land.Tue, Sep 15, 8:39 PM
Nathan Craddock (Zachman) marked an inline comment as done.
  • Cleanup parent element check function
  • Don't use void pointers for parent data