Page MenuHome

Cycles-Bake: 1st round of code-review
AbandonedPublic

Authored by Campbell Barton (campbellbarton) on Mar 22 2014, 11:37 PM.

Details

Summary

For the individual commits:
https://github.com/dfelinto/blender-git/tree/bake-cycles-0.1

Documentation:
http://wiki.blender.org/index.php/User:Dfelinto/CyclesBaking

Open Issues:
https://github.com/dfelinto/blender-git/issues?labels=bake-cycles&page=1&state=open

Supported Features:

  • Margin
  • User Interface (no more need to fiddle with Python scripts)
  • Selected to Active
  • Cage Extrusion
  • Custom Cage
  • File Formats (save as PNG, JPG, OpenEXR, Tiff, Targa and BMP)
  • Color Depth/Mode: RGB/RGBA - 8/16/32
  • Normal swizzle (change the axis that gets mapped to RGB)
  • Normal space (save as tangent, world, object)

Supported Passes:

Data Passes

  • Normal
  • UV
  • Diffuse/Glossy/Transmission/Subsurface/Emit Color

Light Passes

  • AO
  • Combined
  • Shadow
  • Diffuse/Glossy/Transmission/Subsurface/Emit Direct/Indirect
  • Environment

Implementation Notes:

Baking for Cycles was implemented in two fronts:

In Blender we created a new operator that will try to bake using the current engine.
So far only Cycles has this implemented, but other engines (even the Blender Internal) could go through this API/pipeline.
The operator is responsible for the image handling, normal space
transformation, populate the UVs, loop over faces, ...

In Cycles we extended the baking functionality available for environment
and displacement for the other render passes (not all of them since some
of them doesn't make sense for baking).

The API is simple. Blender send a populated array of BakePixels to the
renderer, and gets back an array of floats with the result.

BakePixel is tjhe following struct:

struct BakePixel {

int primitive_id;
float u, v;
float dudx, dudy;
float dvdx, dvdy;

};

Diff Detail

Branch
bake-cycles

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 11 2014, 12:11 AM
  • Cycles-Bake: [review] ui nitty-picking from Thomas Dinges
source/blender/editors/object/object_bake_api.c
132

yep, comparing a char to a number doesnt need a cast. and the result is a bool.

280

Ack! when adding the first comment I thought this was commented, working code, not psudo-code.

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 11 2014, 12:42 AM
  • Cycles-Bake: remove pseudo-code
  • Cycles-Bake: [review] nitty-pick, removing unneded cast

CUDA builds, but CUDA is definitively broken for baking:
http://blenderartists.org/forum/showthread.php?326534-Cycles-baking-is-here&p=2621905&viewfull=1#post2621905

I have my hands a bit tied here. I don't own a NVidia/CUDA card so it's hard to even start on debugging those. I'm glad CUDA works (so Blender itself can be built with CUDA), but maybe we should disable it for baking.

Disabling CUDA for baking is fine with me, we can fix and enable that later.

Disabling CUDA for baking is fine with me, we can fix and enable that later.

For the records:

  • Internal Saving

As per Campbell's suggestion I'm implementing internal saving of baking maps. Which means the map gets saved to an image datablock. I'll be using ED_object_get_active_image to determine the image to bake to. This is the same function projection painting and viewport preview TEXTURED mode uses (when in Cycles).

I have this working for single material objects, but it will take a bit of restructuring in the code to get it working for multiple materials (different images can have different dimensions, the zspan part will need to change a bit then).

  • Multiple High-Poly objects

Users need to be able to select a few objects and only one low poly object. In order to support that I will need to expand BVH and add a new function (bvhtree_from_multiple_mesh_faces) so we can ray cast getting the object/mesh id too. That will also mean that the engine.bake() will be called multiple times (once per high-poly object).

  • Final Considerations and future phases

Both of the changes above will require only changes in the Blender-side of the code. I'll try to take both in consideration simultaneously. Even though the actual BVH change will have to wait, since it smells like a time-drainer for someone inexperienced in BVH like myself.

Apart from them we should be pretty close to the 'phase 1' (mimic blender internal behaviour). Phase 2 would be to improve the workflow (batch baking, object or material bake maps, ...). Phase 3 would be to pipe blender internal baking into the same API. After phase 1 is complete we can merge it to master. Phase 2 can even wait if we want to give people a proper time to test the features and provide usability feedback. For instance I can go work with multiview after phase 1 is in master, if Ton & Campbell agree/want. It all depends on what they consider to be the priority for my allocated time, so not entirely up to me.

All that said, we are still in schedule for 2.71 (phase 1), but my hopes of merging this week are fading away ;)

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 17 2014, 3:18 AM
  • BLI_path_suffix() - new path util functon to add a suffix to a filepath (before the extension)
  • Cycles-Bake: internal saving and split image per material
  • Cycles-Bake: code cleanup (goto, return)

