Autodesk / sitoa

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

Arnold Denoiser #53

Closed JenusL closed 5 years ago

JenusL commented 5 years ago

So here it is. The first "draft" of Arnold Denoiser for SItoA. I would REALLY like to get some feedback on this because I'm not 100% happy with it yet. Here's what I've done.

This PR also contains the commits of the Optix Denoiser because I needed the Denoiser tab I created there. So it would be good to merge #50 before this so we can resolve any conflicts.

sjannuz commented 5 years ago

I tried the branch, working as advertised. One minor defect is when you select a sequence as output (ex. XXX[1..4].exr), then the output sequence does not seem to be compiled correctly (XXX[1,_denoised.4].exr). On running Denoise, no output is generated, regardless of the Frame Range mode. About the AOVs, in Max the confusion is due to the main output being always saved from Max itself, not from Arnold. In theory you should be able to attach the auxiliary AOVs to the Softimage main.

I'd like to proceed with a release by the beginning of the new year. Would you like to have this included, or keep it unmerged ?

JenusL commented 5 years ago

Thanks for testing @sjannuz I'll fix that issue and look at how the AOVs are outputted. I'll probably rewrite it to put them together with Main as in MtoA. I just need to figure out if I can deal with mixed bit depths (16 and 32-bit float) and renaming of AOVs if they already exist. Would probably be nice to have this in the release so if you can wait a couple of weeks that would be nice. I'm still on holiday but will fix these things next week.

JenusL commented 5 years ago

@sjannuz I can't replicate the issue you had with the output. Could you give me more details of what you did to break the output?

JenusL commented 5 years ago

Ok so I rewrote the output of the Denoising AOVs so that they are outputed to the same exr as main. If the AOV already exist, it will be renamed with a "_noice" suffix. For example, if N exist but have the closest_filter, N_noice will be added with correct output filter. One drawback of this is that Noice wont choose that layer in the exr, but will instead choose the one without the suffix. Optimal would be that Noice would choose a feature AOV with the suffix first and fallback to the original if it's not found. Any thoughts on that @sjannuz?

I also made some enhancements to the PPG and it should handle image sequences a little better now.

This passes the testsuite and closes #14 and closes #34

sjannuz commented 5 years ago

@JenusL , see in this video what I was talking about. I think we could unify and simplify the UI, by completely removing the Frame Range, Start and End Frame. The Input field should be enough to cover all the cases. Like, in my example, I obviously want to denoise all the sequence. I would pick a single file for a single output, or edit the [1..4] token for a different range. If so, Output should simply become a suffix to be added to the Input path (ex, default to "_denoised", and my output would then become Beauty_denoised[1..4] under the hood. We lose the ability to save into another directory, in favor of a cleaner UI. Also, I am not sure about your pick to rename the noice AOVs. I would probably go the other way around, that is renaming the existing AOVs (if any), for instance N -> N_main (with a warning), and preserve the original names for the AOVs consumed by noice. I need to check with core, but I think the name matters indeed.

JenusL commented 5 years ago

@sjannuz Thanks for the video! I see the bug now. My regex for parsing Softimage sequences is only tested for padded sequences, ie [1..4;4] and not just unpadded like yours [1..4]. I will fix that later today. In the mean time, could you rename your Beauty to have padded frames and try out the rest of the functionality. If you still think it needs simplification after that, please let me know.

Renaming the existing AOVs shouldn't be a problem so I could do that but I'd also like to hear what the core team thinks first.

sjannuz commented 5 years ago

Yes, adding the padding fixes the input sequence. And, for the AOV names, I take it back. I checked with Ramon, what noice does is picking the right AOV based on the matching filter type, so the name does not matter.

JenusL commented 5 years ago

Ok. I've fixed the Softimage sequence parsing now and in the process I made the padding more robust allround.

@sjannuz Yeah I suspected that Noice did use some undocumented black magic (metadata) to detect the AOVs :) Would be good if Noice could spit out the name of the exr layer so there's no doubt of what it's using. I did a test where I exported a scene to .ass and renamed the original N and Z that use the closest_filter to N_main and Z_main just to check what noice did.

