NVIDIAGameWorks / RayTracingDenoiser

NVIDIA Ray Tracing Denoiser
Other
504 stars 46 forks source link

Small problems in CMakeLists.txt and Wrapper.cpp #33

Closed stkrake closed 2 years ago

stkrake commented 2 years ago

I had these small problems with CMakeLists.txt on win32 and linux. Maybe there are better solutions (I don't know enough cmake):

And this prevents compilation on linux:

manuelk commented 2 years ago
  1. shader compiler path: set(NRD_DXC_CUSTOM_PATH "custom/path/to/dxc") cmake build files that expect user modification to function are bad practice - this should be written as follows, with an empty default value, later checked and automatically filled if possible, or a helpful fatal error ensuing. set(OPTION "Default" CACHE STRING "Helpstring") i suggest looking here for some cmake logic to locate the shader compilers: https://github.com/NVIDIAGameWorks/donut/blob/main/cmake/FindDXCdxil.cmake

  2. compiler specific flags: should at least be checked (with something like this, though there may be a better way): if (MSVC) ... endif() here is a not very elegant check for others: if (COMPILER_ID STREQUAL "gnu" OR COMPILER_ID STREQUAL "clang" OR COMPILER_ID STREQUAL "intel") while in there, i would also recommend turning on "production warnings"... add_definitions(/W3 /WX)

stkrake commented 2 years ago

Just brushed up my cmake skills a bit: "/MT" or "/MTd" could be configured via CMAKE_MSVC_RUNTIME_LIBRARY from the command line. But cmake_minimum_required has to be (VERSION 3.15) at least for this to work.

dzhdanNV commented 2 years ago

Thanks! I believe I have fixed the problems:

@stkrake Please, verify.

stkrake commented 2 years ago

CMakeList.txt now works on win32 and linux without modifications. I also checked that CMAKE_MSVC_RUNTIME_LIBRARY creates a NRD.vcxproj with the expected entries (but I did not compile or further test this).

Unfortunately compiling on linux now breaks due to "-Werror". I will open an new issue for this and closing this one.

Thanks for fixing.