Cycles: Added Cryptomatte passes.
Needs ReviewPublic

Authored by Stefan Werner (swerner) on Thu, Jul 12, 10:37 PM.

Details

Reviewers
None
Group Reviewers
Cycles
Summary

This patch introduces Cryptomatte output passes to Cycles.

There are two modes:
"Accurate" mode, which is CPU only, tracks all
IDs encountered in a pixel before it sorts them by coverage and strips
any that exceed the number of selected Cryptomatte layers. Since that
relies on dynamic memory allocations, it is CPU only.
The other mode keeps track of no more IDs than the requested number of
layers. When the number of IDs for a given pixel exceed the number of
layers, it is possible that an ID with a higher coverage gets rejected
in favor of an ID with lower coverage, if the latter was sampled first.
It would be good improvement in the future to allow GPU renders too to
track any number of IDs per pixel, regardless of the number of
Cryptomatte layers.

In order to support a user-selectable number of Cryptomatte layers,
Cycles passses are now not only distinguished by type but also by name.
That way, any number of passes of type PASS_CRYPTOMATTE can be added
to a render. Going forward, this could become useful for other types too,
should we introduce light path expressions or custom AOVs.

Diff Detail

Repository
rB Blender
Branch
cycles_cryptomatte
Build Status
Buildable 1831
Build 1831: arc lint + arc unit
  • Cycles: Code styling fixes for murmurhash.
  • Cycles: Fixed Cryptomatte pass names to match between Python and C++ code.

@Brecht Van Lommel (brecht) This patch changes array<Pass> back to vector<Pass>, as it was before you did some refactoring. Is that safe to do or are there dangers that I'm overlooking? Since I'd like Pass to contain a name string, it would be important that its destructor gets called.

  • Fixed indentation.
intern/cycles/kernel/kernel_passes.h
325

I wouldn't mind finding a prettier way of writing this. Repeating similar but not identical code three times with macros in it doesn't look too good to my eye.

Still to do:

  • Cryptomattes written by the stochastic (non-accurate) mode should be sorted by coverage.
  • A JSON manifest should be written to the EXR file.
  • When writing to a file it should be ensured that the Cryptomatte passes remain in float32 precision and don't get reduced half16.

Seems like a solid implementation, I've got some inline comments though.

Edit: Also, we might want to consider hiding these passes in the Image Editor, there's not really any point in looking at them directly.

intern/cycles/blender/addon/properties.py
1357

I'm not really a fan of "depth" for this parameter, especially since it might get confused with depth passes...
Maybe "layers" or "slots" would be better?

1365

Typo

intern/cycles/blender/addon/ui.py
575

I'd put depth over accurate mode - it appears to be the more important option (also, accurate isn't always available).

intern/cycles/blender/blender_object.cpp
403

Hm, I'm not really sure about the asset pass - as far as I can see it just stores the highest parent of the object? That seems a bit specific, but I guess it doesn't hurt...

intern/cycles/blender/blender_session.cpp
702

If we're relying on the name to distinguish passes anyways, we might as well just get rid of the pass_type. We're currently mapping back and forth between names and types all over the place, just going for unique identifiers would let us clean up a lot of stuff.

intern/cycles/blender/blender_sync.cpp
43

Is this naming scheme part of the Cryptomatte standard? So far all the passes have regular names, the u prefix doesn't really fit.

intern/cycles/device/device_cpu.cpp
696

Could you just pass kg here instead of keeping a copy in coverage?

756

Is there a reason to declare coverage here instead of having it directly in path_trace?

intern/cycles/kernel/kernel_globals.h
23

Is this include needed?

intern/cycles/kernel/kernel_passes.h
80

I guess this can be removed? It only runs when init already is false...

325

Hm, yeah, cleaning it up is tricky. You could almost fit it into a function, but you can't just pass the pointer to the coverage because only CPU has it.

The best way I can see would be to move the duplicated code into a multi-line macro:

#ifdef __KERNEL_CPU__
#  define WRITE_CRYPTOMATTE(id, coverage_map) \
    if(kg->coverage_map) { \
        (*kg->coverage_map)[id] += matte_weight; \
    } else { \
        kernel_write_id_slots(cryptomatte_buffer, 2 * kernel_data.film.cryptomatte_depth, id, matte_weight, initialize_slots); \
        cryptomatte_buffer += kernel_data.film.cryptomatte_depth * 4; \
    }
#else
#  define WRITE_CRYPTOMATTE(id, coverage_map) \
    kernel_write_id_slots(cryptomatte_buffer, 2 * kernel_data.film.cryptomatte_depth, id, matte_weight, initialize_slots); \
    cryptomatte_buffer += kernel_data.film.cryptomatte_depth * 4;
#endif

if(kernel_data.film.use_cryptomatte & CRYPT_OBJECT) {
    WRITE_CRYPTOMATTE(object_cryptomatte_id(kg, sd->object), coverage_object);
}

if(kernel_data.film.use_cryptomatte & CRYPT_MATERIAL) {
    WRITE_CRYPTOMATTE(shader_cryptomatte_id(kg, sd->shader), coverage_material);
}

if(kernel_data.film.use_cryptomatte & CRYPT_ASSET) {
    WRITE_CRYPTOMATTE(object_cryptomatte_asset_id(kg, sd->object), coverage_asset);
}

Still not really pretty, but significantly shorter. You can get rid of another two lines by passing cryptomatte_buffer by pointer to kernel_write_id_slots and doing the increment in there.

intern/cycles/render/coverage.cpp
71

I'd move the pixel index to its own variable since it's the same for all three passes.

96

Same as above.

128

I'd prefer getting the pass stride from tile->buffers, seems less hacky.

intern/cycles/render/film.cpp
45

This is really nitpicky, but I'd prefer the && on the first line.

189

Same here. Actually, this one seems short enough for one line?

intern/cycles/blender/addon/properties.py
1357

I guess we could call it "levels". The Cryptomatte spec isn't explicit about naming, but they seem to use "layers" to describe full RGBA sets (= two levels), and we're already using "layers" in the same UI tab for Scene/Render layers.

intern/cycles/blender/blender_object.cpp
403

It is a very useful feature when you're pulling plenty of external resources into a scene - then you can easily separate background, characters, props, fx, etc. without having to generate long lists of names. It's also a type that's used in the specification examples and supported by Arnold and Vray for Maya.

intern/cycles/blender/blender_session.cpp
702

I think both names and IDs have their place. This way, I can have multiple passes of the same type, but don't have to handle every one individually. We have switch cases where we do things by type and there we need only one case for PASS_CRYPTOMATTE. If we went to pass names only, we'd have to replace the switch statement with a string parsing if() cascade.

When at some point in the future we introduce LPEs or user defined AOVs, types will allow the user to pick arbitrary names for those passes. If we use the name as the only identifier, we'd have to introduce naming conventions or some other means of encoding type into the name.

intern/cycles/blender/blender_sync.cpp
43

I picked the "uCrypto...." because that's what the Cryptomatte example file used. The current Cryptomatte specification actually makes no requirements for names at all, except for the two digit number at the end. I see that VRay calls them VRayCryptomatte, so I guess we could call them "CyclesCryptomatte".

intern/cycles/device/device_cpu.cpp
756

Not really, no...good point.

Stefan Werner (swerner) marked 15 inline comments as done.Fri, Jul 13, 2:37 PM
Stefan Werner (swerner) added inline comments.
intern/cycles/device/device_cpu.cpp
696

Either way. I kept a copy since kg is expected not to change between calls.

758

Support in the CPU split kernel is still missing.

  • Cycles: Addressed Lukas' comments for Cryptomatte output.