result of some discussion on IRC + review. 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 ...
12 years, 3 months ago
(2012-01-19 14:08:02 UTC)
#3
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) Infact this should just be for ...
12 years, 3 months ago
(2012-01-19 16:19:14 UTC)
#5
Here is a new version of the patch. I addressed most of your advices, with ...
12 years, 3 months ago
(2012-01-19 23:10:37 UTC)
#6
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...
File source/blender/blenkernel/BKE_DerivedMesh.h (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/BKE...
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...
File source/blender/blenkernel/BKE_modifier.h (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/BKE...
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/int...
File source/blender/blenkernel/intern/DerivedMesh.c (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/int...
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/int...
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/int...
File source/blender/blenkernel/intern/dynamicpaint.c (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/int...
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/int...
File source/blender/blenkernel/intern/modifier.c (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/blenkernel/int...
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_...
File source/blender/editors/space_view3d/view3d_header.c (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/editors/space_...
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_m...
File source/blender/makesdna/DNA_modifier_types.h (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/makesdna/DNA_m...
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_m...
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_s...
File source/blender/makesdna/DNA_scene_types.h (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/makesdna/DNA_s...
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_s...
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/inter...
File source/blender/makesrna/intern/rna_modifier.c (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/makesrna/inter...
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/inter...
File source/blender/makesrna/intern/rna_scene.c (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/makesrna/inter...
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/inter...
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/inte...
File source/blender/modifiers/intern/MOD_weightvg_util.c (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/modifiers/inte...
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/inte...
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/inte...
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/inte...
File source/blender/modifiers/intern/MOD_weightvgedit.c (right):
http://codereview.appspot.com/5531047/diff/9001/source/blender/modifiers/inte...
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.
On 2012/01/21 15:33:58, mont29 wrote: > Updated patch... Hi, I just red through mont29 and ...
12 years, 3 months ago
(2012-01-21 17:11:08 UTC)
#10
On 2012/01/21 15:33:58, mont29 wrote:
> Updated patch...
Hi,
I just red through mont29 and Campbell's irc discussion about this. For what it
matters I disagree with some of the decisions made:
I definitely think modifiers should be able to decide what data they preview. Or
even preview something that is never outputted as vgroup / color, but just as
visualization on things that are going on with the modifier. (Currently there
aren't any modifiers that would need this but it definitely shouldn't be limited
in that way.)
There is also question of how modifier stack would know what to preview. For
example Dynamic Paint can simultaneously have weight *and* color surfaces. Best
way to decide which one (or neither) is previewed is to let modifier take care
of it.
About renaming CD_WEIGHT_MCOL to CD_PREVIEW_MCOL or just limiting CD_WEIGHT_MCOL
to weights only. Main reason I decided to use CD_WEIGHT_MCOL for dynapaint
preview in the first place was to not make this any more complex than it has to
be.
Just check some of the mesh drawing code. E.g. cdderivedmesh.c -> line 856.
There is horrible stack of ifs of different MCOLs (CD_ID_MCOL, CD_WEIGHT_MCOL,
CD_MCOL).
Now what if CD_WEIGHT_MCOL is limited to weights only? We would need separate
CD_PREVIEW_WEIGHT and CD_PREVIEW_COLOR and who knows what else in the future.
Which is horribly confusing considering that CD_WEIGHT_MCOL is already used
*only* for 3d-view preview.
Most importantly: there can only be *one* color layer previewed at once anyways.
Having multiple layers of different uses would simply cause more issues like
which one to prefer if both are defined, not to mention code readability and
maintainability. Why not just let modifier stack order decide that for
modifiers? Latter modifier overwrites the preview layer regardless of it's
previous value? Sounds very predictable and consistent with other modifier
actions too.
I'm also wondering what is so big deal about renaming it (to not include it in
this patch), just search+replace it everywhere on the code..? Even simple rename
should be enough to clarify that it isn't about weights but about 3d-view
preview?
Oh, and while I tried to test this patch I noticed that "Vertex Weight
Proximity" modifier immediately crashes on trunk when vertex group and target
are set.
Here is the backtrace:
http://www.miikah.org/blender/vertweight_bt.txt
On 2012/01/21 17:11:08, MiikaH wrote: > On 2012/01/21 15:33:58, mont29 wrote: > > Updated patch... ...
12 years, 3 months ago
(2012-01-21 17:35:02 UTC)
#11
On 2012/01/21 17:11:08, MiikaH wrote:
> On 2012/01/21 15:33:58, mont29 wrote:
> > Updated patch...
>
>
> Hi,
>
> I just red through mont29 and Campbell's irc discussion about this. For what
it
> matters I disagree with some of the decisions made:
>
> I definitely think modifiers should be able to decide what data they preview.
Or
> even preview something that is never outputted as vgroup / color, but just as
> visualization on things that are going on with the modifier. (Currently there
> aren't any modifiers that would need this but it definitely shouldn't be
limited
> in that way.)
>
> There is also question of how modifier stack would know what to preview. For
> example Dynamic Paint can simultaneously have weight *and* color surfaces.
Best
> way to decide which one (or neither) is previewed is to let modifier take care
> of it.
>
>
> About renaming CD_WEIGHT_MCOL to CD_PREVIEW_MCOL or just limiting
CD_WEIGHT_MCOL
> to weights only. Main reason I decided to use CD_WEIGHT_MCOL for dynapaint
> preview in the first place was to not make this any more complex than it has
to
> be.
>
> Just check some of the mesh drawing code. E.g. cdderivedmesh.c -> line 856.
> There is horrible stack of ifs of different MCOLs (CD_ID_MCOL, CD_WEIGHT_MCOL,
> CD_MCOL).
>
> Now what if CD_WEIGHT_MCOL is limited to weights only? We would need separate
> CD_PREVIEW_WEIGHT and CD_PREVIEW_COLOR and who knows what else in the future.
> Which is horribly confusing considering that CD_WEIGHT_MCOL is already used
> *only* for 3d-view preview.
>
> Most importantly: there can only be *one* color layer previewed at once
anyways.
> Having multiple layers of different uses would simply cause more issues like
> which one to prefer if both are defined, not to mention code readability and
> maintainability. Why not just let modifier stack order decide that for
> modifiers? Latter modifier overwrites the preview layer regardless of it's
> previous value? Sounds very predictable and consistent with other modifier
> actions too.
>
> I'm also wondering what is so big deal about renaming it (to not include it in
> this patch), just search+replace it everywhere on the code..? Even simple
rename
> should be enough to clarify that it isn't about weights but about 3d-view
> preview?
>
>
> Oh, and while I tried to test this patch I noticed that "Vertex Weight
> Proximity" modifier immediately crashes on trunk when vertex group and target
> are set.
> Here is the backtrace:
> http://www.miikah.org/blender/vertweight_bt.txt
I’d also rather have a good, generic preview system for modifiers... Anyway,
current patch (third one) does not reaaly satisfies me (it’s not as usable as it
shoud be).
As it’s anyway a bit late for such patch to go in 2.62, I think I’ll rather take
the long path:
* First propose a patch handling nicely modifiers preview. It would be based on
second patch here, plus pename CD_WEIGHT_MCOL to CD_PREVIEW MCOL, and, in apply
funcs of modifiers, replace of useRenderParams, isFinalCalc, etc. flags by a
single, bit-flag var (this way, it’s easy for modifier stack to add flags to
control modifiers behavior).
* Then, add weight preview to Weight modifiers (it would then be trivial, a few
lines of code in modifier files).
Note: Maybe it would be a good idea to prepare these patches directly against
bmesh ?
PS: About your segfault, miikah, it’s quite strange… afaik, it’s in some
completely unrelated code. And Proximity modifier is working well here…
21.1.2012 19:35, montagne29@wanadoo.fr wrote: > PS: About your segfault, miikah, it’s quite strange… afaik, it’s ...
12 years, 3 months ago
(2012-01-22 07:34:16 UTC)
#12
21.1.2012 19:35, montagne29@wanadoo.fr wrote:
> PS: About your segfault, miikah, it’s quite strange… afaik, it’s in some
> completely unrelated code. And Proximity modifier is working well here…
Hey,
It only seems to happen with new objects that have an "empty" vertex group.
Steps that always crash here:
*) (In object mode)
1) Add a new object
2) Add a new vertex group from "Object Data" tab
3) Add "Vertex weight proximity" modifier
4) Select that newly added (only) vgroup and any other object as target
-> Instant crash :(
Apparently I created that backtrace with your patch applied so some line
numbers were wrong, here is the backtrace on trunk r43599:
http://www.miikah.org/blender/vertweight_bt2.txt
On 2012/01/22 07:34:16, MiikaH wrote: > 21.1.2012 19:35, mailto:montagne29@wanadoo.fr wrote: > > PS: About your ...
12 years, 3 months ago
(2012-01-22 10:33:56 UTC)
#13
On 2012/01/22 07:34:16, MiikaH wrote:
> 21.1.2012 19:35, mailto:montagne29@wanadoo.fr wrote:
> > PS: About your segfault, miikah, it’s quite strange… afaik, it’s in some
> > completely unrelated code. And Proximity modifier is working well here…
>
> Hey,
>
> It only seems to happen with new objects that have an "empty" vertex group.
> Steps that always crash here:
> *) (In object mode)
> 1) Add a new object
> 2) Add a new vertex group from "Object Data" tab
> 3) Add "Vertex weight proximity" modifier
> 4) Select that newly added (only) vgroup and any other object as target
> -> Instant crash :(
>
> Apparently I created that backtrace with your patch applied so some line
> numbers were wrong, here is the backtrace on trunk r43599:
> http://www.miikah.org/blender/vertweight_bt2.txt
Fixed in trunk! Thanks for finding it (was a corner-case one :p ).
Issue 5531047: WeightVG modifiers weight preview, and general weight preview enhancement
(Closed)
Created 12 years, 3 months ago by b.mont29
Modified 11 years, 2 months ago
Reviewers: ideasman42, brechtvl, MiikaH
Base URL: https://svn.blender.org/svnroot/bf-blender/trunk/blender
Comments: 42
