Normals baker from sculpt data

Hi everybody!

Morten Mikkelsen and me were working under baker, which could bake normals directly from mesh with multi-resolution modifier without using much of additional memory.

Here's patch for Blender which makes able to bake in such way. It isn't replacing existing maker, just adds new method which could be specified from UI and bakes against given subdivision level.

It's my first change in baker-related area, so i want this patch be reviewed by other developers.

Thanks!

[brechtvl, 2011-05-16 11:09:17]

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/pr...
File release/scripts/startup/bl_ui/properties_render.py (right):

http://codereview.appspot.com/4518055/diff/1/release/scripts/startup/bl_ui/pr...
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/ob...
File source/blender/editors/object/object_bake.c (right):

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

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

http://codereview.appspot.com/4518055/diff/1/source/blender/makesrna/intern/r...
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/r...
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.

[nazgul, 2011-05-16 12:05:21]

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/ob...
File source/blender/editors/object/object_bake.c (right):

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

http://codereview.appspot.com/4518055/diff/1/source/blender/makesrna/intern/r...
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/

[brechtvl, 2011-05-16 12:45:11]

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

http://codereview.appspot.com/4518055/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_bake.c:898: result=
sculptbake_image_exec(C, op);
I don't see why we'd need another copy of the DM, but some kind of data locking
would be needed yes.

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

http://codereview.appspot.com/4518055/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_scene.c:2823: RNA_def_property_ui_text(prop,
"Bake to level", "Bake to specified subdivision level");
Ok, I would expect it to work the other way around. It seems more logical to me
to bake to what you're looking at in the viewport and immediately see the result
applied to that. It's also more consistent with existing bake methods, where you
also bake to the low resolution mesh.

The other reason is that if we ever support something like ptex, you could bake
without actually having the full high res mesh in memory.

In terms of memory usage currently, I don't think doing it either way would make
a difference? In either case, you'd have both the low and high res meshes in
memory once.

[nazgul, 2011-05-16 18:14:08]

I hope all of reported issues were fixed.

[nazgul, 2011-05-16 18:19:57]

On 2011/05/16 18:14:08, nazgul wrote:
> I hope all of reported issues were fixed.

Forgot to mention. In this version of patch heights(displacement) baking is
included.

[nazgul, 2011-05-17 08:59:59]

Fixed errors when baking several objects

[brechtvl, 2011-05-17 14:15:53]

Great improvements, some comments on the updated patch below.

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

http://codereview.appspot.com/4518055/diff/5003/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_render.py:651: layout.prop(rd,
"bake_margin")
I would wove bake clear below smooth heights option, since it's more related to
margin.

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

http://codereview.appspot.com/4518055/diff/5003/source/blender/editors/object...
source/blender/editors/object/object_bake.c:951: if(!ima ||
!BKE_image_get_ibuf(ima, NULL)) {
This still isn't sufficient as a safety check, see baking in rendercore.c. Need
to check if rect/rect_float is actually there and has the correct number of
channels.

http://codereview.appspot.com/4518055/diff/5003/source/blender/imbuf/IMB_imbuf.h
File source/blender/imbuf/IMB_imbuf.h (right):

http://codereview.appspot.com/4518055/diff/5003/source/blender/imbuf/IMB_imbu...
source/blender/imbuf/IMB_imbuf.h:254: void IMB_mask_clear(struct ImBuf *ibuf,
char *mask, int val);
I would use "bake" in the names here, in principle could be used for other
purposes. Maybe BAKE_MASK could be FILTER_MASK, and  functions
IMB_filter_mask_extend, and IMB_filter_mask_clear.

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

http://codereview.appspot.com/4518055/diff/5003/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_scene.c:2816: RNA_def_property_ui_text(prop,
"Smooth Heights", "Calculate heights against sub-divided lo-res mesh");
Regarding naming, maybe these properties could be enums, bake source and target?

[brechtvl, 2011-05-17 14:22:48]

http://codereview.appspot.com/4518055/diff/5003/source/blender/imbuf/IMB_imbuf.h
File source/blender/imbuf/IMB_imbuf.h (right):

http://codereview.appspot.com/4518055/diff/5003/source/blender/imbuf/IMB_imbu...
source/blender/imbuf/IMB_imbuf.h:254: void IMB_mask_clear(struct ImBuf *ibuf,
char *mask, int val);
Correction: meant to say would _not_ use "bake" in the names :)

[nazgul, 2011-05-18 15:22:54]

Next version of patch. Fixed issues reported by Brecht.

[brechtvl, 2011-05-19 09:57:29]

LGTM, though would still change the name of the flat heights option.

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

http://codereview.appspot.com/4518055/diff/14014/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_scene.c:2013: {RE_BAKE_SPEC_COLOR,
"SPEC_COLOR", 0, "Specular Colors", "Bake Specular colors"},
Looks like another patch got mixed in, probably harmless since this is already
in trunk.

http://codereview.appspot.com/4518055/diff/14014/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_scene.c:2822: RNA_def_property_ui_text(prop,
"Flat Heights", "Calculate heights against bare lo-res mesh");
Would change "bare lo-res mesh" to "unsubdivided low resolution mesh". "Flat
Heights" is still a weird name to me, how about "Bake to Low Resolution"?

[sparky, 2011-05-19 16:41:22]

> Heights" is still a weird name to me, how about "Bake to Low Resolution"?

I agree but I think people instinctively think of texture when they see "low
resolution". Perhaps we should make it "Bake to Low Resolution Mesh" or "Disable
subdivision on Low-Res" or something like that? As a programmer I'd prefer
"Piecewise flat low res" but I doubt that'd be popular? :)