outputs 7 1 STRING
  "RGBA RGBA sitoa_output_filter Passes.Default_Pass.Main"
  "N VECTOR sitoa_closest_filter Passes.Default_Pass.Main N_main"
  "Z FLOAT sitoa_closest_filter Passes.Default_Pass.Main Z_main"
  "diffuse_albedo RGB sitoa_output_filter Passes.Default_Pass.Main"
  "N VECTOR sitoa_output_filter Passes.Default_Pass.Main N_noice"
  "Z FLOAT sitoa_output_filter Passes.Default_Pass.Main Z_noice"
  "RGB RGB sitoa_variance_filter Passes.Default_Pass.Main variance"

And here's the Noice output:

Using feature AOV 'diffuse_albedo' with filter 'gaussian_filter'
Using feature AOV 'N' with filter 'gaussian_filter'
Using feature AOV 'Z' with filter 'gaussian_filter'

Would be nicer if the output from Noice would say something like this:

Using feature AOV 'diffuse_albedo' with filter 'gaussian_filter' (exr layer: diffuse_albedo)
Using feature AOV 'N' with filter 'gaussian_filter' (exr layer: N_noice)
Using feature AOV 'Z' with filter 'gaussian_filter' (exr layer: Z_noice)

As a final test I removed only Z_noice from the outputs, but was surprised by the result of Noice:

Using feature AOV 'diffuse_albedo'
    Filters for feature and light AOVs are different: denoising works better with matching filters.
Using feature AOV 'N'
    Filters for feature and light AOVs are different: denoising works better with matching filters.
Using feature AOV 'Z'
    Filters for feature and light AOVs are different: denoising works better with matching filters.

I kind of expected this result:

Using feature AOV 'diffuse_albedo' with filter 'gaussian_filter'
Using feature AOV 'N' with filter 'gaussian_filter'
Using feature AOV 'Z'
    Filters for feature and light AOVs are different: denoising works better with matching filters.

Feel free to forward these notes to the core team :)

So now that the renaming of the AOVs are ok, do you think I should change the severity of the message to Info or do you prefer to keep it as Warning?

sjannuz commented 5 years ago

Ok @JenusL , forwarding your notes to core. It's an unlikely scenerio, but worth to be logged. Any thoughts about my proposal to simplify the UI removing the output sequence controls ?

rmv commented 5 years ago

Thanks for the noice feedback JenusL, good point about using exr layer name and the second one might be a bug when reporting what AOV was used. Thanks!

JenusL commented 5 years ago

@sjannuz I'm all for simplifying. I can remove the output file name and just put in a field for suffix of denoised images. I would really like to keep the frame range selectors. Often times you notice an error in a sequence. You go in and fix the error and re-renders just those frames. Then I would like to denoise just those frames and not the whole sequence again. I get your idea of using the Softimage sequence syntax to change frames, lets say [1..10;4] to [2..3;4] to just output frames 0002 and 0003. But I really want the input field to recognize #### sequences as well. Having start and end frame properties is also very common in the rest of Softimage and is easier to script as well.

I didn't get an answer on if I should keep the rename message severity as a Warning or change it to Info. What do you think?

sjannuz commented 5 years ago

I'd say to use Info. Ok for keeping start/end.

JenusL commented 5 years ago

Ok I think this is it.

arnold_denoiser

JenusL commented 5 years ago

I just came to think that this might fail in a cross platform environment as I don't do any calls to Linktab.ResolvePath Let me fix that as well.

JenusL commented 5 years ago

Linktab support added. Now I'm done :)

caron commented 5 years ago

OK, cool stuff...

BTW, this shouldn't hold up releasing the big 5.2 drop.

JenusL commented 5 years ago

I thought about adding it to the Arnold->Properties menu but because it just ignores whatever is selected and adds itself to the Current Pass i didn't do it. But maybe adding to Selected Pass is better? If nothing selected, add to current pass. If selection is not a pass log error. What do you think?

Using the SItoA log level instead of script editor is a smart thing. Why didn't I think of that? :) I don't have time to fix this today but will do it tomorrow.

caron commented 5 years ago

I do think we should add the denosier property to the menu, I can't think of another property that the plugin creates that doesn't exist in that menu. Is there one?

*Edit... Again, I don't think this should hold up the pending release.

JenusL commented 5 years ago

@caron Both your comments are fixed now. I also added the logic to add the property to the pass where the button is pressed in render options but only if the render options is local. I tried making Denoiser global if render options was global but apparently Custom Properties on Passes didn't remove the global property if one wanted to convert it to local using MakeLocal() properly, so I skipped support for global denoiser properties.