Note: I also updated it to latest master.
Note: We now support internal saving (as per @Campbell Barton (campbellbarton) suggestion)
Note: The only show-stopper at the moment is "select more than one object as high-poly", which is what will require the changes in BVH to allow more than one mesh and the hit point to return which mesh was hit.

I will probably only be able to work on that next week, so if anyone wants to do the BVH changes, I will handle the part in the operator code and would deeply appreciate that (one can wish ;)

Note: I did a few changes in the UI without much thinking. I'm open to suggestions here (once thing for example, the "Automtic Name" (use_automatic_name) property could be something like "Prefix by Map Type" or similar.

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 17 2014, 8:03 PM
  • Cycles-Bake: fix for windows builds
Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 25 2014, 7:11 AM
  • Avoid double property lookups in bake_invoke (by @Campbell Barton (campbellbarton))
  • Cycles-Bake: Support for multiple high-poly objects selected at once
  • Cycles-Bake: Tangent baking from multires
  • Cycles-Bake: silence rna warning and code cleanup

Note 1: I believe we are now feature complete.
Note 2: I migrated the development over to https://developer.blender.org/diffusion/B/browse/bake-cycles/ (git.blender.org bake-cycles branch)

Hey Dalai, I showed the baking at FMX yesterday, works nice.

Just 2 notes/questions:
Internal Output: I get „No valid images in material 0 (Material)“, where do I need to add an image? That is not clear to me. Maybe it should just create a new texture automatically?

If the selected object has no assigned material, baking will not work. A error message should be added here, it fails without feedback atm.

Hey Dalai,
build kicked on win7.x64 / scons...
but I think a fresh merge may help (not sure tho, I know not what I do :p)
Here's the log http://www.pasteall.org/51123/text

Happy coding,
~Tung

@Thomas Dinges (dingto) the no "valid images in material" means exactly that. The criteria to have an active image in Cycles is different than BI. In Cycles viewport (and in projection painting) we take the active Image Texture node of the material. Basically we are using ED_object_get_active_image().

The bug when there is no material is new to me, I'm looking at it at the moment, thanks for spotting it.

@tung ster (tungster) I merged with master again, see if it works now. I tested in OSX and Linux and both built fine.

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 25 2014, 7:13 PM
  • Merge remote-tracking branch 'upstream/master' into bake-cycles
  • Cycles-Bake: bugfix for when no material is present
Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 25 2014, 7:41 PM
  • Cycles-Bake: fix building in WindowsXP 32bits SP3
Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 26 2014, 2:51 AM
  • Cycles-Bake: UI changes: removing External Saving options
  • IMB buffer functions to take a mask as input
  • Cycles-Bake: "Clear" option fully supported

Yo Dalai,
msvc still not happy (ô¿ô)
http://www.pasteall.org/51137/text

Cheers bro

The Cycles part looks good to me now, just minor style comments.

intern/cycles/blender/blender_session.cpp
266–271

I prefer not to use macros for this kind of thing, it's just two lines.

548

Ok, nevermind then.

