Page MenuHome
Differential D2309

Add global/local option for direction parameter in Displace modifier.
ClosedPublic
Actions

Authored by Quentin Wenger (Matpi) on Oct 18 2016, 4:36 PM.

Details

Reviewers
Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)
Commits
rBSe0a34e963fff: Displace modifier: add global/local space option for X/Y/Z/XYZ directions.
rBe0a34e963fff: Displace modifier: add global/local space option for X/Y/Z/XYZ directions.
Summary

Displace modifier along global axes

The aim of this patch is to provide an option for applying the Displace modifier along global XYZ axes.
This has been asked for by a user.

I am not sure if it is a good idea to rename the already existing X/Y/Z axes to LOCAL_X/LOCAL_Y/LOCAL_Z. It could particularly break existing scripts. Alternatively, we could:

  • simply name the new ones GLOBAL_[X/Y/Z] without touching the old ones
  • add a toggle LOCAL/GLOBAL instead, which would be shown only if direction is set to X/Y/Z.

Note that it only works with the new Depsgraph activated.

Diff Detail

Repository
rB Blender

Event Timeline

Quentin Wenger (Matpi) updated this revision to Diff 7641.Oct 18 2016, 4:36 PM
Quentin Wenger (Matpi) retitled this revision from to Add global/local option for direction parameter in Displace modifier..
Quentin Wenger (Matpi) updated this object.
Quentin Wenger (Matpi) added reviewers: Sergey Sharybin (sergey), Brecht Van Lommel (brecht).
Quentin Wenger (Matpi) updated this revision to Diff 7642.Oct 18 2016, 4:39 PM
  • Clean spaces up.
Quentin Wenger (Matpi) added a project: Modifiers.Oct 18 2016, 4:40 PM
Brecht Van Lommel (brecht) edited edge metadata.Oct 20 2016, 12:50 AM

I suggest to have a Local/Global toggle, which then could also work for the RGB to XYZ mode.

I'm not sure why this would only work with the new depsgraph? For both I expect extending the if (dmd->texmapping == MOD_DISP_MAP_GLOBAL) test in the depsgraph update methods to work.

Brecht Van Lommel (brecht) requested changes to this revision.Oct 20 2016, 12:50 AM
Brecht Van Lommel (brecht) edited edge metadata.
This revision now requires changes to proceed.Oct 20 2016, 12:50 AM
Quentin Wenger (Matpi) updated this revision to Diff 7661.Oct 21 2016, 12:39 AM
Quentin Wenger (Matpi) edited edge metadata.
  • Transform LOCAL/GLOBAL option into toggle.
Quentin Wenger (Matpi) updated this revision to Diff 7662.Oct 21 2016, 12:43 AM
Quentin Wenger (Matpi) edited edge metadata.
  • Clean tabs up.
Quentin Wenger (Matpi) updated this revision to Diff 7663.Oct 21 2016, 1:40 AM
  • Use API math functions.
Quentin Wenger (Matpi) added a comment.Oct 21 2016, 1:44 AM

Some quick comments/thoughts:

  1. UI: I somehow tried to mimic the style used in Motion actuators, for example. I'd be happy to know if you come up with a better design. (in particular I would have liked to have the prop stay size-fixed as in "F" (fake user) buttons; but this seems not to be possible via Python layouting)
  1. Not sure if my multiple-line if's do follow the code styling conventions on left alignment.
  1. @Brecht Van Lommel (brecht): as you suggested I also added the functionality for RGB-to-XYZ. In my latest revision I switched to math functions rather than "manual" computation. Is this a better choice?
  1. Concerning the Depsgraph, what I wanted to say is that, in the original state of this patch, only the new depsgraph did update as needed. But this was of course before adding the relevant code into updateDep(s)graph functions (which, TBH, I discovered inbetween :-) ). So, no problem in this area.
Brecht Van Lommel (brecht) requested changes to this revision.Oct 23 2016, 1:53 AM
Brecht Van Lommel (brecht) edited edge metadata.
In D2309#53541, @Quentin Wenger (Matpi) wrote:
  1. @Brecht Van Lommel (brecht): as you suggested I also added the functionality for RGB-to-XYZ. In my latest revision I switched to math functions rather than "manual" computation. Is this a better choice?

Either is fine with me here, depends a bit on the specific situation which one is easier to read.

source/blender/makesrna/intern/rna_modifier.c
2139–2142

I would make this a "space" enum, and have "Local" and "Global" items. No need for a cryptic "L" label.

source/blender/modifiers/intern/MOD_displace.c
195–200

This style follows convention I think, would use the same for the if() above.

291

Convention is:

}
else {
This revision now requires changes to proceed.Oct 23 2016, 1:53 AM
Quentin Wenger (Matpi) updated this revision to Diff 7682.Oct 23 2016, 1:22 PM
Quentin Wenger (Matpi) edited edge metadata.
  • Style
  • Transform boolean direction space into enum.

Not sure if the UI placing is optimal.

Brecht Van Lommel (brecht) accepted this revision.Oct 23 2016, 2:22 PM
Brecht Van Lommel (brecht) edited edge metadata.

Looks good to me!

This revision is now accepted and ready to land.Oct 23 2016, 2:22 PM
Brecht Van Lommel (brecht) added inline comments.Oct 23 2016, 2:36 PM
source/blender/modifiers/intern/MOD_displace.c
259

Actually I think this is wrong, it should just use ob->obmat like other modifiers. What this function does is remove parenting from the matrix, which normally you only need to do in specific cases, like exporting to a file format where parenting is accounted for in another way.

I'll fix that as part of committing.

Closed by commit rBe0a34e963fff: Displace modifier: add global/local space option for X/Y/Z/XYZ directions. (authored by Quentin Wenger (Matpi), committed by Brecht Van Lommel (brecht)). · Explain WhyOct 23 2016, 2:46 PM
This revision was automatically updated to reflect the committed changes.
Quentin Wenger (Matpi) added a comment.Oct 23 2016, 4:57 PM

Thanks for the clarification and for committing, @Brecht Van Lommel (brecht)!

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
3 lines
source/
blender/
makesdna/
8 lines
makesrna/
intern/
11 lines
modifiers/
intern/
60 lines

Diff 7684

View Options

release/scripts/startup/bl_ui/properties_data_modifier.py

Loading...
View Options

source/blender/makesdna/DNA_modifier_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_modifier.c

Loading...
View Options

source/blender/modifiers/intern/MOD_displace.c

Loading...