Windows build improvements about pbrt-v4

[pierremoreau]

(Among other things, the headers don't seem to appearing even though there's a source_group directive that should be putting them there.)

Only the files that are added to the targets will show up, even if they are part of a source_group(). If you add ${PBRT_SOURCE_HEADERS} to add_library (pbrt_lib), those headers will then be listed under "pbrt_lib" -> "Header Files" in Visual Studio; I just tried it out.

[mmmovania]

Thank you so much Matt and the PBRT team for sharing the v4 source code with us early on. I want to share my bit in this if I may. I have managed to successfully build pbrt v4 on a 64-bit Windows 8.1 machine. As you would know the default Cmake setup requires CUDA v> 11.0 and Optix v>7 and if you dont have those, the pbrtv4 visual studio project is not generated. The reason for this is because line 163 of CMakeLists.txt flags unavailability of CUDA 11 and Optix 7 as an error. I have listed my changes below for others as reference.

1. change the cmake version in zlib/CMakeLists.txt and ptex/CMakeLists.txt to 3.12. [This will remove a few CMake warnings]

2. update the pbrtv4 root CMakeLists.txt line 163 from this [Already addressed in 995d9e]
   message(SEND_ERROR "pbrt-v4 requires CUDA version 11.0 or later. If you have multiple versions installed, please update your PATH.")
   to this
   message(WARNING "pbrt-v4 requires CUDA version 11.0 or later. If you have multiple versions installed, please update your PATH.")

3. [This is not required as the default includes should include the appropriate headers as mentioned by mmp below] add the following lines to math.h before the Log2Int function (lines 370-389)

//Ref: https://stackoverflow.com/questions/355967/how-to-use-msvc-intrinsics-to-get-the-equivalent-of-this-gcc-code
#ifdef _MSC_VER
#include <intrin.h>

unsigned long __inline __builtin_ctz(uint32_t value) {
    unsigned long trailing_zero = 0;

    if (_BitScanForward(&trailing_zero, value)) {
        return trailing_zero;
    } else {
        // This is undefined, I better choose 32 than 0
        return 32;
    }
}

unsigned long __inline __builtin_clz(uint32_t value) {
    unsigned long leading_zero = 0;

    if (_BitScanReverse(&leading_zero, value)) {
        return 31 - leading_zero;
    } else {
        // Same remarks as above
        return 32;
    }
}

unsigned long long __inline __builtin_clzll(uint64_t value) {
    unsigned long leading_zero = 0;

    if (_BitScanReverse64(&leading_zero, value)) {
        return 31UL - leading_zero;
    } else {
        // Same remarks as above
        return 32UL;
    }
}
#endif

Thanks,
MMMovania

[mmp]

Thanks for working on this! I just pushed an update to the CMakeLists.txt file (in 995d9e0) that addresses the second issue (in a slightly different way).

I am curious why that change to math.h was necessary and why the changes to the other CMakeLists.txt files was necessary. For math.h, those __builtin_* functions shouldn't be called in the first place in Windows (note the code in there that does #elif defined(PBRT_HAS_INTRIN_H), etc. Also, I'm not sure why requiring a newer version of CMake than those packages currently require makes a difference. Any clarification would be much appreciated!

[mmmovania]

Thanks for a prompt response Matt. Let me address your questions.

1. Regarding the changes in math.h, you are absolutely correct, apparently for me the intrinsics were not defined while I was trying to compile the code and I had to add those functions in. Now that I cleaned and rebuild the code it does indeed take the correct headers in. I can confirm that it works fine without adding these intrinisics definitions in as PBRT_HAS_INTRIN_H does indeed pull the correct header.
2. Regarding the version no. issue this is just to remove the warning during CMake build process. I like a clean CMake log :)