[Bf-codereview] Normals baker from sculpt data (issue4518055)

[brechtvanlommel at gmail.com]

Comments inline. The biggest thing I guess is that this should be using
the jobs system, as already discussed with Ton I think.

As an overall note, it would be good if the code could use a style
consistent with the other code in the file. Spacing, brackets, naming,
comments, use different styles even inside single functions here.


http://codereview.appspot.com/4518055/diff/1/release/scripts/startup/bl_ui/properties_render.py
File release/scripts/startup/bl_ui/properties_render.py (right):

http://codereview.appspot.com/4518055/diff/1/release/scripts/startup/bl_ui/properties_render.py#newcode614
release/scripts/startup/bl_ui/properties_render.py:614: layout.prop(rd,
"bake_method")
This could be exposed differently in the UI. Select bake type Normal,
and then select if you want to bake from Multires or from the mesh.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c
File source/blender/editors/object/object_bake.c (right):

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode86
source/blender/editors/object/object_bake.c:86: /* ******************
sclupt BAKING ********************** */
Typo.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode155
source/blender/editors/object/object_bake.c:155: const int iBSize=
(w*h+0x7)/8;
I wouldn't bother trying to save some bits here with these bitflag
tricks, prefer readable code. Also it seems these texels are the same
the bake filter mask, so best to use same storage if you want to support
that eventually.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode162
source/blender/editors/object/object_bake.c:162:
memset(bake_rast->texels, 0, iBSize);
Already did calloc, no need to zero again.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode170
source/blender/editors/object/object_bake.c:170: memset(bake_rast, 0,
sizeof(SBakeRast));
Prefer to avoid this kind of "paranoid" memory clearing.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode418
source/blender/editors/object/object_bake.c:418:
Some comments in this function would be good, it's hard to understand
what is going on. Perhaps some code could be split into a utility
function in multires.c too.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode476
source/blender/editors/object/object_bake.c:476: rrgb[0]= vec[0]*255;
Use FTOCHAR(vec[0]) for correct rounding and clamping.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode487
source/blender/editors/object/object_bake.c:487: MultiresModifierData
*mmd = get_multires_modifier(scene, ob, 0);
Should call get_multires_modifier after object NULL check.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode498
source/blender/editors/object/object_bake.c:498:
mesh_get_derived_final(scene, ob, scene->customdata_mask);
Is there a specific reason to call this, was it missing in some case? In
principle the derivedmesh should have already been created, as part of
depsgraph updates. Calling this outside of that isn't strictly correct,
though some places do it.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode528
source/blender/editors/object/object_bake.c:528: BKE_report(op->reports,
RPT_ERROR, "You should have active texture to use sculpt baker");
It should check if there are no faces with a texture assigned at all, if
there part of the faces don't have an image assigned it can still
continue. More consistent with other baking code. Also error message
could be more accurate, something like "Mesh faces must have image
texture assigned".

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode574
source/blender/editors/object/object_bake.c:574: ImBuf *ibuf=
BKE_image_get_ibuf(ima, NULL);
Here and elsewhere there should be better check to ensure that
ibuf!=NULL, that it has the pixels and correct channels, etc. See the
existing baking code.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode576
source/blender/editors/object/object_bake.c:576: if(ibuf->x>0 &&
ibuf->x>0)
Second check should be ibuf->y > 0.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode626
source/blender/editors/object/object_bake.c:626: Object *ob=
CTX_data_active_object(C);
Other baking functions work on selected objects, not the active object.
Ideally this would work consistent with that.

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode641
source/blender/editors/object/object_bake.c:641: DerivedMesh *cddm=
CDDM_from_mesh(me, NULL);
The above call to CDDM_from_mesh passes in the object, this one doesn't,
any specific reason?

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode643
source/blender/editors/object/object_bake.c:643: tmp_mmd.lvl = lvl;
Maybe the level here should be clamped against the max level of the
modifier?

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode648
source/blender/editors/object/object_bake.c:648: /* Clear image if
needed (currently always needed) */
 From the code, it's not clear to me how this different than other baking
methods, why it's always needed here?

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/object_bake.c#newcode898
source/blender/editors/object/object_bake.c:898: result=
sculptbake_image_exec(C, op);
The operator here is being executed blocking, this should be using the
jobs system and progress bar like other bake methods, as already
discussed with Ton.

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

http://codereview.appspot.com/4518055/diff/1/source/blender/makesrna/intern/rna_scene.c#newcode2003
source/blender/makesrna/intern/rna_scene.c:2003: {R_BAKE_SCULPT,
"SCULPT", 0, "Sculpt baker", "Optimized baker for sculpt data"},
Calling it multires baking rather than sculpt baking would be more
accurate.

http://codereview.appspot.com/4518055/diff/1/source/blender/makesrna/intern/rna_scene.c#newcode2823
source/blender/makesrna/intern/rna_scene.c:2823:
RNA_def_property_ui_text(prop, "Bake to level", "Bake to specified
subdivision level");
I'd get rid of this option, and just bake to the current viewport level.

http://codereview.appspot.com/4518055/