Skip to content

Re-add optional OIDN denoise as an external dynamic library. - #82831

Closed
bruvzg wants to merge 1 commit into
godotengine:masterfrom
bruvzg:oidn_external_lib
Closed

Re-add optional OIDN denoise as an external dynamic library.#82831
bruvzg wants to merge 1 commit into
godotengine:masterfrom
bruvzg:oidn_external_lib

Conversation

@bruvzg

@bruvzg bruvzg commented Oct 5, 2023

Copy link
Copy Markdown
Member

Implements godotengine/godot-proposals#7640

Alternative variant #82832 (uses executable).

Pros: more stable API, no writing/reading to disk.
Cons: library should be the same architecture as executable (and there's no ARM64 prebuild library for macOS in official releases), might have issues with dependency loading paths.

TODO: ensure correct library lookup paths are set on all platforms.

Comment thread modules/lightmapper_rd/lightmapper_rd.cpp Outdated
@akien-mga

Copy link
Copy Markdown
Member

Superseded by #82832.

@atirutw

atirutw commented Oct 12, 2023

Copy link
Copy Markdown
Contributor

Bummer. Using OIDN as an external lib sounded mor reasonable for integrations.

@akien-mga

Copy link
Copy Markdown
Member

Well this can be reconsidered later if need be, it doesn't change much in the implementation. But not having macOS ARM support would be the bigger bummer IMO.

@bruvzg

bruvzg commented Oct 12, 2023

Copy link
Copy Markdown
Member Author

But not having macOS ARM support would be the bigger bummer IMO.

It seems to be possible to build it for ARM macOS, but for some reason it's not part of official releases.

@AThousandShips AThousandShips removed this from the 4.x milestone Oct 12, 2023
@atafra

atafra commented Oct 13, 2023

Copy link
Copy Markdown

We're planning to include ARM macOS builds for OIDN starting with the next major release.

@bruvzg

bruvzg commented Oct 13, 2023

Copy link
Copy Markdown
Member Author

TODO: ensure correct library lookup paths are set on all platforms.

Seems like on Windows, OIDN is unusable as a dynamic library unless all dependency libs are in the same folder as executable.

Main dll is loading fine, and it's finding module dll, but unable to load its dependencies (tbb dlls), and since it's using LoadLibraryW we can't override library search path with AddDllDirectory on the engine side.

But in should be fixable in the OIDN module loader with something like this:

diff --git a/core/module.cpp b/core/module.cpp
index 14ac590..db74014 100644
--- a/core/module.cpp
+++ b/core/module.cpp
@@ -50,7 +50,7 @@ OIDN_NAMESPACE_BEGIN
     // Prevent the system from displaying a message box when the module fails to load
     UINT prevErrorMode = GetErrorMode();
     SetErrorMode(prevErrorMode | SEM_FAILCRITICALERRORS);
-    void* module = LoadLibraryW(path.c_str());
+    void* module = LoadLibraryExW(path.c_str(), nullptr, LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR);
     SetErrorMode(prevErrorMode);
   #else
     void* module = dlopen(path.c_str(), RTLD_NOW | RTLD_LOCAL);

I'll check it on Linux and macOS a bit later.
Edit: macOS libs seems to have correctly set @rpath, so it's not an issue.
Edit: seems to be working on Linux as well, so it's a Windows specific issue.
Edit: upstream PR - RenderKit/oidn#179

@bruvzg

bruvzg commented Oct 14, 2023

Copy link
Copy Markdown
Member Author

A new PR version - #83314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants