Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arnold Denoiser #53

Merged
merged 32 commits into from
Jan 29, 2019
Merged

Arnold Denoiser #53

merged 32 commits into from
Jan 29, 2019

Conversation

JenusL
Copy link
Contributor

@JenusL JenusL commented Dec 18, 2018

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.

  • I made the Arnold Denoiser a self contained python plugin instead of adding it to Render Options. There's two reasons I did this:
    1. Denoiser is written in Python in MtoA, so I could share some of the code if I also wrote it in Python.
    2. This felt more like the Softimage way.
  • There's a button in the Denoiser tab that adds a Arnold_Denoiser property to the current pass. And from there the denoising settings is controlled. This means every pass has independent Denoising settings.
  • The output of the Denoising AOVs is working just like in MAXtoA right now. This is not optimal and doesn't feel like a nice and clean way to handle it in SItoA. I would like to change this so that the AOVs gets added to the Main output instead of to a separate file. I just need to get some more info on recommended bit-depths and stuff like that from Autodesk. @sjannuz Any idea of who I can contact about that?

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
Copy link
Contributor

sjannuz commented Dec 28, 2018

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
Copy link
Contributor Author

JenusL commented Dec 29, 2018

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
Copy link
Contributor Author

JenusL commented Jan 1, 2019

@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
Copy link
Contributor Author

JenusL commented Jan 4, 2019

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

@JenusL JenusL changed the title [WIP] Arnold Denoiser Arnold Denoiser Jan 4, 2019
@sjannuz
Copy link
Contributor

sjannuz commented Jan 7, 2019

@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
Copy link
Contributor Author

JenusL commented Jan 7, 2019

@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
Copy link
Contributor

sjannuz commented Jan 7, 2019

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
Copy link
Contributor Author

JenusL commented Jan 7, 2019

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
Copy link
Contributor

sjannuz commented Jan 8, 2019

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
Copy link

rmv commented Jan 8, 2019

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
Copy link
Contributor Author

JenusL commented Jan 8, 2019

@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
Copy link
Contributor

sjannuz commented Jan 8, 2019

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

@JenusL
Copy link
Contributor Author

JenusL commented Jan 8, 2019

Ok I think this is it.

  • Added a Output Suffix field
  • Made the Output read only so that you can still se the generated output path
  • Added check to see that file exist for each input frame (logs error then continues)
  • Made sure End Frame can't be less than Start Frame when in Start / End mode

arnold_denoiser

@JenusL
Copy link
Contributor Author

JenusL commented Jan 9, 2019

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
Copy link
Contributor Author

JenusL commented Jan 9, 2019

Linktab support added. Now I'm done :)

@caron
Copy link
Contributor

caron commented Jan 22, 2019

OK, cool stuff...

  • Should we add the Denoiser property to the Arnold>Properties menu?
  • Is it possible to see the noice.exe output inside Softimage script editor based on the render options verbosity instead of the script editor preference? So, 'Info' and 'Debug'...

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

@JenusL
Copy link
Contributor Author

JenusL commented Jan 22, 2019

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
Copy link
Contributor

caron commented Jan 22, 2019

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
Copy link
Contributor Author

JenusL commented Jan 23, 2019

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

@sjannuz sjannuz merged commit 5132ff2 into Autodesk:develop Jan 29, 2019
@JenusL JenusL deleted the feat/noice branch December 10, 2019 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants