Weight Paint (Radish Branch 2011)

[ideasman42, 2011-09-15 03:00:08]

Some issues from my review raised a few issues..

- Should pose bone select behavior be changed?
- Should we accept vgroup weight fix tool (moves verts about and checks their
new locations by recalculating the entire modifier stack).


There are some other todo's but Im not so worried about these. - vertex theme
color, hidden verts, they are easy to do.

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

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/DerivedMesh.c:1729: input = -1;
a bit odd logic, could just set the color to black and return here?

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/intern...
File source/blender/blenkernel/intern/cdderivedmesh.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/cdderivedmesh.c:284: // TODO define selected
color
This is fairly easy to add, can do this once in trunk.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/armature/...
File source/blender/editors/armature/editarmature.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/armature/...
source/blender/editors/armature/editarmature.c:4312: /* Jason was here, I'm
doing a select for multibone painting */
Not sure about changing bone selection logic (from user POV), at the very least
I think it should work the same as it does now, unless multipaint is enabled -
if the functionality is really needed.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
File source/blender/editors/object/object_vgroup.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:708: MVert *mv = me->mvert;
picky comment: 'mv' is mainly used for single verts, would rename to 'mvert'  to
avoid confusion.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:2365: void
OBJECT_OT_vertex_group_fix(wmOperatorType *ot)
I'm worried about this operator, it moves verts 1 by 1 and rebuilds all
modifiers, this just seems like its going to be really inefficient.

On the other hand if a user has to do this manually its going to take them a
time anyway.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
File source/blender/editors/sculpt_paint/paint_ops.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
source/blender/editors/sculpt_paint/paint_ops.c:622: 
Notice CTRL+I for invert selection is missing here.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
File source/blender/editors/sculpt_paint/paint_vertex.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
source/blender/editors/sculpt_paint/paint_vertex.c:1077: #if 0 /* UNUSED */
left in commented code, once merged into trunk we should remove them.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
source/blender/editors/sculpt_paint/paint_vertex.c:1512: static void
do_weight_paint_vertex(VPaint *wp, Object *ob, int index,
I'd like to see if do_weight_paint_vertex() can be split up better so the simple
behavior can he kept easy to follow and the complex behavior kept in its own
else {} clause.

I'd need to look into this further in exactly how to do it, but think its worth
trying.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
source/blender/editors/sculpt_paint/paint_vertex.c:1936: // Ugly hack, to avoid
drawing vertex index when getting the face index buffer - campbell
I had to add this, but should have made more comprehensive comment, vertex
selection is not needed here. we could make it not conflict but even then it
still wouldnt be needed.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
File source/blender/editors/space_view3d/drawobject.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
source/blender/editors/space_view3d/drawobject.c:6531: // TODO: support hidden
vertices
This is fairly easy to add, just need to pass the vertex array in user data.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
File source/blender/editors/space_view3d/view3d_select.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
source/blender/editors/space_view3d/view3d_select.c:206: static void
EM_backbuf_checkAndSelectTVerts(Mesh *me, int select)
This functions confusingly prefixed EM_ which stands for EditMesh, but this
actually runs in object mode.

[mattebb, 2011-09-15 06:49:12]

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_scene.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_scene.c:1123: RNA_def_property_update(prop,
0, "rna_update_active_object");
This doesn't look right - especially doing it from a scene property. I think
this needs an alternative.

[ideasman42, 2011-09-15 07:07:43]

reply to matt's comment.

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_scene.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_scene.c:1123: RNA_def_property_update(prop,
0, "rna_update_active_object");
On 2011/09/15 06:49:12, mattebb wrote:
> This doesn't look right - especially doing it from a scene property. I think
> this needs an alternative.

You're right, replaced these with RNA_def_property_update(prop, NC_GEOM|ND_DATA,
0); r40213.

[nazgul, 2011-09-15 07:57:10]

Made some review.

