Autodesk / sitoa

Arnold plugin for Softimage
Apache License 2.0
33 stars 16 forks source link

Add AOV shaders #13

Closed JenusL closed 5 years ago

JenusL commented 6 years ago

This has been discussed in the mailing list and there's an old trac ticket for it as well. I think the conclusion was to add it to the Output Shaders. This will add Cryptomatte support.

caron commented 6 years ago

I started added support for AOV Shaders and its a bit tricky...

Is it true only one background and one volume shader connection is supported?

I started using the existing 'UpdateShader' and 'CNodeSetter::SetPointer' functions used for the background and volume shaders but I am not sure they support AtArrays. Whenever I use those functions I don't get the correct result in the .ass file export. But when I wrote my own creating an AtArray, adding AtNode, and setting the array with AiNodeSetArray I get this in the .ass file.

options
{
 AA_seed 74
 AA_samples_max 8
 skip_license_check on
 error_color_bad_pixel 0 1 0
 outputs 4 1 STRING
  "RGBA RGB sitoa_output_filter Passes.Default_Pass.Main"
  "Crypto_Asset RGBA sitoa_output_filter Passes.Default_Pass.Arnold_Crypto_Asset"
  "Crypto_Object RGBA sitoa_output_filter Passes.Default_Pass.Arnold_Crypto_Object"
  "Crypto_Material RGBA sitoa_output_filter Passes.Default_Pass.Arnold_Crypto_Material"
 aov_shaders "Passes.Default_Pass.cryptomatte.SItoA.74000.2"
 xres 1920
 yres 1080
...
}

Weird is the sample .ass files from the cryptomatte repository show something like this for the aov_shaders parameter.

aov_shaders 1 1 NODE
  "cryptomatte1"

Also, some shaders which use the 'closure' node don't connect to the output shader stack, gotta see about changing that output port type.

kikou commented 6 years ago

Is it true only one background and one volume shader connection is supported?

Yes

Weird is the sample .ass files from the cryptomatte repository show something like this for the aov_shaders parameter.

I believe both ways are supported in the .ass syntax (single element array vs single value)

caron commented 6 years ago

Thanks, both are using the API to write the ass file, not sure why the result would be different.

I will try writing multiple items and see if it switches

caron commented 6 years ago

OK, I believe I have it working...

 aov_shaders 2 1 NODE
"Passes.Default_Pass.closure.SItoA.69000.2" "Passes.Default_Pass.cryptomatte.SItoA.69000.5"

I still need to test the output of the Cryptomatte just to be sure all is working well. But to test the AOV shaders generically I added a noise shader connected to an aov_write_rgb shader and the output looked good to me. but still two things to discuss about implementation...

https://github.com/caron/sitoa/commit/a74b867109ccefc896a46de5997cde2b2e5271ca

I had to change the SItoA 'closure' node to add the 'texture' family. Otherwise it wouldn't connect to the output shader ports. I don't know yet what side effects this may have.

It seems NodeSetter helper class doesn't support AiNodeSetArray, so should we add that functionality for this one case? Or just let this be?

https://github.com/Autodesk/sitoa/blob/6429823d9471a7f5fe88c64918ce85b796489add/plugins/sitoa/common/NodeSetter.cpp#L12-L14

JenusL commented 6 years ago

I think it looks good. Just two notes / questions:

  1. Can't it be siShaderFamilyOutput instead of siShaderFamilyTexture? Or is the output type something completely different? I'm on vacation and can't test for myself.
  2. I see both Background and Volume pass shader stack have a suffix of ".Item". Do we need a suffix on the output shader stack as well?

@sjannuz is probably the best person to answer the NodeSetter question.

meanleybh commented 6 years ago

@caron , I can confirm here that our own build of sitoa-4.1.x writes out the aov_shaders entry as a single value as you indicated above (rather than an array). We aren't really using it anymore, but it worked fine for us in production when we were.

caron commented 6 years ago

@meanleybh , Thanks Brian! Ya, if you only want to use it for cryptomatte (most users) then a single shader is fine. It's not hard to add the support for multiple aov_shaders and it's nice to add all your various utility stuff here instead of on each shader or as a separate pass.

@JenusL I looked quickly at the siShaderFamilyOutput in the docs and it shows its only for camera shaders? I will have to try it and see.

http://download.autodesk.com/global/docs/softimage2014/en_us/sdkguide/si_om/siShaderFamilyType.html

mental ray Output Shader (output phenomenon in a camera)

Regarding 'Item' suffix part of the code... background and volume shader connections on the options node only support one connection. So those can just look for the first 'Item' in the ShaderArrayParameter. The aov shaders on the other hand support multiple shaders connected, so I need to loop over the ShaderArrayParameter to collect all the 'Items' and shaders connected to them.

caron commented 6 years ago

OK, so siShaderFamilyOutput seems to work fine. I also did some work to protect against empty items in the shader stack. Running testsuite now and should be ready for others to try.

caron commented 6 years ago

OK, most tests passed but I had some issues with the noicon.pic not rendering for some of the tests and the mesh lights test 161 looked messed up. I will look into those before sending a PR... but if anyone wants to try it before then.

https://github.com/caron/sitoa/tree/develop

caron commented 6 years ago

@JenusL already ran into test 161 failing... will ignore for now #11

JenusL commented 6 years ago

@caron I also had problems with noicon.pic when I ran the tests at first. I think it was just the Arnold texture path that wasn't set to the correct Softimage path or something because of different Softimage versions. And I can't remember how I fixed it either… I'm still on vacation so can't test anything for at least another week.

caron commented 6 years ago

@JenusL no worries, enjoy your time off!

sjannuz commented 5 years ago

Merged. Sorry for the delay. Thankyou !