[Bf-codereview] WeightVG modifiers weight preview, and general weight preview enhancement (issue 5531047)

[montagne29 at wanadoo.fr]

Here is a new version of the patch.

I addressed most of your advices, with one exception: imho, setting
CD_WEIGHT_MCOL from the main modifier evaluation code (in DerivedMesh.c)
is not a good idea, and is not even doable, at least for DPaint VCol
preview. There would also be the problem of determining which vgroup’s
weights to preview (modifier one might be different from current active
one). Not to mention that a modifier might want to output something else
than just a single group’s weights (I already have on my hdd a new
weight modifier that uses a more complex preview option…).

So, I think the modifier should keep the responsibility of preview
compute, and hence be aware whether it is authorized to compute a given
type of preview (in WPaint mode, we want weight preview, but not VCol
preview!). In current code, I added two more temporary flags to
md->mode, but I’m quite aware this is not a good solution.

Which leads me to that proposition: I’d like to replace the
useRenderParam, useCache, useDeform, etc. params passed to the exec
modifier functions by a single flag one, which would allow much more
flexibility to main code to control each modifier’s behavior.

Obviously, this would be a huge change in modifiers code, to be done on
its own (and before applying that patch!). Not sure whether we want to
do that kind of things with bmesh narrowing to merge point?

As a side note, as suggested by miikha, we should also rename
CD_WEIGHT_MCOL to something like CD_PREVIEW_MCOL... another big patch,
but that can wait later.

(new patch following)