Some general notes (which aren't actually stoppers and can be made after merge).
I'm not fan of adding comments like "Jason was here" -- it's svn and it stores
that it's your code, so i prefer putting only comments which actually helps to
understand code. Also i've seen ambiguity in using '//' vs. '/* */' comments.
IMO better not to mix this comments. Also, not sure why, but we didn't use
FALSE/TRUE constants much before.

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/BKE_De...
File source/blender/blenkernel/BKE_DerivedMesh.h (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/BKE_De...
source/blender/blenkernel/BKE_DerivedMesh.h:226: void
(*drawSelectedVerts)(DerivedMesh *dm);
Not sure it's really cool change. If you add new callback to DerivedMesh it
should be implemented for all types of derived mesh, but I didn't see changes to
CCGDM. If this function is intended to work with CDDM only, wouldn't it better
to implement it outside of DerivedMesh structure?

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/mesh/edit...
File source/blender/editors/mesh/editmesh.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/mesh/edit...
source/blender/editors/mesh/editmesh.c:1995: /* Jason note: caller needs to run
paintvert_flush_flags(ob) after this */
I can see paintvert_flush_flags() called in the end of this function. Is this
comment still relevant or this function will be called twice in several cases?

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
File source/blender/editors/object/object_vgroup.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:789: static int*
getSurroundingVerts(Mesh *me, int vert, int *count) {
Looks like count wouldn't be set in several cases (if found is zero to the end
of this funciton)

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:851: static void
getSingleCoordinate(MVert **points, int count, float *coord) {
Why points are passed as reference to an array?

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:854: coord[k] = 0;
This line and few next lines: why not use vector functions from math module? I
mean zero_v3(), add_v3_v3() and mul_v3_fl.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:901: projA =
MEM_callocN(sizeof(float)*3, "projectedA");
Why don't allocate vectors in stack? It'll be faster and you shouldn't worry
about free-ing memory in the end of this function.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:920: dm->needsFree = 1;
This doesn't look correct for me. Or readability can be improved here. You're
releasing dm which was passed as argument, and then set ob->derivedDeform to
NULL. If you're always passing derivedDeform here as dm, then why not use
ob->derivedDeform for this and avoid sending dm as argument. And if dm is not
always derivedDeform, then it'll be memory leak here because you're setting
derivedDeform to NULL without freeing memory.
Also, i thought it's more convenient to free derivedFinal so all familiar
derived meshes are also re-calculated.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:945: float *oldPos =
MEM_callocN(sizeof(float)*3, "oldPosition");
Again, why not allocate in stack?

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:948: float **changes =
MEM_mallocN(sizeof(float *)*totweight, "vertHorzChange");
IMO, this can be simplified by using float (*changes)[2] and allocating memory
for totweight 2-component vectors. Saves amount of allocations and should give
speedup.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:2337: Object *ob=
CTX_data_pointer_get_type(C, "object", &RNA_Object).data;
Isn't it CTX_data_active_object? If it is, i've seen more usages of
CTX_data_pointer_get_type() to obtain object.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
File source/blender/editors/sculpt_paint/paint_vertex.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
source/blender/editors/sculpt_paint/paint_vertex.c:1192: #if 0 /* UNUSED */
IMO better to remove unused code. But it can be done after merge, so it'll be in
svn history.

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_object.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_object.c:224: void
rna_update_active_object(Main *bmain, Scene *scene, PointerRNA *ptr)
I've seen this function is used only once in rna_scene. Is it really so common
so it should be public?

[ideasman42, 2011-09-15 12:43:04]

Reply to nazgul's comments.

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/BKE_De...
File source/blender/blenkernel/BKE_DerivedMesh.h (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/BKE_De...
source/blender/blenkernel/BKE_DerivedMesh.h:226: void
(*drawSelectedVerts)(DerivedMesh *dm);
On 2011/09/15 07:57:10, nazgul wrote:
> Not sure it's really cool change. If you add new callback to DerivedMesh it
> should be implemented for all types of derived mesh, but I didn't see changes
to
> CCGDM. If this function is intended to work with CDDM only, wouldn't it better
> to implement it outside of DerivedMesh structure?

Agree, this should use 'foreachMappedVert' with a callback for drawing.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
File source/blender/editors/object/object_vgroup.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:901: projA =
MEM_callocN(sizeof(float)*3, "projectedA");
On 2011/09/15 07:57:10, nazgul wrote:
> Why don't allocate vectors in stack? It'll be faster and you shouldn't worry
> about free-ing memory in the end of this function.

I replaced a few of these but evidently missed this one, of course they shuld be
stack memory.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:2337: Object *ob=
CTX_data_pointer_get_type(C, "object", &RNA_Object).data;
On 2011/09/15 07:57:10, nazgul wrote:
> Isn't it CTX_data_active_object? If it is, i've seen more usages of
> CTX_data_pointer_get_type() to obtain object.

Yep, should be changed.

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_object.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_object.c:224: void
rna_update_active_object(Main *bmain, Scene *scene, PointerRNA *ptr)
On 2011/09/15 07:57:10, nazgul wrote:
> I've seen this function is used only once in rna_scene. Is it really so common
> so it should be public?

Its been removed now.

[jason.hays22, 2011-09-17 19:17:37]

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_object.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_object.c:224: void
rna_update_active_object(Main *bmain, Scene *scene, PointerRNA *ptr)
On 2011/09/15 12:43:04, ideasman42 wrote:
> On 2011/09/15 07:57:10, nazgul wrote:
> > I've seen this function is used only once in rna_scene. Is it really so
common
> > so it should be public?
> 
> Its been removed now.
Hey, I had a reason for this.

RNA_def_property_update(prop, NC_GEOM|ND_DATA, 0);
didn't work well enough in rna_scene because when Multi-Paint is
activated/deactivated, the colors will not change right away when multiple bones
are selected.
The same thing occurs with auto normalize (while multi-paint is active)

What should I do as an alternative?

[jason.hays22, 2011-09-17 19:26:51]

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/mesh/edit...
File source/blender/editors/mesh/editmesh.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/mesh/edit...
source/blender/editors/mesh/editmesh.c:1996: void
paintvert_deselect_all_visible(Object *ob, int action, short flush_flags)
The optional flush is mostly for the selection tools (like box select). In such
cases you would want to modify the selection immediately afterwards, and
flushing until that is finished would be a waste.

I'll modify the comment, however, to say that the caller needs to run it
eventually if flush_flags is false.

[jason.hays22, 2011-09-17 22:48:38]

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
File source/blender/editors/space_view3d/view3d_select.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
source/blender/editors/space_view3d/view3d_select.c:206: static void
EM_backbuf_checkAndSelectTVerts(Mesh *me, int select)
On 2011/09/15 03:00:08, ideasman42 wrote:
> This functions confusingly prefixed EM_ which stands for EditMesh, but this
> actually runs in object mode.
I was following the example of the face mask function
EM_backbuf_checkAndSelectTFaces()
Would you like me to change it anyway?

[jason.hays22, 2011-09-18 01:14:46]

I also added hidden verts support to a function in drawobject.c--I forgot that
in the SVN list of changes.

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/BKE_De...
File source/blender/blenkernel/BKE_DerivedMesh.h (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/BKE_De...
source/blender/blenkernel/BKE_DerivedMesh.h:226: void
(*drawSelectedVerts)(DerivedMesh *dm);
On 2011/09/15 07:57:10, nazgul wrote:
> Not sure it's really cool change. If you add new callback to DerivedMesh it
> should be implemented for all types of derived mesh, but I didn't see changes
to
> CCGDM. If this function is intended to work with CDDM only, wouldn't it better
> to implement it outside of DerivedMesh structure?

Done.

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

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/DerivedMesh.c:1729: input = -1;
On 2011/09/15 03:00:08, ideasman42 wrote:
> a bit odd logic, could just set the color to black and return here?

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/armature/...
File source/blender/editors/armature/editarmature.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/armature/...
source/blender/editors/armature/editarmature.c:4312: /* Jason was here, I'm
doing a select for multibone painting */
On 2011/09/15 03:00:08, ideasman42 wrote:
> Not sure about changing bone selection logic (from user POV), at the very
least
> I think it should work the same as it does now, unless multipaint is enabled -
> if the functionality is really needed.

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
File source/blender/editors/object/object_vgroup.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:708: MVert *mv = me->mvert;
On 2011/09/15 03:00:08, ideasman42 wrote:
> picky comment: 'mv' is mainly used for single verts, would rename to 'mvert' 
to
> avoid confusion.

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:789: static int*
getSurroundingVerts(Mesh *me, int vert, int *count) {
On 2011/09/15 07:57:10, nazgul wrote:
> Looks like count wouldn't be set in several cases (if found is zero to the end
> of this funciton)

How?  Unless it is a single point in space, it'll have vertices surrounding it..
 Do you have an example?

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:851: static void
getSingleCoordinate(MVert **points, int count, float *coord) {
On 2011/09/15 07:57:10, nazgul wrote:
> Why points are passed as reference to an array?

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:854: coord[k] = 0;
On 2011/09/15 07:57:10, nazgul wrote:
> This line and few next lines: why not use vector functions from math module? I
> mean zero_v3(), add_v3_v3() and mul_v3_fl.

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:901: projA =
MEM_callocN(sizeof(float)*3, "projectedA");
On 2011/09/15 07:57:10, nazgul wrote:
> Why don't allocate vectors in stack? It'll be faster and you shouldn't worry
> about free-ing memory in the end of this function.

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:920: dm->needsFree = 1;
On 2011/09/15 07:57:10, nazgul wrote:
> This doesn't look correct for me. Or readability can be improved here. You're
> releasing dm which was passed as argument, and then set ob->derivedDeform to
> NULL. If you're always passing derivedDeform here as dm, then why not use
> ob->derivedDeform for this and avoid sending dm as argument. And if dm is not
> always derivedDeform, then it'll be memory leak here because you're setting
> derivedDeform to NULL without freeing memory.
> Also, i thought it's more convenient to free derivedFinal so all familiar
> derived meshes are also re-calculated.

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:945: float *oldPos =
MEM_callocN(sizeof(float)*3, "oldPosition");
On 2011/09/15 07:57:10, nazgul wrote:
> Again, why not allocate in stack?

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:948: float **changes =
MEM_mallocN(sizeof(float *)*totweight, "vertHorzChange");
On 2011/09/15 07:57:10, nazgul wrote:
> IMO, this can be simplified by using float (*changes)[2] and allocating memory
> for totweight 2-component vectors. Saves amount of allocations and should give
> speedup.

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:2337: Object *ob=
CTX_data_pointer_get_type(C, "object", &RNA_Object).data;
On 2011/09/15 07:57:10, nazgul wrote:
> Isn't it CTX_data_active_object? If it is, i've seen more usages of
> CTX_data_pointer_get_type() to obtain object.

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
File source/blender/editors/sculpt_paint/paint_ops.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_pa...
source/blender/editors/sculpt_paint/paint_ops.c:622: 
On 2011/09/15 03:00:08, ideasman42 wrote:
> Notice CTRL+I for invert selection is missing here.

Done.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
File source/blender/editors/space_view3d/drawobject.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
source/blender/editors/space_view3d/drawobject.c:6531: // TODO: support hidden
vertices
On 2011/09/15 03:00:08, ideasman42 wrote:
> This is fairly easy to add, just need to pass the vertex array in user data.

Done.

[ideasman42, 2011-09-18 03:28:21]

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
File source/blender/editors/space_view3d/view3d_select.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
source/blender/editors/space_view3d/view3d_select.c:206: static void
EM_backbuf_checkAndSelectTVerts(Mesh *me, int select)
On 2011/09/17 22:48:38, jason.hays22 wrote:
> On 2011/09/15 03:00:08, ideasman42 wrote:
> > This functions confusingly prefixed EM_ which stands for EditMesh, but this
> > actually runs in object mode.
> I was following the example of the face mask function
> EM_backbuf_checkAndSelectTFaces()
> Would you like me to change it anyway?

ack, I see you're point.
EM_backbuf_checkAndSelectTFaces is also wrong.

committed r40313, and commented that this is wrong confusing, temp name:
EM_backbuf_checkAndSelectVerts_obmode

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_object.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_object.c:224: void
rna_update_active_object(Main *bmain, Scene *scene, PointerRNA *ptr)
On 2011/09/17 19:17:37, jason.hays22 wrote:
> On 2011/09/15 12:43:04, ideasman42 wrote:
> > On 2011/09/15 07:57:10, nazgul wrote:
> > > I've seen this function is used only once in rna_scene. Is it really so
> common
> > > so it should be public?
> > 
> > Its been removed now.
> Hey, I had a reason for this.
> 
> RNA_def_property_update(prop, NC_GEOM|ND_DATA, 0);
> didn't work well enough in rna_scene because when Multi-Paint is
> activated/deactivated, the colors will not change right away when multiple
bones
> are selected.
> The same thing occurs with auto normalize (while multi-paint is active)
> 
> What should I do as an alternative?

Yep, I see this is a problem - Added this back under
'rna_Scene_update_active_object_data', with a note that this should only be used
in special cases and that it may be better as a notifier.

[jason.hays22, 2011-09-18 03:41:30]

On 2011/09/18 03:28:21, ideasman42 wrote:
>
http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
> File source/blender/editors/space_view3d/view3d_select.c (right):
> 
>
http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_vie...
> source/blender/editors/space_view3d/view3d_select.c:206: static void
> EM_backbuf_checkAndSelectTVerts(Mesh *me, int select)
> On 2011/09/17 22:48:38, jason.hays22 wrote:
> > On 2011/09/15 03:00:08, ideasman42 wrote:
> > > This functions confusingly prefixed EM_ which stands for EditMesh, but
this
> > > actually runs in object mode.
> > I was following the example of the face mask function
> > EM_backbuf_checkAndSelectTFaces()
> > Would you like me to change it anyway?
> 
> ack, I see you're point.
> EM_backbuf_checkAndSelectTFaces is also wrong.
> 
> committed r40313, and commented that this is wrong confusing, temp name:
> EM_backbuf_checkAndSelectVerts_obmode
> 
>
http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
> File source/blender/makesrna/intern/rna_object.c (right):
> 
>
http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/r...
> source/blender/makesrna/intern/rna_object.c:224: void
> rna_update_active_object(Main *bmain, Scene *scene, PointerRNA *ptr)
> On 2011/09/17 19:17:37, jason.hays22 wrote:
> > On 2011/09/15 12:43:04, ideasman42 wrote:
> > > On 2011/09/15 07:57:10, nazgul wrote:
> > > > I've seen this function is used only once in rna_scene. Is it really so
> > common
> > > > so it should be public?
> > > 
> > > Its been removed now.
> > Hey, I had a reason for this.
> > 
> > RNA_def_property_update(prop, NC_GEOM|ND_DATA, 0);
> > didn't work well enough in rna_scene because when Multi-Paint is
> > activated/deactivated, the colors will not change right away when multiple
> bones
> > are selected.
> > The same thing occurs with auto normalize (while multi-paint is active)
> > 
> > What should I do as an alternative?
> 
> Yep, I see this is a problem - Added this back under
> 'rna_Scene_update_active_object_data', with a note that this should only be
used
> in special cases and that it may be better as a notifier.

What about auto normalize?  It changes the way multi-paint draws the weight
colors, and if it is toggled without that update function, the colors will not
update immediately.

[nazgul, 2011-09-18 08:59:50]

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
File source/blender/editors/object/object_vgroup.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_vgroup.c:789: static int*
getSurroundingVerts(Mesh *me, int vert, int *count) {
On 2011/09/18 01:14:46, jason.hays22 wrote:
> On 2011/09/15 07:57:10, nazgul wrote:
> > Looks like count wouldn't be set in several cases (if found is zero to the
end
> > of this funciton)
> 
> How?  Unless it is a single point in space, it'll have vertices surrounding
it..
>  Do you have an example?

Well, for example mesh as point cloud. Not sure it's really issue. That message
was just more like a warning which better be checked -- depends on how result of
function is used.