CPU lightmapper's denoiser doesn't work on macOS x86-64 since 3.2.4 rc2

[snougo]

Godot version:

3.3.2 stable

OS/device including version:

Issue description:

it's been while since I tested new CPU lightmapper, and recently I test it again, but denosier doesn't work, then I test some old version, and find this issue appears since 3.2.4 rc2

3.2.4 beta6

3.2.4 rc1

3.2.4 rc2

Steps to reproduce: hit bake button

Minimal reproduction project:

New Game Project.zip

[akien-mga]

I can't reproduce the issue on Linux, the denoise step works fine for me in 3.3.2-stable. The denoiser is not built for macOS ARM64 as it's only supported on x86_64, but in your case it should be available on an old Mac. Maybe something is off in the build config.

[akien-mga]

I checked the build config, it seems to properly evaluate to True for x86_64 macOS on the official buildsystem, so I don't know why the denoiser would be missing. https://github.com/godotengine/godot/blob/3.3/modules/denoise/config.py#L20

CC @JFonS @bruvzg

[JFonS]

Not sure what's going on. If the module is built it should be calling LightmapDenoiserOIDN::make_default_denoiser() in register_denoise_types() so I would be inclined to think the module is not being built, but I don't have a macos build environment at hand to test.

[Calinou]

@snougo Can you (or anyone else) still reproduce this bug in Godot 3.3.4 or any later release?

[snougo]

@snougo Can you (or anyone else) still reproduce this bug in Godot 3.3.4 or any later release?

yes, and 3.4 rc1 too

[akien-mga]

Not sure what's going on. If the module is built it should be calling LightmapDenoiserOIDN::make_default_denoiser() in register_denoise_types() so I would be inclined to think the module is not being built, but I don't have a macos build environment at hand to test.

Here's an excerpt from the build logs of 3.4 RC 1, it's properly built in the x86_64 build:

generate_modules_enabled(["modules/modules_enabled.gen.h"], [OrderedDict([('bmp', 'modules/bmp'), ('bullet', 'modules/bullet'), ('camera', 'modules/camera'), ('csg', 'modules/csg'), ('cvtt', 'modules/cvtt'), ('dds', 'modules/dds'), ('denoise', 'modules/denoise'), ('enet', 'modules/enet'), ('etc', 'modules/etc'), ('fbx', 'modules/fbx'), ('freetype', 'modules/freetype'), ('gdnative', 'modules/gdnative'), ('gdscript', 'modules/gdscript'), ('gltf', 'modules/gltf'), ('gridmap', 'modules/gridmap'), ('hdr', 'modules/hdr'), ('jpg', 'modules/jpg'), ('jsonrpc', 'modules/jsonrpc'), ('lightmapper_cpu', 'modules/lightmapper_cpu'), ('mbedtls', 'modules/mbedtls'), ('minimp3', 'modules/minimp3'), ('mobile_vr', 'modules/mobile_vr'), ('ogg', 'modules/ogg'), ('opensimplex', 'modules/opensimplex'), ('opus', 'modules/opus'), ('pvr', 'modules/pvr'), ('raycast', 'modules/raycast'), ('recast', 'modules/recast'), ('regex', 'modules/regex'), ('squish', 'modules/squish'), ('stb_vorbis', 'modules/stb_vorbis'), ('svg', 'modules/svg'), ('tga', 'modules/tga'), ('theora', 'modules/theora'), ('tinyexr', 'modules/tinyexr'), ('upnp', 'modules/upnp'), ('vhacd', 'modules/vhacd'), ('visual_script', 'modules/visual_script'), ('vorbis', 'modules/vorbis'), ('webm', 'modules/webm'), ('webp', 'modules/webp'), ('webrtc', 'modules/webrtc'), ('websocket', 'modules/websocket'), ('webxr', 'modules/webxr'), ('xatlas_unwrap', 'modules/xatlas_unwrap')])])
/root/osxcross/target/bin/x86_64-apple-darwin20.2-c++ -o modules/denoise/denoise_wrapper.osx.opt.tools.x86_64.o -c -std=gnu++14 -O2 -arch x86_64 -mmacosx-version-min=10.12 -w -Werror=return-type -DDEBUG_ENABLED -D__MACPORTS__ -DOSX_ENABLED -DUNIX_ENABLED -DGLES_ENABLED -DAPPLE_STYLE_KEYS -DCOREAUDIO_ENABLED -DCOREMIDI_ENABLED -DGL_SILENCE_DEPRECATION -DPTRCALL_ENABLED -DTOOLS_ENABLED -DMINIZIP_ENABLED -DZSTD_STATIC_LINKING_ONLY -DGLAD_ENABLED -DGLES_OVER_GL -DMKLDNN_THR=MKLDNN_THR_SEQ -DOIDN_STATIC_LIB -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS -DDISABLE_VERBOSE -DMKLDNN_ENABLE_CONCURRENT_EXEC -DNDEBUG -Ithirdparty/oidn -Ithirdparty/oidn/include -Ithirdparty/oidn/mkl-dnn/include -Ithirdparty/oidn/mkl-dnn/src -Ithirdparty/oidn/mkl-dnn/src/common -Ithirdparty/oidn/mkl-dnn/src/cpu/xbyak -Ithirdparty/oidn/mkl-dnn/src/cpu -Ithirdparty/libpng -Ithirdparty/glad -Ithirdparty/zstd -Ithirdparty/zlib -Iplatform/osx -I. modules/denoise/denoise_wrapper.cpp
/root/osxcross/target/bin/x86_64-apple-darwin20.2-c++ -o modules/denoise/lightmap_denoiser.osx.opt.tools.x86_64.o -c -std=gnu++14 -O2 -arch x86_64 -mmacosx-version-min=10.12 -w -Werror=return-type -DDEBUG_ENABLED -D__MACPORTS__ -DOSX_ENABLED -DUNIX_ENABLED -DGLES_ENABLED -DAPPLE_STYLE_KEYS -DCOREAUDIO_ENABLED -DCOREMIDI_ENABLED -DGL_SILENCE_DEPRECATION -DPTRCALL_ENABLED -DTOOLS_ENABLED -DMINIZIP_ENABLED -DZSTD_STATIC_LINKING_ONLY -DGLAD_ENABLED -DGLES_OVER_GL -Ithirdparty/libpng -Ithirdparty/glad -Ithirdparty/zstd -Ithirdparty/zlib -Iplatform/osx -I. modules/denoise/lightmap_denoiser.cpp
/root/osxcross/target/bin/x86_64-apple-darwin20.2-c++ -o modules/denoise/register_types.osx.opt.tools.x86_64.o -c -std=gnu++14 -O2 -arch x86_64 -mmacosx-version-min=10.12 -w -Werror=return-type -DDEBUG_ENABLED -D__MACPORTS__ -DOSX_ENABLED -DUNIX_ENABLED -DGLES_ENABLED -DAPPLE_STYLE_KEYS -DCOREAUDIO_ENABLED -DCOREMIDI_ENABLED -DGL_SILENCE_DEPRECATION -DPTRCALL_ENABLED -DTOOLS_ENABLED -DMINIZIP_ENABLED -DZSTD_STATIC_LINKING_ONLY -DGLAD_ENABLED -DGLES_OVER_GL -Ithirdparty/libpng -Ithirdparty/glad -Ithirdparty/zstd -Ithirdparty/zlib -Iplatform/osx -I. modules/denoise/register_types.cpp

