Page MenuHome

T47435: Adds deformation axis selection to the Simple Deform modifier
ClosedPublic

Authored by Dan Marra (dmarra) on Jan 5 2018, 3:26 AM.

Details

Summary

Adds an option for the user to select the axis of deformation on the simple deform modifier. It also adds an additional lock Z axis option.

This change is backward compatible with old saves. Files saved in versions 279 subversion 0 and prior will have their defaults set to Z (X for bend mode) to mimic the previous behavior of the modifier.

A demonstration of the changes can be viewed here:

https://youtu.be/IGIqFxBWf0s

Diff Detail

Repository
rB Blender
Branch
arcpatch-D2989_2
Build Status
Buildable 1065
Build 1065: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested changes to this revision.Jan 5 2018, 8:17 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenloader/intern/versioning_270.c
1742–1743

No need to check for pad

source/blender/makesdna/DNA_modifier_types.h
870–872

This is going to cause issues with existing files, since the meaning of axis is changed.

Existing variables shouldn't be renamed unless do-versions handles this.

source/blender/modifiers/intern/MOD_simpledeform.c
68–81

Since this is being called on every vertex, it'd be better to have a static array defined for all functions to read from:

static const axis_map[3][3] = {{...}};

For ease of access this can be assigned to idx within the function.

151–187

Avoid adding trailing space.

This revision now requires changes to proceed.Jan 5 2018, 8:17 AM

All requested changes have been made.

Originally I was testing for "pad" and "lock_axis" because the names had changed, and this was my way to determine that, but this update keeps the old struct members intact. I am curious though if there is a better way to handle the original change in do_versions(), such that we do not need the unnecessary padding.

Am not sure using idx for every index access is worth the hassle, I think it makes the code needlessly complicated.

Why not re-order the location before calling simpleDeform_callback and reverse this operation after?

This means we can keep deform logic less complicated & error prone.

source/blender/makesdna/DNA_modifier_types.h
872–873

pad and pad2 are just to keep the struct aligned, both can be removed now.

source/blender/modifiers/intern/MOD_simpledeform.c
159–174

dcut variable was used before and now isn't.

Won't this change default behavior?

Dan Marra (dmarra) added a comment.EditedJan 6 2018, 4:04 PM

Sure I can do the reordering before and after the simpleDeform_callback gets called; sounds like a good idea.

As far as dcut is concerned for bend, at first glance I thought dcut served no purpose for the bend modifier, but it seems I missed a line of code that demonstrates that is DOES serve a purpose. I just compared my build against release, and they are indeed different. I'll be fixing this as well.

  • pad and pad2 have been removed.
  • idx has been deprecated in favor of mapping/remapping before/after calling the callback
  • dcut has been reintroduced for bend mode.

I noticed while re-adding the correct limit behavior to bend mode that it is a little wonky. It matches the current release behavior, but the behavior itself is a bit off. When you set the upper limit, it causes a rotation of the object that I don't think is desirable. I think addressing that is out of scope of this ticket, but it is something to consider for a separate one.

Thanks for the updates, looks good - will apply today.

This revision is now accepted and ready to land.Jan 7 2018, 10:54 PM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
  • Fix locking, more compact UI, use inline functions to remap