intern/cycles/render/bake.cpp
51

Code style convention is to have { on the next line.

176

Code style convention is to have { and } on separate lines.

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 26 2014, 6:23 PM
  • build fix for windows after 68893054 (IMB buffer functions to take a mask as input)
  • Cycles-Bake: [review] Cycles style cleanup
  • Revert "Cycles-Bake: [review] using MACRO in get_shader_type()"
  • changes to b2ffdf99 (BLI_path_suffix()) - to be squashed
  • Cycles-Bake: cleanup (removing uneeded function)
  • Cycles-Bake: better support for multiple materials referring to the same image

Committed some cleanup to baking:

rB350d253a4132e8af9ab5b23ea8e5cdeb3e4b3f4c
rB01dd80bb0ce320d03daeaa5cecd4e3b092f64789
rBc4b963f63a54385f53518e19e5018367fba4f819
rB6b655a1f241f1ff3f65c95f0eb972e6fe9286663

However Im not able to get baking working in the most simple case I could try,

(set the image to use any PNG, I tried 2 and both gave same problem).

As well as not working, if I press Bake, it crashes sometimes (approx one in five), see output from address sanitizer.

Tested this before and after my changes to ensure I didnt cause it.

Campbell Barton (campbellbarton) requested changes to this revision.Apr 27 2014, 12:55 PM

msvc/scons kick with recent build,
error = http://www.pasteall.org/51159/text

Ummm...
Scrap, Shread, Burn that last comment.
I don't know wth is going on (ô¿ô)
It does compile.
I'm officially.... Psycho!

Sorry for the noise.

Cheers,
~Tung

@Campbell Barton (campbellbarton) What happened is:

  1. I did a poor job on getting rid of External in the UI. Even though I removed it from the UI, it still was the default elsewhere. R_BAKE_SAVE_INTERNAL is the default now. It worked for me because I saved save mode for INTERNAL in my test files, and forgot to test in a blank new file.
  2. My last commits ignored External as an option. So it is broken at the moment. I'm looking at it now, either way you should be testing 'INTERNAL' only.

As for test files this is one of my latest ones:


That's why I didn't want to rush the final commits, too many parts that can go wrong if I don't test it throughly

Tested and works well, one last thing,

You can't currently cancel a bake (and bakes - like renders, can take a long time),
this seems fairly important before including it in a release.

Dalai, Brecht - any thaights on this?

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 27 2014, 11:23 PM
  • Merge remote-tracking branch 'upstream/master' into bake-cycles
  • external saving is working again (though there is no UI)
  • other cleanups from Campbell (I don't know why arcanist is not listing them here)

It would be nice to be able to cancel as well as to see the progress. But I'm not sure what it takes to get this working. I'll look at how rendering is implementing that (the cancelling at least) - note: there is already some code for cancelling in place, but it's not all that it's needed.

@Dalai Felinto (dfelinto), IMHO realtime preview is nice-to-have, but a bake can take 30min, or ... as long as a render.

Ability to cancel is essential IMHO.

I didn't mean preview, but a percentage progress bar, or printing to the consloe

Did some quick review. The patch contains loads of unrelated changes. Make sure you don't include them into the final patch.

Two main concerns (which are not stoppers for merge, but which would need to be addressed):

  • Colormanagement. Not sure how to make it more clear, but it tights render engines to a legacy CM a bit stronger. We really would need to make RE aware of scene linear space and so.
  • We now do have three bakers which don't reuse anything from each others: BI, multires and cycles. Seems it's possible to share some code between cycles and multires bakers, or even make it more generic and support both bakers via the same api.
intern/cycles/kernel/geom/geom_curve.h
785 ↗(On Diff #1567)

If it's not needed anymore just delete. Don't leave a dead code.

release/scripts/modules/bpy/ops.py
204 ↗(On Diff #1567)

Same as above. Don't keep dead code.

release/scripts/startup/bl_ui/space_time.py
134 ↗(On Diff #1567)

Seems to be unrelated change?

source/blender/blenkernel/intern/DerivedMesh.c
44 ↗(On Diff #1567)

Make sure you don't have unrelated changes in the final patch. Can see quite a few of this.

source/blender/blenkernel/intern/mesh.c
2093

Picky: use enum values for the argument.
Also better use bool instead of ints here.

source/blender/editors/object/object_bake_api.c
128

I don't see a reason to use IMB_buffer_float_from_float() here unless buffer might have different number of channels (which doesn't seem to be a case?). Use memcpy here.

187

Same as above.

818

Not sure why we need to check properties here? Seems this is only invoked from operator which have all this props?

@Sergey Sharybin (sergey) see my inline replies. As for the unrelated changes you can see that arcanist is listing a few unrelated commits in Local Commits. Either way I'll make sure they are not included in the final merge.

  • Colormanagement: what is the idea? to use something other than IB_PROFILE_LINEAR_RGB? It doesn't seem a problem specific of my code though, since I'm pretty sure I based this of somewhere else.
  • Three baking systems: The idea is to have the Blender internal also to benefit from the bake api (so it can use cage, normal swizzling, saving externally, ...). Multires is a special case. For "Baking API" every time the user bakes a tangent map with no "highpoly" object selected, I'm assuming it can be a multires baking. All I do differently is to generate a low-res mesh without a multi-res modifier active and use its tangent/normals for the final map.
intern/cycles/kernel/geom/geom_curve.h
785 ↗(On Diff #1567)

This was introduced in a2489e29 (in master) and I don't know why it's listed here in arcanist.

release/scripts/modules/bpy/ops.py
204 ↗(On Diff #1567)

same as above

release/scripts/startup/bl_ui/space_time.py
134 ↗(On Diff #1567)

same as above

source/blender/blenkernel/intern/DerivedMesh.c
44 ↗(On Diff #1567)

same as above

source/blender/blenkernel/intern/mesh.c
2093

I agree here, but I merely copied the prototype of the original function, without addressing those changes. So although I see it would be nice to do those changes, I would rather have them as a cleanup after the initial merge.

source/blender/editors/object/object_bake_api.c
128

The *buffer always has 4 channels. The ibuf not necessarily since it's created by the user previously and may or may not have alpha, BW, .... I didn't try BW buffers, but I know as a fact that alpha or not alpha both work fine.

187

See my reply above. Here we also may or may not have alpha depending on the image format settings option.

818

The idea was to allow it to be called from the UI, by exposing the operator properties directly (instead of the scene.bake settings). This can help addon-makers to try different designs/workflows for baking.

Hi, chiming in with an idea for UI:

This is how I handled multiple per material texture slots in the paint branch, maybe you could borrow this for the bake target approach:

So I have a list with the object materials and then a list with the possible texture slots to paint on below.

This could work similarly for baking but with a templateID with images instead of paint slots.

Of course materials with no bake target get no baking. Simple and clean.

The idea of color management is to get rid of IB_PROFILE_* and also make renderers aware of scene linear space. This is not directly related with the current patch and mostly to be kept in memory. What i don't like is that you're adding more functions (_mask things) which are relying on the legacy profile flags. This is just wrong and not gonna to work. Use new color management instead. See the inlined comment.

Multires baker is not only about tangent maps, but AO and displacement as well. We might still keep it as a rather separate api, but i saw some functions which potentially could be re-used between all the bakers (mainly image tessellation, derivatives computation and so). Wouldn't consider this a stopper for merge, but it's for sure need to be clarified up in the future (and not future like 10years from now ;)

source/blender/editors/object/object_bake_api.c
128

None of the bakers allows to baking to non-RGBA float. This is better to be kept consistent across all bakers. We can either disable non-RGBA here or do the same trick for other bakers.

But still bet RGBA is more likely a common case for baking and fr this we can memcpy easily. Would save quite reasonable amount of time, especially when doing hires baking.

134

On the second thought, the code above would give expected results from the user POV (just something which could be optimized and got rid of legacy CM code).

This call is totally wrong.You can not assume byte image is in sRGB space, it is in the space of imbuf->rect_colorspace. What you can do is to use IMB_colormanagement_colorspace_to_scene_linear() to convert buffer to rect_colorspace space and then use IMB_buffer_byte_from_float(.., IB_PROFILE_SRGB, IB_PROFILE_SRGB) to convert float to byte (note that the same profile is to be used for target and source). This is the only way to produce image in the proper color space.

Rule of thumb: NEVER-EVER use IMB_buffer_XXX_from_YYY to change color space now.

Sergey Sharybin (sergey) requested changes to this revision.Apr 29 2014, 12:21 PM

Easier idea of how to make imbuf in the proper space: temporary assign float buffer and call IMB_rect_from_float, then set float buffer to NULL.

Still not ideal coz you'll have float buffers hanging around during the bake which is a separate topic actually..

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 29 2014, 6:51 PM
  • Cycles-Bake: small fix for external saving + cleanup
  • Cycles-Bake: Cycles changes for shader evaluation to support cancelling via progress cancel callback
  • Cycles-Bake: Support for 'cancelling' the bake (Esc)
  • Cycles-Bake: force saving to be always external
  • Cycles-Bake: bring back the SHADOW pass
Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 29 2014, 6:57 PM
  • Merge remote-tracking branch 'upstream/master' into bake-cycles

@Campbell Barton (campbellbarton) this is in my list as the last thing I was planning to visit. There are two separate issues:

  1. when we export -X -Y -Z is that actually -X -Y -Z, or it's something else? (e.g., +X +Y +Z).
  2. what is the correct swizzle we should use for tangent, object, ...? (e.g., I think right now you get correct tangent maps if you bake -X -Y -Z)

Since this require testing and may even need me to look at xNormals, I pushed it for later. The code itself is trivial to fix once I get what the code is supposed to reproduce.

Anyways, my idea was to look at BI and see what it's producing. But IIRC Blender internal uses a different convention than Cycles. See Cycles Normal Map node. At least Object Space seems to be different for Cycles and BI.

I'll address it soon.
(for the records: @Felix Schlitter (dalai) is not me, apparently someone uses my name as nickname)

geez I got scared now. I thought I had pushed this to master. re-opening, not sure why it was automatically closed.

Sorry for the noise :/
I ran a 2nd pass and it gave a lil' extra info as well (weird, didn't change anything ) = http://www.pasteall.org/51215/text

msvc2008/scons makesrna.exe crash in compile = http://www.pasteall.org/51219/text

Sorry Dalai,
seems trunks related same thing in another branch as well

@Sergey Sharybin (sergey) It took me awhile to understand that you were proposing the opposite transformations that I believe are necessary. The buffer is already in linear space, it comes straight from the render engine (Cycles).

Anyways, see if this is correct: 3ff708a 3ff708a e5c6ca3 . The last one (e5c6ca3) I'm not so sure about, it feels like it's missing something. There is also something strange when I use an image datablock that is 'Non-Color'. It bakes, the data is there (I can save as and it works), but it shows all black. Not sure if it's a bug in Blender or in my code.

@Brecht Van Lommel (brecht) any thoughts on this? 02fc6ef1 (Cycles changes to support cancelling on kernel evaluation)

Dalai Felinto (dfelinto) updated this revision to Unknown Object (????).Apr 30 2014, 12:47 AM
  • Cycles-Bake: fix windows build
  • IMB buffer _mask functions shouldn't convert colorspaces (ammend to
  • Cycles-Bake: using proper colormanagement friendly routines
  • Cycles-Bake: skipping color management transformations for data passes (e.g., Normal)

committed on 56d8aff 198f5e5 97641a0 3312b20 eec3eab
I forgot to reset the date of the commits so the git rebase squash messed up their orders a bit ... but it's all there.

Thanks everyone for the reviews, closing the issue now