This pass is built based on the passes the user finds fit.
It is useful for lightmap baking, as well as non-view dependent effects
baking.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- custom-bake
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| intern/cycles/kernel/kernel_types.h | ||
|---|---|---|
| 373–413 | these should be parenthesized, getting build warning/error. /src/blender/intern/cycles/kernel/./kernel_bake.h:393:20: error: suggest parentheses around '+' in operand of '&' [-Werror=parentheses]
if((pass_filter & BAKE_FILTER_DIFFUSE_DIRECT) == BAKE_FILTER_DIFFUSE_DIRECT) | |
| source/blender/makesrna/intern/rna_scene.c | ||
| 3764–3813 | PROP_ENUM_FLAG could be used here to have RNA define many toggles in one property. | |
Comment Actions
This doesn't seem to be really intuitive change. You might still want to have decoupled direct, indirect and color passes. If we want to make it possible to merge some light contribution together it should be done in an intuitive for artists way.
Another thing is that we can do all this on bake_api.c level using diagram provided by @Brecht Van Lommel (brecht), keeping all this passes stuff local in blender and common for all possible engines we ever want to support.
And finally, i would really like to see exact usecases first and not try to build some trying-to-be-totally-flexible configuration system, because otherwise you'll just end up with some totally obscure thing to work with.
Comment Actions
- From review: silence warnings from Linux
- From review: use enum-flag
That takes care of the operator options. Now the only missing bit is to prevent baking when the pass_filter is used but not toggled properly
Comment Actions
@Sergey Sharybin (sergey) did you check on the latest version? I implemented it just like @Brecht Van Lommel (brecht) suggested.
Comment Actions
Code review: You're missing initialization of pass_filter for the new scenes.
Answering question:
- No, i didn't check patch on runtime, it fails to compile for me. While implementation seems to follow Brecht's feedback the interface is quite tricky to follow. Current tooltips does not give any clues about actual formula to use as well.
- L.emission and shader_emissive_eval(kg, &sd) are the different things. First one is accumulated emission across the path, second one is an own emission. For the purpose of the patch you can only ignore L.emission from the combined pass.
- Not sure how you're ignoring background. Bake ray always hitting object to my knowledge?
As for bake_api.c thing i mentioned before -- on a closer look it's more tricky due to mix of Cycles-only properties and DNA ones. This divergence is quite annoying, but there's no simple way to avoid it now i'm afraid.
| intern/cycles/blender/addon/ui.py | ||
|---|---|---|
| 1442 | Don't think box is really needed, it's only eating the space. | |
| intern/cycles/kernel/kernel_bake.h | ||
| 191 | No need in if-else statement. | |
| 235 | Sentence start with capital and ends wit ha fullstop. | |
| 249 |
See similar cases below. | |
| 333 | Merge into single line. | |
| 343 | foo = (bar == JAZZ) | |
| intern/cycles/kernel/kernel_types.h | ||
| 373 | You can define those as an enum value, so you can watch them from gdb. Also use: (A | B | C) instead of A + B + C. | |
| intern/cycles/render/bake.cpp | ||
| 273 | Just return. | |
| source/blender/blenloader/intern/versioning_270.c | ||
| 1033 | I'm real skeptical about adding version check without actual bump. You can also simply use negated RNA and keep default values initialzied to 0. | |
Comment Actions
- Typo: COlor > Color
- From review: UI - remove box
- From review: sorted code-style changes
- From review: better tooltops (Deliver glossy pass > Add glossy contribution)
Comment Actions
@Sergey Sharybin (sergey), all the points you raised are addressed. pass_filter is initialized in scene.c since the first patch, you probably overlooked it.
No, i didn't check patch on runtime, it fails to compile for me.
Strange, I built it with buildbot and it went fine in Linux and Windows (and I build in Mac).
Current tooltips does not give any clues about actual formula to use as well.
I renamed "Deliver diffuse pass" to "Add diffuse contribution". I hope it's more clear now.
| intern/cycles/blender/addon/ui.py | ||
|---|---|---|
| 1442 | ok, I removed the box from the normal settings as well then. | |
| source/blender/blenloader/intern/versioning_270.c | ||
| 1033 | What is to be skeptical about? It just works fine, and the code DNA_struct_elem_find is intended exactly for that. If we are to no longer use it, let's remove it then. And negated RNA won't work with enum-flags (and even if it did I think it complicates the bitwise checks along the code unnecessarily). | |
Comment Actions
I just realized that I need a doversion for files that were set to bake DIFFUSE | GLOSSY | TRANS | SSSS _DIRECT / INDIRECT / COLOR. I will do it later today
Comment Actions
- From review: doversion for Cycles -- I had to duplicate the properties :/
Another option is to simply set pass_type to 'COMBINED'. Thoughts?
Comment Actions
And finally, i would really like to see exact usecases first and not try to build some trying-to-be-totally-flexible configuration system, because otherwise you'll just end up with some totally obscure thing to work with.
This is a recap from our original problem:
When doing ordinary renders with Cycles, you can get diffuse direct and diffuse indirect passes with one render by filling out the checkboxes.
With Cycles baking, you only have a limited set of choices and you can't combine the passes freely.
Because of that limitation, our lightmap baking process contains a lot of workarounds and complications.
In our automated scripts we use the following method to get the indirect and direct light information without having to bake two full passes:
We bake a combined map and a color map and then subtract the color map via a node setup.
If the textures/materials are too dark, we lose light information due to the subtraction and overexposed black pixels.
What we would like to have is a possibility to choose which passes we would like to combine for our bake, similar to what the regular Cycles rendering is already offering.
Baking lightmaps is a widely required feature, especially for Game Development and a lot of other 3d projects. Another option would be a predefined set "lightmap" (like the "combined" option) for direct and indirect light.
But then again in our specific case we don't need to render the glossy or transmission passes.
Comment Actions
- Merge remote-tracking branch 'origin/master' into custom-bake
(got a fix in master that is required if anyone test the patch from the code here))
Comment Actions
Not really happy with the versioning stuff. We should not have custom properties just for the verisoning code. Suggested ways are in inline comments.
| intern/cycles/blender/addon/properties.py | ||
|---|---|---|
| 483 | I'd rather not have any deprecated stuff in the python API. If it's only needed to get proper enum flag after enum flags are changed think we should check on getting original value from the API. | |
| intern/cycles/blender/addon/version_update.py | ||
| 145 ↗ | (On Diff #5609) | Think we only need to set use direct/indirect here and set a proper type here. Alternatively, fallabck to a default bake type when we can't do it without having annoying "deprecated" stuff in the API. |
| intern/cycles/blender/blender_session.h | ||
| 55 | Time to split this into column, see other examples like BlenderSync::sync_object. | |
| intern/cycles/kernel/kernel_bake.h | ||
| 65 | Do we really need to add explicit != 0? | |
| 178–179 | Capital and fullstop. | |
| 228 | Why this can't be const? | |
| intern/cycles/kernel/kernel_types.h | ||
| 383 | Can see why BAKE_FILTER_COMBINED is split into lines, but why those are also split? | |
| intern/cycles/render/bake.cpp | ||
| 161 | This is sort of "don't try this at home". If we need to pass additional configuration stuff to kernel this is to be passed as an argument similar to bake type, not as a hidden setting in the per-pixel setting array. | |
| source/blender/blenloader/intern/versioning_270.c | ||
| 1033 | Because all the un-bumped versioning code is to be simply inside of {
/* .... */
}block. Also this "let's add subversion check just in case" only causes confusion whether someone forgot to bump something or simply had something else in mind. | |
Comment Actions
- From review: sentence-comments are to be capitalized and fullstop
- From review: remove explicit != 0 bitwise flag for sd->flag tests
- From review: splitting bake() into columns
- From review: sorted codestyle
- Revert "From review: doversion for Cycles -- I had to duplicate the properties :/"
- From review: better backward compatible (with subversion bump)
Comment Actions
@Sergey Sharybin (sergey) all the raised issues were handled, and the code still works fine after the changes (including backward compatibility).
| intern/cycles/blender/addon/properties.py | ||
|---|---|---|
| 483 | I changed this approach, and I'm now doing a subversion bump | |
| intern/cycles/kernel/kernel_bake.h | ||
| 65 | I guess we don't but this is how I'm doing elsewhere in this patch. It's also what we have in util_system.cpp, osl_services.cpp, blender_object.cpp. I'm fine either way, so I'm getting rid of != 0 here. If you prefer me to get rid of all the != 0s in the rest of the patch let me know. | |
| 133 | I'm removing this != as well | |
Comment Actions
- Typo in bake_pass_filter_check() DIFFUSE > DIRECT
- Merge remote-tracking branch 'origin/master' into custom-bake (subversion bumped to 2.76.6 now)
Comment Actions
- Merge remote-tracking branch 'origin/master' into custom-bake
(related cycles-bake fixes in master)
The patch is finished though, I'm just waiting for its review.
Comment Actions
Some style feedback. Changes in kernel seems fine, but i didn't really test those deep enough. Also assuming you've checked with artists about interface and such.
Other than that, think we can commit this and do any possible tweaks after that.
| intern/cycles/blender/addon/version_update.py | ||
|---|---|---|
| 163 ↗ | (On Diff #5736) | Move it to an utility function, so actual do_versions() function is small and nice. |
| intern/cycles/kernel/kernel_types.h | ||
| 374 | One more question, is it indeed required to have \ here? | |
Comment Actions
- Merge remote-tracking branch 'origin/master' into custom-bake
- From review: remove uneeded \
- From review: move doversion to its own function
As soon as I update the documentation, I will merge this in master.