I wonder if lipo would strip compiled code only available for one architecture when creating the universal binary? CC @bruvzg

3.2.4 beta 2 seems to be the first release including an arm64 build as a universal archive. There was also still an x86_64 only build included, could you confirm that https://downloads.tuxfamily.org/godotengine/3.2.4/beta2/Godot_v3.2.4-beta2_osx.64.zip works but https://downloads.tuxfamily.org/godotengine/3.2.4/beta2/Godot_v3.2.4-beta2_osx.universal.zip doesn't @snougo?

[bruvzg]

Denoise module is disabled for the ARM builds, the version of OIDN we use supports x86-64 only.

[bruvzg]

Ah, sorry, did not notice issue is on x86. No, lipo should not strip anything, it's primitive and just gluing two executable together as is and add a header. Maybe something is wrong with the build config.

[bruvzg]

3.4 rc1 running under Rosetta seems to generate light map without any noise, probably this was fixed at some point.

[bruvzg]

@snougo Try it in the mono version of the editor. Or try re-signing it locally with the full set of entitlements, if you have Xcode installed.

Copy this to the entitlements.plist file:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
 <key>com.apple.security.cs.allow-dyld-environment-variables</key>
 <true/>
 <key>com.apple.security.cs.allow-jit</key>
 <true/>
 <key>com.apple.security.cs.allow-unsigned-executable-memory</key>
 <true/>
 <key>com.apple.security.cs.disable-executable-page-protection</key>
 <true/>
 <key>com.apple.security.cs.disable-library-validation</key>
 <true/>
 <key>com.apple.security.device.audio-input</key>
 <true/>
 <key>com.apple.security.device.camera</key>
 <true/>
</dict>
</plist>

And run the following command from the terminal:

codesign --force -s - --options=runtime --entitlements entitlements.plist /Applications/Godot.app

OIDN seems to require JIT entitlements (https://github.com/OpenImageDenoise/oidn#entitlements-on-macos). Maybe Rosetta is ignoring entitlements.

[akien-mga]

@bruvzg Nice catch! You can update misc/dist/osx/editor.entitlements accordingly, I'm also changing the build scripts to make sure we use those versions from the engine repo: https://github.com/godotengine/godot-build-scripts/pull/47

[bruvzg]

I'm also changing the build scripts to make sure we use those versions from the engine repo: godotengine/godot-build-scripts#47

I guess we can use single editor.entitlements for both mono and non-mono since both will be identical. It's probably a good idea anyway, in case there are add-ons with JIT it's also required.