Page MenuHome

Refactor modifier copying code.
ClosedPublic

Authored by Bastien Montagne (mont29) on Jan 19 2021, 4:34 PM.

Details

Summary

Things like pointers to particle systems, or softbody data being stored
outside of its modifier, make it impossible for internal modifier copy
data code to be self-contained currently. It requires extra processing.

In existing code this was handled in several different places, in
several ways, and alltogether fairly inconsistently. Some cases were
even not properly handled, causing e.g. crashes as in T82945.

This commit addresses those issues by:

  • Adding comments about the hackish/unsafe parts psys implies when copying some modifier data (since we need to ensure particle system copying and remapping of those pointers separately).
  • Adding as-best-as-possible handling of those cases to BKE_object_copy_modifier (note that it remains fragile, but is expected to behave 'good enough' in any practical usecase).
  • Remove special handling for specific editor code (copy_or_reuse_particle_system). This should never have been accepted in ED code area, and is now handled by BKE_object_copy_modifier.
  • Factorize copying of the whole modifier stack into new BKE_object_modifier_stack_copy, now used by both object_copy_data and BKE_object_link_modifiers.

Note that this implies that BKE_object_copy_modifier and
BKE_object_copy_gpencil_modifier are now to be used exclusively to
copy single modifiers. Full modifier stack copy should always use
BKE_object_modifier_stack_copy instead.

Fix T82945: Crash when dragging modifiers in Outliner.

Diff Detail

Repository
rB Blender

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Jan 19 2021, 4:34 PM

The use of FIXME in the comments is a bit confusing to me, since they don't make any suggestion of a way to fix the issue. I guess in the case of particle systems the solution is a better design for the replacement of particle systems, but not sure about the others.

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

typo synamic -> dynamic

1394

We never allow to copy -> We never allow copying

Bastien Montagne (mont29) marked 2 inline comments as done.Jan 19 2021, 5:59 PM

The use of FIXME in the comments is a bit confusing to me, since they don't make any suggestion of a way to fix the issue. I guess in the case of particle systems the solution is a better design for the replacement of particle systems, but not sure about the others.

FIXME means that there is something to be fixed, it does not have to explain how to fix it (otherwise just fix it already xD ). Indeed here this is job for particles refactor.

Fix typos in comments.

Implementation looks fine, but agree about the FIXME comments. Personally would just use NOTE.

This revision is now accepted and ready to land.Jan 19 2021, 6:32 PM

Implementation looks fine, but agree about the FIXME comments. Personally would just use NOTE.

Fine by me, will change then. Thanks for the reviews.

This revision was automatically updated to reflect the committed changes.