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

[g.ulairi at gmail.com]

Thanks for review.
Most of reported issues are easy enough and some are already fixed in my
local version and in patch i've sent to tracker.

With some issues i can't agree and i've commented them a bit.


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#newcode487
source/blender/editors/object/object_bake.c:487: MultiresModifierData
*mmd = get_multires_modifier(scene, ob, 0);
On 2011/05/16 11:09:17, brechtvl wrote:
> Should call get_multires_modifier after object NULL check.

Don;t think it's needed. All checking happens before starting all this
code. Pardon for a bit outdated patch, but i moved all necessery
checking before actual bake logic and i'd prefer to keep check only
there. And ofcourcse there's no CTX_* access in bake logic anymore..

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);
On 2011/05/16 11:09:17, brechtvl wrote:
> 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.

Pardon again for outdated version.. This call is absolutely needed now.
Changes, made in sculpt mode should be applied on derivedMesh properly
(viewport uses optimized PBVH display for multires when sculpting).
That's why i'm using multires_force_update and as a result --
mesh_get_derived_final.

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);
On 2011/05/16 11:09:17, brechtvl wrote:
> 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.

I think it'll be better to do when checking if operator could run only
(near the place where face texture is checking)

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) */
On 2011/05/16 11:09:17, brechtvl wrote:
>  From the code, it's not clear to me how this different than other
baking
> methods, why it's always needed here?

It was simply non-implemented things for margin. It'll be fixed in new
version of patch

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);
On 2011/05/16 11:09:17, brechtvl wrote:
> 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.

Yep, it's blocking. I've got newer patch where i'm ready to switch to
jobs. But it is something which should be done with Ton -- i need
object's derivedMesh stay unchanged during the whole job work.

If you'll tell to make another copy of DM, then it'll make this patch
useless -- it'll require plenty of memory again which i and Morten don't
want. This patch was created to be able to bake plenty-million-faces
meshes which you're unabe to bake with current baker.

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#newcode2823
source/blender/makesrna/intern/rna_scene.c:2823:
RNA_def_property_ui_text(prop, "Bake to level", "Bake to specified
subdivision level");
On 2011/05/16 11:09:17, brechtvl wrote:
> I'd get rid of this option, and just bake to the current viewport
level.

There's "baking from" and "baking to" levels. Baking from is always the
same as viewport level (because it uses the same derivedMesh to save
memory). "Baking to" is used for cases when you don't want to apply
baked texture to level 0 of multires, but want to apply it to lover
level. It would give better results on render.

For example when you've cube with multires level 10 and want this cube
be level 5 on final render. In this case you'll choose bake_level=5 and
it'll give best result for such configuration/

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