Page MenuHome

Copy a single modifier to all selected objects.
ClosedPublic

Authored by Erik Abrahamsson (erik85) on Nov 12 2020, 3:18 AM.

Details

Summary

These operators will copy a single modifier from the active object to all selected objects. This should work right now for all object types and modifiers, including particles, soft body and so on.

Diff Detail

Repository
rB Blender

Event Timeline

Erik Abrahamsson (erik85) requested review of this revision.Nov 12 2020, 3:18 AM
Erik Abrahamsson (erik85) created this revision.
Hans Goudey (HooglyBoogly) requested changes to this revision.Nov 12 2020, 11:14 PM

I like this idea! Just a few comments, but this looks pretty good. I didn't test it though.

source/blender/editors/object/object_modifier.c
1630

Pointcloud and volume objects also support modifiers.

There should also be a warning in this case of unsupported object types selected. (BKE_reportf)

1647

I would rather see the "special handling" in this patch, or at least already posted as a follow-up patch before continuing the review here.

1674

I would add "from the active object" here to make that more clear.

This revision now requires changes to proceed.Nov 12 2020, 11:14 PM
Erik Abrahamsson (erik85) marked 3 inline comments as done.
Erik Abrahamsson (erik85) edited the summary of this revision. (Show Details)

The patch grew quite a bit, but I think everything should be covered now.

Getting there! Some of these comments apply to both types of modifiers (I wish gpencil modifiers were just modifiers, oh well..), I trust you'll apply the changes in both places if they apply : )

source/blender/editors/object/object_modifier.c
1665

Maybe this would be more useful as a warning message:

"Modifier type cannot be added to object '%s'" -> "Object '%s' does not support %s modifiers", ob->id.name + 2, BKE_modifier_get_info(md->type)->name

Maybe that suggestion is a little too conversational, but something like that would be nice.

1674

Nitpicky: I think "Copy the modifier from the active object to all selected objects" reads a bit better.

1684–1686

Besides LISTBASE_FOREACH I would suggest reorganizing this code a bit, mostly to make the extra checks for these modifier types more separate.

It's a bit easier to just put a diff here if you don't mind:

diff --git a/source/blender/editors/object/object_modifier.c b/source/blender/editors/object/object_modifier.c
index 9cab829b9bc..90ca3828ae0 100644
--- a/source/blender/editors/object/object_modifier.c
+++ b/source/blender/editors/object/object_modifier.c
@@ -1681,27 +1681,24 @@ static int modifier_copy_to_selected_exec(bContext *C, wmOperator *op)
       ParticleSystemModifierData *psmd = (ParticleSystemModifierData *)md;
       object_copy_particle_system(bmain, scene, ob, psmd->psys);
     }
