Details

Reviewers

Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)

Group Reviewers

Cycles

Commits

rC2f17ad30d4f8: Cycles-Bake: Custom Baking passes
rBS9a76354585e2: Cycles-Bake: Custom Baking passes
rB9a76354585e2: Cycles-Bake: Custom Baking passes

Summary

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.

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

Campbell Barton (campbellbarton) added inline comments.Dec 18 2015, 11:36 AM

intern/cycles/kernel/kernel_types.h
372–412	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.

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.

This revision now requires changes to proceed.Dec 18 2015, 12:47 PM

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

@Sergey Sharybin (sergey) did you check on the latest version? I implemented it just like @Brecht Van Lommel (brecht) suggested.

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.
229	Sentence start with capital and ends wit ha fullstop.
243	Space after keyword Braces around single-operator block as well. See similar cases below.
327	Merge into single line.
337	`foo = (bar == JAZZ)`
intern/cycles/kernel/kernel_types.h
372	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
272	Just return.
source/blender/blenloader/intern/versioning_270.c
1039	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.

This revision now requires changes to proceed.Dec 18 2015, 1:44 PM

Typo: COlor > Color
From review: UI - remove box
From review: sorted code-style changes
From review: better tooltops (Deliver glossy pass > Add glossy contribution)

@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
1039	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).

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

From review: doversion for Cycles -- I had to duplicate the properties :/

Another option is to simply set pass_type to 'COMBINED'. Thoughts?

Madlaina Kalunder (Rkia) added a subscriber: Madlaina Kalunder (Rkia).Dec 21 2015, 1:27 PM

Error management of unsupported pass filter combinations

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.

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))

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	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–62	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.
222	Why this can't be const?
intern/cycles/kernel/kernel_types.h
382	Can see why `BAKE_FILTER_COMBINED` is split into lines, but why those are also split?
intern/cycles/render/bake.cpp
162	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
1039	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.

This revision now requires changes to proceed.Dec 22 2015, 2:46 PM

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)

From review: pass shader_filter as argument to the kernel_bake_evaluate function

@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 `!= 0`s in the rest of the patch let me know.
133	I'm removing this != as well

small code cleanups

minor Python notes.

intern/cycles/blender/addon/version_update.py
147	use a tuple if theres no intention to modify this.
174	picky `... is None`
179	picky - this could be set once outside the loop.

From review: small Python changes

Lapineige added a subscriber: Lapineige.Dec 24 2015, 8:27 PM

Typo in bake_pass_filter_check() DIFFUSE > DIRECT
Merge remote-tracking branch 'origin/master' into custom-bake (subversion bumped to 2.76.6 now)

Merge remote-tracking branch 'origin/master' into custom-bake

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.

@Sergey Sharybin (sergey) any news on that?

Will be checking updated patch soon (tonight or tomorrow afternoon).

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
147	Move it to an utility function, so actual `do_versions()` function is small and nice.
intern/cycles/kernel/kernel_types.h
373	One more question, is it indeed required to have `\` here?

Accepting, assuming that minor feedback is addressed.

This revision is now accepted and ready to land.Jan 13 2016, 9:01 AM

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.

Fixup from previous commit

Merge remote-tracking branch 'origin/master' into custom-bake

Closed by commit rB9a76354585e2: Cycles-Bake: Custom Baking passes (authored by Dalai Felinto (dfelinto)). · Explain WhyJan 15 2016, 5:39 PM

This revision was automatically updated to reflect the committed changes.

Brecht Van Lommel (brecht) mentioned this in T46550: Cycles bake black.Feb 12 2016, 11:25 AM

Sergey Sharybin (sergey) mentioned this in T47802: bake does not accept DIFFUSE_COLOR as parameter in 2.77 rc2.Mar 15 2016, 11:35 AM

Cycles-Bake: Custom Baking passClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 5618

intern/cycles/blender/addon/__init__.py

intern/cycles/blender/addon/engine.py

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/addon/version_update.py

intern/cycles/blender/blender_python.cpp

intern/cycles/blender/blender_session.h

intern/cycles/blender/blender_session.cpp

intern/cycles/device/device_cpu.cpp

intern/cycles/device/device_task.h

intern/cycles/device/device_task.cpp

intern/cycles/kernel/kernel.h

intern/cycles/kernel/kernel_bake.h

intern/cycles/kernel/kernel_types.h

intern/cycles/kernel/kernels/cpu/kernel.cpp

intern/cycles/kernel/kernels/cpu/kernel_avx.cpp

intern/cycles/kernel/kernels/cpu/kernel_avx2.cpp

intern/cycles/kernel/kernels/cpu/kernel_sse2.cpp

intern/cycles/kernel/kernels/cpu/kernel_sse3.cpp

intern/cycles/kernel/kernels/cpu/kernel_sse41.cpp

intern/cycles/kernel/kernels/cuda/kernel.cu

intern/cycles/kernel/kernels/opencl/kernel.cl

intern/cycles/render/bake.h

intern/cycles/render/bake.cpp

source/blender/blenkernel/BKE_blender.h

source/blender/blenkernel/intern/scene.c

source/blender/blenloader/intern/versioning_270.c

source/blender/editors/object/object_bake_api.c

source/blender/makesdna/DNA_scene_types.h

source/blender/makesrna/RNA_enum_types.h

source/blender/makesrna/intern/rna_render.c

source/blender/makesrna/intern/rna_scene.c

source/blender/render/extern/include/RE_bake.h

source/blender/render/extern/include/RE_engine.h

source/blender/render/intern/source/external_engine.c

Cycles-Bake: Custom Baking pass
ClosedPublic
Actions

Revision Contents
Changeset List

intern/cycles/blender/addon/init.py