http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/BKE_DerivedMesh.h
File source/blender/blenkernel/BKE_DerivedMesh.h (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/BKE_DerivedMesh.h#newcode72
source/blender/blenkernel/BKE_DerivedMesh.h:72: #define DM_MOD_DO_WMCOL
(1 << 0)
On 2012/01/19 14:08:02, ideasman42 wrote:
> think this can be avoided, and mod stack detect this case without a
flag in the
> DM.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/BKE_modifier.h
File source/blender/blenkernel/BKE_modifier.h (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/BKE_modifier.h#newcode105
source/blender/blenkernel/BKE_modifier.h:105:
eModifierTypeFlag_UsesWMColPreview = (1<<9)
On 2012/01/19 16:19:15, ideasman42 wrote:
> Infact this should just be for any modifier which changes weights,
color is
> incidental.

Eh, no, in fact, it’s for any modifier generating some preview stuff in
CD_WEIGHT_MCOL...

Renamed it to just eModifierTypeFlag_UsesPreview

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/intern/DerivedMesh.c
File source/blender/blenkernel/intern/DerivedMesh.c (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/intern/DerivedMesh.c#newcode681
source/blender/blenkernel/intern/DerivedMesh.c:681: r_col[3] =
FTOCHAR(colf[0]);
On 2012/01/19 14:08:02, ideasman42 wrote:
> re FTOCHAR:
> This isn't needed, we can assume values are already clamped.
> existing code was fine.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/intern/DerivedMesh.c#newcode785
source/blender/blenkernel/intern/DerivedMesh.c:785: unsigned char
*wtcol_f = DM_get_face_data_layer(dm, CD_WEIGHT_MCOL);
On 2012/01/19 14:08:02, ideasman42 wrote:
> prefer dm->getVertDataArray(), for now its unlikley to make a real
difference.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/intern/dynamicpaint.c
File source/blender/blenkernel/intern/dynamicpaint.c (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/intern/dynamicpaint.c#newcode1728
source/blender/blenkernel/intern/dynamicpaint.c:1728: int index =
*((&mface[i].v1)+j);
On 2012/01/19 14:08:02, ideasman42 wrote:
> re: edits to the for loop and not doing quad/tri color edits.

> this is an improvement, but would prefer these be committed to trunk
directly.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/intern/modifier.c
File source/blender/blenkernel/intern/modifier.c (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/intern/modifier.c#newcode146
source/blender/blenkernel/intern/modifier.c:146: int
modifier_usesWMColPreview(ModifierData *md)
On 2012/01/19 14:08:02, ideasman42 wrote:
> with a generic flag on the modifier this function wont be needed. or
at least
> the type spesific parts wont.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/editors/space_view3d/view3d_header.c
File source/blender/editors/space_view3d/view3d_header.c (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/editors/space_view3d/view3d_header.c#newcode515
source/blender/editors/space_view3d/view3d_header.c:515: uiItemR(layout,
&toolsptr, "weights_preview_mode", UI_ITEM_R_ICON_ONLY|UI_ITEM_R_EXPAND,
On 2012/01/19 14:08:02, ideasman42 wrote:
> better solve global data layer preview options in a different patch.
uvs can use
> too but rather exclude from wight preview patch.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesdna/DNA_modifier_types.h
File source/blender/makesdna/DNA_modifier_types.h (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesdna/DNA_modifier_types.h#newcode881
source/blender/makesdna/DNA_modifier_types.h:881: int		common_flags;
On 2012/01/19 14:08:02, ideasman42 wrote:
> think this should be removed and the modifiers flag used.

> one of the flags like eModifierMode_OnCage

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesdna/DNA_modifier_types.h#newcode1025
source/blender/makesdna/DNA_modifier_types.h:1025: #define
MOD_WVG_CFLAG_WEIGHT_PREVIEW		(1 << 0)
On 2012/01/19 14:08:02, ideasman42 wrote:
> dont think this flag is needed, the modifier stack function can see if
modifier
> preview is used on any mod and set a var on the stack based on this.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesdna/DNA_scene_types.h
File source/blender/makesdna/DNA_scene_types.h (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesdna/DNA_scene_types.h#newcode911
source/blender/makesdna/DNA_scene_types.h:911: short uv_selectmode;
On 2012/01/19 14:08:02, ideasman42 wrote:
> please don't do tidy up edits in branch.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesdna/DNA_scene_types.h#newcode1547
source/blender/makesdna/DNA_scene_types.h:1547: /*
toolsettings->weights_preview */
On 2012/01/19 14:08:02, ideasman42 wrote:
> again, think this should be removed, and use operator option only.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesrna/intern/rna_modifier.c
File source/blender/makesrna/intern/rna_modifier.c (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesrna/intern/rna_modifier.c#newcode2603
source/blender/makesrna/intern/rna_modifier.c:2603: prop=
RNA_def_property(srna, "use_weight_preview", PROP_BOOLEAN, PROP_NONE);
On 2012/01/19 14:08:02, ideasman42 wrote:
> again, make generic option like cage.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesrna/intern/rna_scene.c
File source/blender/makesrna/intern/rna_scene.c (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesrna/intern/rna_scene.c#newcode1418
source/blender/makesrna/intern/rna_scene.c:1418: {WP_WPREVIEW_ORG,
"ORG", ICON_BRUSH_DATA, "Original", "View original weights, before any
modifier action"},
On 2012/01/19 15:55:55, brechtvl wrote:
> There's no reason to give this cryptic "ORG" name, but I also have
doubts if
> this is a good option to have.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/makesrna/intern/rna_scene.c#newcode1447
source/blender/makesrna/intern/rna_scene.c:1447: prop=
RNA_def_property(srna, "weights_preview_mode", PROP_ENUM, PROP_NONE);
On 2012/01/19 14:08:02, ideasman42 wrote:
> -1 for this option.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/modifiers/intern/MOD_weightvg_util.c
File source/blender/modifiers/intern/MOD_weightvg_util.c (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/modifiers/intern/MOD_weightvg_util.c#newcode268
source/blender/modifiers/intern/MOD_weightvg_util.c:268: void
weightvg_set_weightcol(DerivedMesh *dm, int num, const int *indices,
float *weights)
On 2012/01/19 14:08:02, ideasman42 wrote:
> Think this function can be merged with add_weight_mcol_dm()

> probably rename and expose add_weight_mcol_dm() as an api funk of
> BKE_derivedmesh.h

> any important differences can be args.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/modifiers/intern/MOD_weightvg_util.c#newcode275
source/blender/modifiers/intern/MOD_weightvg_util.c:275: MCol *col =
CustomData_duplicate_referenced_layer(&dm->faceData, CD_WEIGHT_MCOL,
nfaces);
On 2012/01/19 14:08:02, ideasman42 wrote:
> since this layer is temp view cache you can not bother checking if its
> referenced.

> you could ifdef this and note that editing original is ok.

> or just leave it if your worried it causes problems later.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/modifiers/intern/MOD_weightvg_util.c#newcode288
source/blender/modifiers/intern/MOD_weightvg_util.c:288: #pragma omp
parallel for schedule(static)
On 2012/01/19 15:55:55, brechtvl wrote:
> Parallelization of such simple loops will introduce considerable
slowdown for
> simpler meshes, don't think it's a good idea to use this here.

Done.

http://codereview.appspot.com/5531047/diff/9001/source/blender/modifiers/intern/MOD_weightvgedit.c
File source/blender/modifiers/intern/MOD_weightvgedit.c (right):

http://codereview.appspot.com/5531047/diff/9001/source/blender/modifiers/intern/MOD_weightvgedit.c#newcode241
source/blender/modifiers/intern/MOD_weightvgedit.c:241: if(do_prev)
On 2012/01/19 14:08:02, ideasman42 wrote:
> would prefer that the generic modifier flag be checked in the main
modifier loop
> and this function run there, not call from within each vgroup
modifier.

> This way you have the advantage that if 2+ modifiers have this option
you can
> only calculate for the last one.

Problem is, there would be no nice way to indicate which vgroup weights
to preview, and no way to preview special things... Proposed another
solution in new patch.

http://codereview.appspot.com/5531047/