Radial control RNA update

Patch to make the radial control more generic with RNA.

In the current radial control code in trunk, generic parts of the radial control are implemented as an incomplete operator within WM. Then each different user of the radial control has to implement a separate operator to actually pass in specific brush data -- e.g. sculpt's brush size, vpaint's brush size, etc.

This patch removes all the extra operators and makes the WM operator do everything. It now takes several RNA path strings as its properties -- the only required property is data_path, which specifies the data to be modified by the radial control. The other paths affect display in various ways, e.g. rotation, color, etc.

In addition to decreasing some duplicate paint brush code, these updates make it pretty easy to enable radial control for other purposes (and it can be set up entirely though python or keymaps, no extra C code needed.)

[brechtvl, 2011-04-01 09:02:50]

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_pa...
File source/blender/editors/sculpt_paint/paint_ops.c (right):

http://codereview.appspot.com/4280080/diff/1/source/blender/editors/sculpt_pa...
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/r...
File source/blender/makesrna/intern/rna_brush.c (right):

http://codereview.appspot.com/4280080/diff/1/source/blender/makesrna/intern/r...
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/r...
File source/blender/makesrna/intern/rna_space.c (right):

http://codereview.appspot.com/4280080/diff/1/source/blender/makesrna/intern/r...
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/r...
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/int...
File source/blender/windowmanager/intern/wm_operators.c (right):

http://codereview.appspot.com/4280080/diff/1/source/blender/windowmanager/int...
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/int...
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/int...
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/int...
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/int...
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/int...
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?

[nicholasbishop, 2011-04-03 05:23:03]

Updated patch; should address everything except for the brush GL texture issue

[nicholasbishop, 2011-04-03 05:31:44]

Thanks for the detailed review :) I uploaded a new patch to address most of the
issues you raised.

A question about improving the brush texture-getter:
> 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.

Do you mean that a GL texture field should be added to the ID struct itself?

[brechtvl, 2011-04-03 16:00:01]

On 2011/04/03 05:31:44, nicholasbishop wrote:
> A question about improving the brush texture-getter:
> > 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.
> 
> Do you mean that a GL texture field should be added to the ID struct itself?

If it doesn't go through RNA, the gl texture id could simply be stored in the
operator data and freed at the end.

The reason you added the RNA function as a property is to make it more generic,
but my impression is that this adds quite a bit of complexity, without making
things all that generic in practice. So if the datablock is passed as a
property, it could check if it's a brush, and if so use the texture from that.

That's going against the purpose of this patch, to me it still seems better, I
can't really thing of a nice way to decouple this.

[nicholasbishop, 2011-04-03 17:24:08]

Removed brush image RNA function

[nicholasbishop, 2011-04-03 17:26:02]

That makes good sense, updated the patch to remove the extra RNA stuff.

[brechtvl, 2011-04-15 11:57:28]

Also here, LGTM after latest changes.