-    else if (ELEM(md->type, eModifierType_DynamicPaint, eModifierType_Fluid)) {
-      DynamicPaintModifierData *pmd = (DynamicPaintModifierData *)md;
-      FluidModifierData *fmd = (FluidModifierData *)md;
-
+    else {
       BKE_object_copy_modifier(ob, obact, md);
+    }
 
+    if (ELEM(md->type, eModifierType_DynamicPaint, eModifierType_Fluid)) {
       ParticleSystem *psys_on_modifier = NULL;
 
       if (md->type == eModifierType_DynamicPaint) {
-        if (pmd->brush) {
-          if (pmd->brush->psys) {
-            psys_on_modifier = pmd->brush->psys;
-          }
+        DynamicPaintModifierData *pmd = (DynamicPaintModifierData *)md;
+        if (pmd->brush && pmd->brush->psys) {
+          psys_on_modifier = pmd->brush->psys;
         }
       }
       else if (md->type == eModifierType_Fluid) {
+        FluidModifierData *fmd = (FluidModifierData *)md;
         if (fmd->type == MOD_FLUID_TYPE_FLOW) {
-          if (fmd->flow) {
-            if (fmd->flow->psys) {
-              psys_on_modifier = fmd->flow->psys;
-            }
+          if (fmd->flow && fmd->flow->psys) {
+            psys_on_modifier = fmd->flow->psys;
           }
         }
       }
@@ -1749,9 +1746,6 @@ static int modifier_copy_to_selected_exec(bContext *C, wmOperator *op)
         }
       }
     }
-    else {
-      BKE_object_copy_modifier(ob, obact, md);
-    }
 
     WM_event_add_notifier(C, NC_OBJECT | ND_MODIFIER, ob);
     DEG_id_tag_update(&ob->id, ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY | ID_RECALC_ANIMATION);

I actually think this whole section of moving the particle systems should probably be split into a separate function.
And adding a comment to that function about the big picture of why this special case is necessary would be even better

1710

I'd like to avoid using needlessly terse names like npsys, just expand it something like psys_new

1715

Use LISTBASE_FOREACH to iterate over ListBases, it's much more readable and enforces smaller scope for variables

source/blender/gpencil_modifiers/intern/MOD_gpencil_ui_common.c
274

This should be title case (Copy to Selected)

275

I know the existing menu already does this, but I think the double icon here is a bit ugly. Personally I would skip the icon for this operator.

279
/home/hans/Documents/Blender-Git/blender/source/blender/modifiers/intern/MOD_ui_common.c:266:32: warning: declaration of ‘ob’ shadows a previous local [-Wshadow]
  266 |   CTX_DATA_BEGIN (C, Object *, ob, selected_objects) {
      |

I suggest using ob_iter for this name, same for gpencil modifiers.

source/blender/modifiers/intern/MOD_ui_common.c
267–268

Since this is the third time I'm seeing this check in this patch and it seems like a reasonable check to have in BKE, I would add this as BKE_object_supports_modifiers() to object.c

269

Instead of counting the entire total just to check if it's greater than one, just use a bool and break if one of the selected objects doesn't equal the active object.

Hans Goudey (HooglyBoogly) requested changes to this revision.Nov 13 2020, 5:01 AM
This revision now requires changes to proceed.Nov 13 2020, 5:01 AM
Erik Abrahamsson (erik85) marked 10 inline comments as done.

Realized I forgot to change to BKE_object_supports_modifiers in one place.

The bigger notes are about the poll functions and the particle system copying.

source/blender/blenkernel/intern/object.c
1276

Just because it's not super clear from the name, I would suggest adding a comment:

/**
 * \return True if the object's type supports regular modifiers (not grease pencil modifiers).
 */
source/blender/editors/object/object_gpencil_modifier.c
846

This should be RPT_ERROR, because the operator doesn't work in this case.

source/blender/editors/object/object_modifier.c
1620

If the modifier uses particles

1622

Okay, I did some testing with the Fluid modifier that left me a bit confused about the goal of this function. I copied a liquid fluid modifier with particle settings to another object. The modifier was copied, but the fluid didn't move when I started playback, and the particle system wasn't copied to the new object. Either way, a description of the situations this is supposed to handle and some thorough testing would probably be best.

1640–1642

This is checked by the caller with

if (ELEM(md->type, eModifierType_DynamicPaint, eModifierType_Fluid)) {
  copy_or_reuse_particle_system(C, ob, md);
}

So instead of the previous else if I would recommend using if and BLI_assert(md->type == eModifierType_Fluid

1644

Check psys_on_modifier == NULL here and return early, that way the rest of the function loses one level of indentation.

1703

RPT_ERROR here too.

1780

See the note in MOD_ui_common.c about the poll function, which I don' think makes sense in this case, since the modifier is not edited.

source/blender/modifiers/intern/MOD_ui_common.c
265–278

This should become the poll function for this operator (same for the grease pencil modifiers). The poll currently checks if the modifier itself is editable, but the operator doesn't actually edit the modifier, it copies it.

267

Using BKE_object_support_modifier_type_check would be better, that way if you're in the menu of a bevel modifier and have 4 other volume objects selected, it will be disabled.

Hans Goudey (HooglyBoogly) requested changes to this revision.Nov 20 2020, 10:54 PM
This revision now requires changes to proceed.Nov 20 2020, 10:54 PM

Okay, I did some testing with the Fluid modifier that left me a bit confused about the goal of this function.

I'm constantly confused by the Fluid system...
Was it a Domain you copied, or a Flow-type?
I found an error in that function that could be related to that maybe, row 1680 it says (FluidModifierData *)md but should be (FluidModifierData *)new_md which made the code update the flow particle system to the one on the original object.
I've had problems with the fluids where I need to change the resolution in the domain and then change it back in order for it to update the particles.

Domains are a bit interesting. Maybe the easiest would be to just skip copying the particle systems of those (like now). It seems like the particle systems are generated but not tied to the modifier through a pointer in some way?
I wonder if that generation could be triggered or if I should just skip that.

There's also something strange going on with animated modifiers. If I copy a Warp modifier to it, it's not moving when I change frame. But if I create the Warp-modifier on the object it's working as it should. It also works if I after copying create a new modifier and just delete it. Must be some update issue going on there.
edit: Solved by calling DEG_relations_tag_update(bmain);

Erik Abrahamsson (erik85) updated this revision to Diff 31278.EditedNov 22 2020, 4:49 AM
Erik Abrahamsson (erik85) marked 8 inline comments as done.

Created poll functions and moved all checks on the source object there.
Now it should work fine to call the operator from Python too.

Erik Abrahamsson (erik85) marked 2 inline comments as done.

Updated to current master so should compile now.

Hans Goudey (HooglyBoogly) requested changes to this revision.Dec 18 2020, 12:23 AM

I tested the copying of particle settings with the function we discussed earlier. First, I tried to check this with dynamic paint. Even though I've used it in the past though, I find the whole thing pretty confusing. My sense is that copying the particle system here is not so important though. Second I tested the fluid modifier, and I'm not convinced copying the particle system is necessary here either. I doubt there's much of a use case for copying the domain modifier to multiple objects, and the liquid fluid domain is the only one that uses particles if I'm not mistaken. Anyway, the particles are generated data right, so it should be fine not to copy them.

So, all that said, I guess I would prefer not to do the special handling for particle systems in this patch. I'm sorry I pushed you to do it earlier, thanks for working on it, but it just doesn't feel worth the risk. I'm curious about your thoughts here though.

Apart from that, the patch looks great. I've heard other devs sound a little iffy on iterating through the selected objects in a poll function. Worst case here the performance might be pretty bad, but I expect the average case is totally fine, since it stops the loop if it finds any supported object. Plus, this operator is only exposed in a menu, so the poll function will only run when that's open.

source/blender/editors/object/object_modifier.c
1818

md->name

This revision now requires changes to proceed.Dec 18 2020, 12:23 AM
Erik Abrahamsson (erik85) marked an inline comment as done.

Sets the new modifier active on destination objects.

After some discussion and testing copying the particle systems is working well. They are copied when they're the source for fluid emitter modifiers and dynamic paint modifiers. They aren't copied for fluid domain objects, which wouldn't make any sense anyway (that's what I was concerned about above.

@Brecht Van Lommel (brecht) Since this is a pretty straightforward self contained feature, I'm hoping it would be fine to commit in Bcon2?

This revision is now accepted and ready to land.Dec 20 2020, 6:11 PM

It's fine to add this functionality in bcon2.