RenderKit / ospray

An Open, Scalable, Portable, Ray Tracing Based Rendering Engine for High-Fidelity Visualization
http://ospray.org
Apache License 2.0
1.01k stars 182 forks source link

Fixing cmake configuration for multi-config generators #450

Open i-sultan opened 4 years ago

i-sultan commented 4 years ago

The assumption made throughout ospray cmake files is that CMAKE_BUILD_TYPE will always contain the build config type. The problem is that in multi-config generators (such as vs and xcode), the build type is only selected during build time, and setting CMAKE_BUILD_TYPE should have no impact.

This gets problematic if the user selects Debug during build time through --config, while CMAKE_BUILD_TYPE is not set, so it takes the default value of Release, causing a mismatch between the actual build type and the one set through CMAKE_BUILD_TYPE. Another scenario is when a user switches from Debug to Release builds, then they need to switch the CMAKE_BUILD_TYPE as well to keep it consistent with the actual config type, which is not the usual process in multi-config.

The solution proposed here relies on the cmake generator-expression ($) to derive the config type for both single and multi configuration types. It also enables debugging for windows since it was disabled before, for no good reason that I am aware off. The final impact is that we remove the default setting for CMAKE_BUILD_TYPE since it should use whatever is default for the generator.

The only downside comes to single-config builds, which is that we had to remove some variables from the UI (the cmake_build_type through cycling CONFIGURATION_TYPES), and also remove OSPRAY_BUILD_RELEASE/DEBUG/RELWITHDEBINFO from the config files, since we cannot use configure_file to support these as they rely now on generator expressions which are evaluated post config.

This change has been tested with both single (make) and multi-config (vs) and worked fine in both cases.

If not taking this pull request, I suggest at least removing the win32 condition from ispc debug, and clarifying in the documentation that multi-config generators (such as vs) need to set the CMAKE_BUILD_TYPE to be consistent with the chosen config type, otherwise some inconsistencies will happen (including the case where CMAKE_BUILD_TYPE is not set at all).

jeffamstutz commented 4 years ago

Just wanted to say thanks for this! We are busy with SIGGRAPH preparations right now, but this will get incorporated ASAP after that...we haven't forgotten about it. :)

i-sultan commented 4 years ago

Sure thing, Jeff! Looking forward to the magic you guys will show us at Siggraph this year.