[Bf-codereview] Radial control RNA update (issue4280080)

[brechtvanlommel at gmail.com]

I like this a lot, works much more like it should now. Comments inline.


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

http://codereview.appspot.com/4280080/diff/1/source/blender/editors/sculpt_paint/paint_ops.c#newcode315
source/blender/editors/sculpt_paint/paint_ops.c:315:
A bit nitpicky perhaps, but is there really a good reason to use macro's
instead of C functions here?

http://codereview.appspot.com/4280080/diff/1/source/blender/makesrna/intern/rna_brush.c
File source/blender/makesrna/intern/rna_brush.c (right):

http://codereview.appspot.com/4280080/diff/1/source/blender/makesrna/intern/rna_brush.c#newcode301
source/blender/makesrna/intern/rna_brush.c:301: static int
rna_Brush_gl_load(Brush *br)
I think this is a bit dangerous, requiring the texture to be freed by
the caller, and it's different than the image datablock. Could the
opengl texture id be owned by the Brush instead?

Another alternative would be to not go through RNA here at all, and tie
these textures to ID blocks, and basically pass in the ID block as a
property, and have a generic function to get a texture from them. I'd
prefer that personally, and it makes it possible to get rid of fairly
complex RNA function calling code in the operator.

http://codereview.appspot.com/4280080/diff/1/source/blender/makesrna/intern/rna_space.c
File source/blender/makesrna/intern/rna_space.c (right):

http://codereview.appspot.com/4280080/diff/1/source/blender/makesrna/intern/rna_space.c#newcode484
source/blender/makesrna/intern/rna_space.c:484:
ED_space_image_zoom(sima, ar, &values[0], &values[1]);
This should get a stub in
source/blenderplayer/bad_level_call_stubs/stubs.c for the blenderplayer.

http://codereview.appspot.com/4280080/diff/1/source/blender/makesrna/intern/rna_space.c#newcode1499
source/blender/makesrna/intern/rna_space.c:1499:
RNA_def_property_ui_text(prop, "Zoom", "Zoom factor");
This property should also its editable flag cleared, otherwise it can
write to SpaceImage.zoom directly.

http://codereview.appspot.com/4280080/diff/1/source/blender/windowmanager/intern/wm_operators.c
File source/blender/windowmanager/intern/wm_operators.c (right):

http://codereview.appspot.com/4280080/diff/1/source/blender/windowmanager/intern/wm_operators.c#newcode84
source/blender/windowmanager/intern/wm_operators.c:84: /* TODO: remove
this */
Guess it can be removed now, compiles fine without.

http://codereview.appspot.com/4280080/diff/1/source/blender/windowmanager/intern/wm_operators.c#newcode2658
source/blender/windowmanager/intern/wm_operators.c:2658: unsigned int
gltex;
It seems this texture is never deleted?

http://codereview.appspot.com/4280080/diff/1/source/blender/windowmanager/intern/wm_operators.c#newcode2680
source/blender/windowmanager/intern/wm_operators.c:2680: d =
WM_RADIAL_CONTROL_DISPLAY_SIZE * sin(rc->initial_value);
The first line setting d will have no effect.

http://codereview.appspot.com/4280080/diff/1/source/blender/windowmanager/intern/wm_operators.c#newcode2698
source/blender/windowmanager/intern/wm_operators.c:2698: PropertyRNA
**prop, FunctionRNA **func)
I think it would be good to create a function RNA_path_resolve_function
in rna_access.c, once there is string manipulation of the path it's best
to keep it together there in case changes are made later.

http://codereview.appspot.com/4280080/diff/1/source/blender/windowmanager/intern/wm_operators.c#newcode2900
source/blender/windowmanager/intern/wm_operators.c:2900: #define
_GET_RNA(_name, _ptr, _prop, _check) \
I would also just make this a function instead of a macro, and maybe
make a separate radial_control_get_properties to get all the properties
and texture function, but that's just personal preference.

_GET_RNA should also check that the property is of the right type and
array length, otherwise functions like RNA_property_float_get_array can
write outside array bounds.

http://codereview.appspot.com/4280080/diff/1/source/blender/windowmanager/intern/wm_operators.c#newcode3073
source/blender/windowmanager/intern/wm_operators.c:3073:
RNA_def_string(ot->srna, "data_path", "", 256, "Data Path", "Path of
property to be set by the radial control.");
Passing 0 instead gives the string unlimited length, I guess there's no
particular reason to have 256 as max length, and the other code doesn't
seem to assume?

http://codereview.appspot.com/4280080/