Replace OIDN denoiser in Lightmapper with a JNLM denoiser compute shader.

[DarioSamo]

Original proposal: godotengine/godot-proposals#7640

Background

"Nonlinearly Weighted First-order Regression for Denoising Monte Carlo Renderings" (https://benedikt-bitterli.me/nfor/nfor.pdf) mentions in its opening chapter the possibility of using Non-Local Means with Joint Filtering, which has a fairly simple implementation over at https://github.com/ManuelPrandini/YoctoImageDenoiser. However, upon reviewing the implementation I found a few minor corrections that needed to be made and decided having the algorithm run in a compute shader would definitely yield far better results when it comes to runtime.

As mentioned in the proposal, the current implementation of OIDN is fairly outdated, and most importantly, extremely slow due to being forced to use a single CPU thread. On the scene I used for testing this out, the overall rendering time went down from 3 minutes to just over 1 minute in an RTX 3090 Ti with an 8K lightmap.

While it is not expected for the visual quality to be able to compete due to the nature of the algorithms (one is just a compute shader, the other uses an entire neural network and definitely understands the topic far more than I do), I think overall the result is fairly competitive and this might prove to be a decent fallback if we choose to go via the route of only using OIDN via the CLI.

One interesting advantage JNLM has over OIDN here is that it can use both the albedo and normal inputs to help reduce artifacts like seams and help improve the denoising and filtering process significantly. This could potentially be further improved if we can think of more auxiliary inputs we can use to help the algorithm out.

Lighting Only

Raw

JNLM

OIDN

Composite Result

Raw

JNLM

OIDN

Feedback

My reason for the PR is that I think it'd be interesting if people could test the branch out and see if they achieve decent enough results for this implementation to act as the future fallback when OIDN is not installed. However, seeing as it got very close, I think it would be reasonable to also experiment with this algorithm and see if with some tweaks the result could be even closer than what I initially got.

For that I'd like to point out the following code in the shader itself:

https://github.com/DarioSamo/godot/blob/ed6a4e53e7420e93ca07bd48960e6eebe00c2f67/modules/lightmapper_rd/lm_compute.glsl#L753-L777

	// Half the size of the patch window around each pixel that is weighted to compute the denoised pixel.
	// A value of 1 represents a 3x3 window, a value of 2 a 5x5 window, etc.
	const int HALF_PATCH_WINDOW = 4;

	// Half the size of the search window around each pixel that is denoised and weighted to compute the denoised pixel.
	const int HALF_SEARCH_WINDOW = 10;

	// For all of the following sigma values, smaller values will give less weight to pixels that have a bigger distance
	// in the feature being evaluated. Therefore, smaller values are likely to cause more noise to appear, but will also
	// cause less features to be erased in the process.

	// Controls how much the spatial distance of the pixels influences the denoising weight.
	const float SIGMA_SPATIAL = 5.0f;

	// Controls how much the lightmap color distance of the pixels influences the denoising weight.
	const float SIGMA_COLOR = 0.1f;

	// Controls how much the albedo color distance of the pixels influences the denoising weight.
	const float SIGMA_ALBEDO = 1.0f;

	// Controls how much the normal vector distance of the pixels influences the denoising weight.
	const float SIGMA_NORMAL = 0.5f;

	// Strength of the filter. The original paper recommends values around 10 to 15 times Sigma parameters.
	const float FILTER_VALUE = 15.0f * SIGMA_COLOR;

Some of the feature losses such as shadows and some particular areas as shown in the other picture might be fixable by playing around with these values and finding a sweet spot. It'd be preferable to not have to expose them to end-users as they can be values that are hard to understand what direct impact they have, but it could be evaluated to do so if it's deemed necessary to fit the needs of other scenes.

TODO

• Make strength configurable with a slider.
• Proposal: Remove OIDN and the denoiser module.

[Calinou]

Tested locally, it works as expected. The new denoiser does its job much faster than OIDN.

The scenes below use fully baked lights (Static bake mode).

Without textures

No denoiser	OIDN	JNLM (this PR)

With textures

No denoiser	OIDN	JNLM (this PR)

The final result looks pleasant, but it's a bit aggressive on shadows in this scene. Outdoor scenes fare better on this aspect:

Without textures

No denoiser	JNLM (this PR)

With textures

No denoiser	JNLM (this PR)

Also, as bake quality goes up, the denoiser should probably become less and less aggressive as it's less and less "needed" so to speak. This could be based on the number of rays thrown (as set in the Project Settings for each bake quality).

Here's an example with only indirect light baked:

Without textures

No denoiser	JNLM (this PR)

With textures

No denoiser	JNLM (this PR)

[DarioSamo]

It would be interesting to modify these two values to see if they help out with the shadows:

• Reducing SIGMA_COLOR will give more weights to the colors closer in value. Maybe it's a bit too high so it's allowing colors in a wider range (e.g. the shadowed and lit areas in the interior) to be denoised too easily. This might need to be adjusted depending on interiors or outdoors based on the expected difference between lit areas and shadows.
• FILTER_VALUE should have a pretty direct correlation to show strong the denoiser is. It'd be good to get a reference here on what strength we might need depending on the ray count.

I agree adjusting the strength might be easily doable by using the quality preset setting we already have for the ray count. That'd allow us to adjust the filtering to be stronger or weaker based on the expected noise.

[DarioSamo]

Also noticed a bug that slipped by that invalid pixels where getting added to the filter. This fixes the extra darkness a bit.

Before

After

[wareya]

Looking at Calinou's screenshots makes me feel like JNLM breaks and just does a giant blur in a lot of situations where it probably shouldn't; the part under the stairs in the first screenshot set stands out a lot in a bad way.

[DarioSamo]

Looking at Calinou's screenshots makes me feel like JNLM breaks and just does a giant blur in a lot of situations where it probably shouldn't; the part under the stairs in the first screenshot set stands out a lot in a bad way.

I agree the feature loss is a bit much, but I think it's fixable by tweaking the one of the constants I mentioned before, so I'd be interested in how much lower the color range would need to be so the shadows are not lost but a reasonable amount of noise is reduced.

[DarioSamo]

To provide an example, here's how much the filtering is affected by changing the filter value.

This one has a pretty direct result so maybe we could expose it as a setting next to the "Use Denoiser" toggle as well.

1.0

5.0

10.0

15.0

[DarioSamo]

Also reducing the normal bandwidth drastically can fix the banding artifacts visible on the pillars and the carpets, it was definitely too high for its intended purpose.

[DarioSamo]

Exposed this new setting that controls the filter value directly so we can tweak it if necessary.

[Calinou]

Testing project: https://0x0.st/HO8p.zip

I've tested the latest revision of this PR on the same testing project as above (complex/complex.tscn). On the staircase's shadows, changing the denoiser strength doesn't seem to do much:

Denoiser off	Denoiser 0.1	Denoiser 5.0	Denoiser 20.0

Note that I disabled the texture before baking lightmaps here, so the result appears naturally brighter than in the screenshots I posted yesterday.

Other shadows in the scene fare better at lower denoiser levels, keeping a good level of detail while not exhibiting too much visible noise when textures are added to the mix:

Denoiser off	Denoiser 0.1	Denoiser 5.0

[Calinou]

The Denoise Strength property in LightmapGI should be hidden when Use Denoiser is disabled, as it has no effect in that case.

This can be done by modifying LightmapGI::_validate_property() (see the existing examples there), and changing LightmapGI::set_use_denoiser() to call notify_property_list_changed(); at the end of the method.

[mrjustaguy]

A suggestion with the lightmapper in general, wouldn't it make sense to store a Non-denoised lightmap, and have the denoiser make a copy of it and save a second denoised result that gets overwritten every time denoiser settings change? it'd speed up Iteration times a ton when it comes to changing the Denoising Settings, and might make it more viable to add more settings to play around with.

[akien-mga]

@DarioSamo If we proceed with removing OIDN as discussed on RC, here's a commit that does it, removing all references also in license details and docs:
akien-mga@9412222

-115k LOC, nice! Feel free to grab ownership and those sweet sweet negative LOC stats :D

[DarioSamo]

A suggestion with the lightmapper in general, wouldn't it make sense to store a Non-denoised lightmap, and have the denoiser make a copy of it and save a second denoised result that gets overwritten every time denoiser settings change? it'd speed up Iteration times a ton when it comes to changing the Denoising Settings, and might make it more viable to add more settings to play around with.

That'd double up the result of storing lightmaps, so it does come with the tradeoff of having to store double the textures as well as coming up with a new naming scheme for them and possibly file format changes.

I think the ideal goal here would be to avoid having to change these settings as much as possible or just have very few of them, as we don't want to impact the workflow of people significantly.

@DarioSamo If we proceed with removing OIDN as discussed on RC, here's a commit that does it, removing all references also in license details and docs: akien-mga@9412222

-115k LOC, nice! Feel free to grab ownership and those sweet sweet negative LOC stats :D

Ah sweet, I had a similar commit of my own but I missed the COPYRIGHT.TXT file. I'll just grab yours instead.

I will proceed to make some changes and see if we can restore those shadows in Calinou's scene somehow.

[mrjustaguy]

it could be an editor setting, and you'd just call the OG one something like original or noisy or base and it could be hidden away in editor cache, with the denoised being what people get in their project files.

[DarioSamo]

Did @Calinou's change and also changed what variable of the denoiser the strength was linked to.

While it can't exactly match OIDN's result, it is significantly improved by adjusting the strength on according to the level of contrast in the scene.

Taking it out of drafts so we can start testing it out more in detail. I don't think the design will change significantly at this point, but we might need to evaluate whether an additional setting is needed or not to have better control over it. I'd rather keep one single slider if we can.

[atirut-w]

@atirut-w I'm pretty confused about the scene you just shared. I think I see the seams but they're also visible without the denoiser. Are you sure they're even related? Can you point to what the seams are so we're in the same page?

These?:

[DarioSamo]

@atirut-w I'm pretty confused about the scene you just shared. I think I see the seams but they're also visible without the denoiser. Are you sure they're even related? Can you point to what the seams are so we're in the same page?

These?:

This scene has really visible seams even without the denoiser.

Honestly just looks like the lightmap size is way too low to me to expect any coherence between boxes that are joined together and the denoiser is working over far too few pixels.

An increase in the texture size also clearly helps with the coherence.

But really, unless you can somehow get a better result with OIDN, which I doubt because the problem here is the coherence of the texture across the surface, you should just do a better model. You won't find this behaves much better in other engines when you give the light-mapper a hard time to match the UV2 across meshes like this. It is far easier to solve this by just making one surface instead of a bunch of boxes (that and it's a waste of texture space considering the sides can't be seen at all).

For context for anyone else, this is the structure of the scene.

[atirut-w]

So, in conclusion, it's caused by bad UV2?

[clayjohn]

Implementation looks good to me.

In the Production team meeting we also discussed removing OIDN and agreed that we were okay removing it for now because the quality is only slightly better. We can evaluate bringing it back as an extension if there is user demand.

I have not tested locally, but I trust the testing done by other contributors

[DarioSamo]

So, in conclusion, it's caused by bad UV2?

The UV2 itself isn't the problem (although you could manually alter it to mitigate it further). The construction of the scene itself just works against how a lightmapper works in general.

Each of those cubes has its own lightmap. A denoiser will only work in the context of one texture at a time. It's not gonna be possible for any denoiser to fix that unless it starts working with multiple lightmaps at once and figuring out how things align in world space. That is a really tough problem to solve.

Think of how tough it can usually be to get a texture to tile properly across seams like this: now imagine how tough it's gonna be for a lightmapper to guarantee consistency across pixels, especially at low resolution when basically one pixel covers far more than that in the screen depending on the camera distance.

[Calinou]

I think this is good to go now 🙂
Great work!

[YuriSizov]

Thanks! This is a hefty reduction in the size of the codebase! Here's to faster build times 🎉

[Anutrix]

JNLM stands for Joint NL-Means.
Took me some time find the full form xD.

[Listwon]

2 years ago Blender dropped NLM in favor of superior OIDN and here instead of properly implementing latest version of OIDN, Godot drops it in favor of inferior denoiser algorithm? It's not even affecting runtime binaries, just the editor, so i don't get it. NLM could be an external alternative denoiser or something you're able to select and choose from other options, but not the only core tech as it gives worse results. Quality should be more favorable for the offline denoising tool or at least switchable (to get better results in the final bake).

NLM as faster (i doubt it with multithreaded implementation of OIDN), choosable alternative sounds ok.

Open Image Denoise is used by Blender and Unity.

[DarioSamo]

2 years ago Blender dropped NLM in favor of superior OIDN and here instead of properly implementing latest version of OIDN, Godot drops it in favor of inferior denoiser algorithm? It's not even affecting runtime binaries, just the editor, so i don't get it. NLM could be an external alternative denoiser or something you're able to select and choose from other options, but not the only core tech as it gives worse results. Quality should be more favorable for the offline denoising tool or at least switchable (to get better results in the final bake).

NLM as faster (i doubt it with multithreaded implementation of OIDN), choosable alternative sounds ok.

Open Image Denoise is used by Blender and Unity.

It's already been explained in the original proposal, you'll never have an embedded OIDN integration that works properly due to how Godot's build system works unless you want to port their custom compiler, CUDA, HIP, etc. Nobody's really up for maintaining that level of integration with how complex it can be.

The purpose of this is to have an embedded alternative that works and we can have a command line option that you can set so you can use OIDN properly. The current version that was embedded was slow to the point of being unusable (single threaded). I don't see any realistic scenario on how that was viable for production the way it was.

[Listwon]

It's not an "alternative" if it's the only core option available. Any functionality you remove from the core/move outside will lead to less Godot users using and caring about it and in turn less people interested in using engine without industry standard functionalities (because nobody or hardly anybody maintains something outside the core, which people doesn't care about; it's a vicious cycle).

Why wouldn't you make it a choosable denoiser and try to get the funding for better implementation of OIDN or simply spread awareness that it needs more love? (i know it's not that easy, but with OIDN removed from the core it's simply impossible)

It could also encourage implementing other choosable denoisers.

[DarioSamo]

It's not an "alternative" if it's the only core option available. Any functionality you remove from the core/move outside will lead to less Godot users using and caring about it and in turn less people interested in using engine without industry standard functionalities (because nobody or hardly anybody maintains something outside the core, which people doesn't care about; it's a vicious cycle).

Why wouldn't you make it a choosable denoiser and try to get the funding for better implementation of OIDN or simply spread awareness that it needs more love? (i know it's not that easy, but with OIDN removed from the core it's simply impossible)

It could also encourage implementing other choosable denoisers.

The plan is already laid out in the original proposal on how to go about it. It's not out of the cards to add back OIDN and it'd definitely be as part of the core as this is, it's just the way OIDN's build system works that makes it prohibitive for Godot to embed it by itself (TBB, CUDA, HIP and ISPC). It could be as simple as automatically downloading the executable from Intel when you choose the option in the lightmapper. People wouldn't have to go hunt down an unmaintained extension. I'm very much in favor of that being implemented.

It was also missing from the blog post perhaps, but it's worth mentioning this denoiser can actually fix stuff OIDN was just getting wrong or straight up breaking, as it lacks the information from surface normals and albedo which helps guide the result a lot better than your average NLM implementacion (hence the J part of the name).

[clayjohn]

@Listwon What it comes down to is that Godot can't use OIDN out of the box. If we want to use it, we have to cobble together a series of hacks to make it work with Godot. That's what we did previously and it made OIDN borderline unusable (no multithreading support, no GPU support, we were stuck on an old version). The effort to get OIDN working well in Godot would be monumental. It would increase the size of the editor by tens of mbs and would require a significant amount of resources to maintain.

Ultimately what we found was that with the time savings from not using OIDN, quality can be increased to a degree that denoising is less necessary and the functional difference between the two approaches was minimized. Given that, it made little sense for us to keep OIDN around.

We considered making the denoiser switchable between JNLM and OIDN, but the quality difference was too small to justify keeping OIDN around at this point.

More funding is certainly welcome if you have some leads who would like to pay for bringing OIDN back. Given the substantial build system changes necessary and the amount of work to connect things like CUDA and HIP, we would likely need around $50,000 or so for the initial work and then slightly less, maybe $10,000 per year to keep it up to date.

[bruvzg]

The effort to get OIDN working well in Godot would be monumental.

Is there any reason not to use oidnDenoise executable as an external tool (as an option), like we do with blender? It's using extremely simple PFM image format (basically 3 lines of text metadata and raw float image data, supporting reading/writing it should be trivial) and seems to support all features OIDN module were using.

[DarioSamo]

The effort to get OIDN working well in Godot would be monumental.

Is there any reason not to use oidnDenoise executable as an external tool (as an option), like we do with blender? It's using extremely simple PFM image format (basically 3 lines of text metadata and raw float image data, supporting reading/writing it should be trivial) and seems to support all features OIDN module were using.

That's pretty much what the proposal lays out we should do, and I agree. @Calinou pointed out there might be some other things to solve when it comes to the image format at least.

Admittedly maybe this PR went out earlier than having that option back in yet, but it wouldn't take as much effort and it'd yield a far better result than choosing to make it part of Godot itself.

[clayjohn]

The effort to get OIDN working well in Godot would be monumental.

Is there any reason not to use oidnDenoise executable as an external tool (as an option), like we do with blender? It's using extremely simple PFM image format (basically 3 lines of text metadata and raw float image data, supporting reading/writing it should be trivial) and seems to support all features OIDN module were using.

That's the current plan!

[Listwon]

Is there any reason not to use oidnDenoise executable as an external tool (as an option), like we do with blender?

I think the only reason could be that it's a sample implementation and the commands can change between binary releases. But it's good enough approach, allows passing extra data to OIDN (albedo, normals) and running it on multiple CPU threads or GPU.

[bruvzg]

I think the only reason could be that it's a sample implementation and the commands can change between binary releases.

I guess loading the dynamic lib should not have these issues (at least across the same major version), and can be done in the same way as executable (the only downside, there's no ARM64 macOS prebuild).

Here's a quickly made test for dynamic library loading - https://github.com/bruvzg/godot/tree/oidn_external_lib (seems to work, at least on macOS), library location can be set in Editor Settings → Filesystem → Tools → Oidn → Oidn library path.

[DarioSamo]

I think the only reason could be that it's a sample implementation and the commands can change between binary releases.

I guess loading the dynamic lib should not have these issues (at least across the same major version), and can be done in the same way as executable (the only downside, there's no ARM64 macOS prebuild).

Here's a quickly made test for dynamic library loading - https://github.com/bruvzg/godot/tree/oidn_external_lib (seems to work, at least on macOS), library location can be set in Editor Settings → Filesystem → Tools → Oidn → Oidn library path.

That's pretty sweet, I think you'll have an easy chance for this to get accepted now that the JNLM denoiser is in.

[bruvzg]

Not sure which option is better, so made both:

#82831 (dynamic library)
#82832 (executable)