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.

Sign up for GitHub

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

Jump to bottom

Re-add optional OIDN denoise as an external executable. #82832

Merged
merged 1 commit into from Oct 11, 2023
Merged

Re-add optional OIDN denoise as an external executable. #82832

merged 1 commit into from Oct 11, 2023

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Oct 5, 2023 •
edited

Implements godotengine/godot-proposals#7640

Alternative variant #82831 (load dynamic library directly).

Pros: can use executables for the different architecture (there's no ARM64 prebuild version for macOS in official releases).
Cons: oidnDenoise is a sample implementation and command line arguments might change in feature releases, temporary images are written/read to disk.

@bruvzg bruvzg added this to the 4.x milestone Oct 5, 2023
This was referenced Oct 5, 2023
Closed
Merged
@bruvzg bruvzg force-pushed the oidn_external_exe branch 2 times, most recently from 0c94d3e to a0a7df0 Compare October 5, 2023 12:31
@jcostello
Copy link
Contributor

How can I test this?

@bruvzg bruvzg marked this pull request as ready for review October 6, 2023 13:39
@bruvzg bruvzg requested review from a team as code owners October 6, 2023 13:39
akien-mga
akien-mga reviewed Oct 11, 2023
View reviewed changes
doc/classes/EditorSettings.xml Outdated
@@ -500,6 +500,9 @@
If [code]true[/code], when saving a file, the editor will rename the old file to a different name, save a new file, then only remove the old file once the new file has been saved. This makes loss of data less likely to happen if the editor or operating system exits unexpectedly while saving (e.g. due to a crash or power outage).
[b]Note:[/b] On Windows, this feature can interact negatively with certain antivirus programs. In this case, you may have to set this to [code]false[/code] to prevent file locking issues.
</member>
<member name="filesystem/tools/oidn/oidn_path" type="String" setter="" getter="">
The path to the directory containing the OIDN denoise executable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The path to the directory containing the OIDN denoise executable.
The path to the directory containing the Open Image Denoise (OIDN) executable, used optionally for denoising lightmaps. It can be downloaded from [url=https://www.openimagedenoise.org/downloads.html]openimagedenoise.org[/url].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we might need some sort of special interface for all tools like this, like a dedicated "External tool manager" next to template manager. Which present the categorized list of all external tools, checks the current status of the tool (using both user configured location and normal PATH lookup) and maybe provide a button to download and automatically configure each tool.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great. The UX currently is really subpar, especially with Blender that throws a warning in new projects because it's enabled by default but the path isn't configured by default.

modules/lightmapper_rd/lightmapper_rd.cpp Outdated Show resolved Hide resolved
modules/lightmapper_rd/lightmapper_rd.cpp Outdated Show resolved Hide resolved
modules/lightmapper_rd/lightmapper_rd.cpp Outdated Show resolved Hide resolved
modules/lightmapper_rd/lightmapper_rd.cpp Outdated Show resolved Hide resolved
akien-mga
akien-mga approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested successfully on Linux with the Global Illumination demo.

modules/lightmapper_rd/lightmapper_rd.cpp Outdated Show resolved Hide resolved
Calinou
Calinou approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected. GPU acceleration is working correctly on NVIDIA with CUDA libraries installed on the system (confirmed by using watch -n1 nvidia-smi while baking lightmaps).

Testing project: test_lightmap_preview_bake_4.x.zip

No denoiser

Screenshot_20231011_173457

JNLM

Screenshot_20231011_173055

OIDN

There are more seams visible with OIDN, but the normal information in this scene is kind of broken due to regressions in the OBJ importer I need to bisect (these aren't new in 4.2).

Screenshot_20231011_173428

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Oct 11, 2023
Calinou
Calinou reviewed Oct 11, 2023
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to mention: LightmapGI's denoiser_strength documentation should note that it only has an effect when using the JNLM denoiser, not OIDN.

@bruvzg
Copy link
Member Author

bruvzg commented Oct 11, 2023

Moved the check to the beginning of the bake function and changed the path to oidn_denoise_path.

@akien-mga akien-mga merged commit efc0b08 into godotengine:master Oct 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga mentioned this pull request Oct 11, 2023
Closed
@bruvzg bruvzg mentioned this pull request Oct 12, 2023
Merged
@atafra
Copy link

atafra commented Oct 13, 2023 •
edited

I wouldn't recommend using oidnDenoise for this. It was never meant to be used in production, it's just an example app. Also, you would need to keep writing and loading images from disk, which often would be much slower than actually denoising the image on the GPU. I think dynamically loading the library would make a lot more sense, both in terms of robustness and performance.

@Calinou
Copy link
Member

Calinou commented Oct 13, 2023 •
edited

I think dynamically loading the library would make a lot more sense, both in terms of robustness and performance.

See #82831. This is indeed a better approach, but we need precompiled releases with ARM64 macOS libraries to be available for this approach to be viable.

@atafra
Copy link

atafra commented Oct 13, 2023

ARM64 macOS builds are planned to be added starting with the next OIDN release.

@Calinou Calinou mentioned this pull request Oct 18, 2023
Merged
@bruvzg bruvzg deleted the oidn_external_exe branch October 26, 2023 19:22
@Calinou Calinou mentioned this pull request Nov 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Calinou Calinou Calinou approved these changes

@DarioSamo DarioSamo DarioSamo left review comments

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
enhancement topic:editor topic:rendering topic:thirdparty topic:3d
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bruvzg @jcostello @akien-mga @atafra @Calinou @DarioSamo