Modal bevel operator

This patch modifies the bevel operator slightly so that it becomes modal. Moving the mouse will modify the bevel factor and E/D keys will change the behavior of the tool

[psy-fi, 2012-05-15 15:04:55]

On 2012/05/15 13:59:24, psy-fi wrote:

Fix a few non-modal operator issues, duplicate just the bmesh for the operator
and avoid bmesh duplication for the non-modal case

[psy-fi, 2012-05-15 15:06:03]

fixes mentioned

[psy-fi, 2012-05-15 22:05:26]

Add support for inset, separate bmesh caching code.

[psy-fi, 2012-05-15 22:07:00]

Also, add hotkey for inset (I). To change thickness move mouse, to change depth
press Ctrl and move mouse and to change to outset press O.

[ideasman42, 2012-05-15 22:40:02]

one main request - regarding storing copies of operator settings, other changes
are knitpicks.

http://codereview.appspot.com/6208066/diff/8001/source/blender/editors/mesh/e...
File source/blender/editors/mesh/editmesh_tools.c (right):

http://codereview.appspot.com/6208066/diff/8001/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4317: int modal;
*knitpick*, prefer 'is_modal' since modal is a callback struct member elsewhere.

http://codereview.appspot.com/6208066/diff/8001/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4503: if(!edbm_bevel_init(C, op,
1))
*knitpick*, and own personal pref - use TRUE/FALSE makes bool args more
readable.

http://codereview.appspot.com/6208066/diff/8001/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4634: int use_boundary;
storing settings for modal ops like mouse coords for comparison makes sense but
think this is overkill storing many settings twice. mot related to storage - but
general maintanance - we have settings in wmOperator and BMesh-operators double
up already. couldnt the modal operator allocate (or duplicate) the op->ptr? - of
you need original settings it could make 2 duplicates and then use RNA access.

Internally if a var has to be accessed in a loop or so ofcourse it should be
made a local var to avoid many string lookups.

http://codereview.appspot.com/6208066/diff/8001/source/blender/editors/mesh/e...
File source/blender/editors/mesh/editmesh_utils.c (right):

http://codereview.appspot.com/6208066/diff/8001/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_utils.c:64: BMBackup
EDBM_save_mesh_backup(BMEditMesh *em)
*knitpick*, backup is a bitof an odd name here...
perhaps
EDBM_redo_state_store()
EDBM_redo_state_restore()
EDBM_redo_state_free()
 ?

http://codereview.appspot.com/6208066/diff/8001/source/blender/editors/transf...
File source/blender/editors/transform/transform.c (right):

http://codereview.appspot.com/6208066/diff/8001/source/blender/editors/transf...
source/blender/editors/transform/transform.c:1137: int
calculateTransformCenter(bContext *C, int centerMode, float *vec, int *ivec)
*knitpiick* named here are not too helpful - suggest
float tx_cent_3d[3], float tx_cent_region[2] ? 
or just cent3d[3], cent2d as struct members are.

[psy-fi, 2012-05-16 10:23:39]

Address issues pointed at during review

[ideasman42, 2012-05-16 10:44:09]

looks good, still some remaining knitpicks and one question.

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
File source/blender/editors/mesh/editmesh_tools.c (right):

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4309: float *w;
*knitpick* weights ? - better readable

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4495: static int
edbm_bevel_invoke(bContext *C, wmOperator *op, wmEvent *event)
suggest add "TODO make modal keymap (see fly mode)"

not needed for initial commit

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4509: opdata->initial_length =
sqrt(mlen[0]*mlen[0] + mlen[1]*mlen[1]);
use len_v2

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4541: factor = sqrt(xdiff*xdiff +
ydiff*ydiff)/opdata->initial_length;
len_v2

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4653: static char str[] = "Confirm:
Enter/LClick, Cancel: (Esc/RClick), thickness: %f, depth (Ctrl to tweak): %f
(%s), Outset (O): (%s)";
on topic of keymaps - this kind of string should be generated from modal
keymaps. longer term todo.

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4681: opdata->modify_depth = 0;
FALSE

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4800: opdata->initial_length =
sqrt(mlen[0]*mlen[0] + mlen[1]*mlen[1]);
len_v2

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4830: depth = opdata->old_depth +
(sqrt(xdiff*xdiff + ydiff*ydiff)
len_v2

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4839: -
opdata->initial_length)/opdata->initial_length;
len_v2

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4877: opdata->initial_length =
sqrt(mlen[0]*mlen[0] + mlen[1]*mlen[1]);
len_v2

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4923: RNA_def_boolean(ot->srna,
"use_relative_offset", TRUE, "Offset Relative", "Scale the offset by surrounding
geometry");
why is this changed?

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/transf...
File source/blender/editors/transform/transform.c (right):

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/transf...
source/blender/editors/transform/transform.c:1137: int
calculateTransformCenter(bContext *C, int centerMode, float *cent3d, int
*cent2d)
*knitpick* use cent3d[3], cent2d[2], makes more obvious and means compilers can
detect buffer overruns in places.

[psy-fi, 2012-05-16 11:06:21]

Address more issues pointed at during review

[psy-fi, 2012-05-16 11:07:35]

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
File source/blender/editors/mesh/editmesh_tools.c (right):

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4653: static char str[] = "Confirm:
Enter/LClick, Cancel: (Esc/RClick), thickness: %f, depth (Ctrl to tweak): %f
(%s), Outset (O): (%s)";
On 2012/05/16 10:44:09, ideasman42 wrote:
> on topic of keymaps - this kind of string should be generated from modal
> keymaps. longer term todo.

Yes, I was thinking along the same lines too. That would be great to have.

http://codereview.appspot.com/6208066/diff/3002/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_tools.c:4923: RNA_def_boolean(ot->srna,
"use_relative_offset", TRUE, "Offset Relative", "Scale the offset by surrounding
geometry");
On 2012/05/16 10:44:09, ideasman42 wrote:
> why is this changed?

Because it is difficult to estimate a good absolute thickness when using screen
space mouse values. Changing to relative gives more predictable results. If the
user needs absolute values he usually will know the exact value and place it
manually.