pbrt-v4 icon indicating copy to clipboard operation
pbrt-v4 copied to clipboard

Update the Ptex submodule to better support MinGW on Windows

Open c8ef opened this issue 2 years ago • 8 comments

My building environment is windows with mingw64-gcc. Now its build will fail like below:

In file included from C:/Users/x/Desktop/pbrt-v4/src/ext/ptex/src/ptex/Ptexture.h:65,
                 from C:/Users/x/Desktop/pbrt-v4/src/pbrt/textures.cpp:24:
C:/Users/x/Desktop/pbrt-v4/src/ext/ptex/src/ptex/PtexInt.h:49:27: error: conflicting declaration
 'typedef char int8_t'
   49 | typedef __int8            int8_t;
      |                           ^~~~~~
In file included from C:/msys64/ucrt64/lib/gcc/x86_64-w64-mingw32/12.2.0/include/stdint.h:9,
                 from C:/Users/x/Desktop/pbrt-v4/src/pbrt/pbrt.h:8,
                 from C:/Users/x/Desktop/pbrt-v4/src/pbrt/textures.h:8,
                 from C:/Users/x/Desktop/pbrt-v4/src/pbrt/textures.cpp:5:
C:/msys64/ucrt64/include/stdint.h:35:21: note: previous declaration as 'typedef signed char int8_t'
   35 | typedef signed char int8_t;
      |                     ^~~~~~
[248/462] Building CXX object CMakeFiles/pbrt_lib.dir/src/pbrt/cpu/integrators.cpp.obj

The reason is as follow:

#if defined(_WIN32) || defined(_WINDOWS) || defined(_MSC_VER)

#if defined(_MSC_VER) && _MSC_VER >= 1600
#include <stdint.h>
#else
typedef __int8            int8_t;
typedef __int16           int16_t;
typedef __int32           int32_t;
typedef __int64           int64_t;
typedef unsigned __int8   uint8_t;
typedef unsigned __int16  uint16_t;
typedef unsigned __int32  uint32_t;
typedef unsigned __int64  uint64_t;
#endif

#else
#include <stdint.h>
#endif

When building with MinGW, it won't include <stdint.h> and use its own int types. The newest version of plex has fixed this problem.

#if defined(_MSC_VER) && _MSC_VER >= 1600 || defined(__MINGW64__)

Add this feature test macro then pbrt can successfully build!

c8ef avatar Oct 13 '22 05:10 c8ef

Yes.. I are send a pull request a few weeks ago fixing this issue: https://github.com/wdas/ptex/commit/c138f8391c3b6555c347d25ae9b05eca778a29a1

Cheers..

pbrt4bounty avatar Oct 17 '22 21:10 pbrt4bounty

It would be good to update pbrt to use a modern version of ptex, but unfortunately it's not straightforward. I've pushed a branch, update-ptex, that updates Ptex to v2.4.2 but it causes build breakage on windows: https://github.com/mmp/pbrt-v4/actions/runs/3401367217 (though works elsewhere). I don't have the time dig into the further at the moment, but would happily accept a PR that got it all working!

mmp avatar Nov 05 '22 19:11 mmp

This patch should fix it. I thought I upstreamed it but maybe not

diff --git a/CMakeLists.txt b/CMakeLists.txt index 4c7e0c3..a933eea 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -33,9 +33,7 @@ enable_testing()

Setup platform-specific threading flags.

find_package(Threads REQUIRED)

-# Use pkg-config to create a PkgConfig::Ptex_ZLIB imported target -find_package(PkgConfig REQUIRED) -pkg_checK_modules(Ptex_ZLIB REQUIRED zlib IMPORTED_TARGET) +find_package(ZLIB REQUIRED)

if (NOT DEFINED PTEX_SHA) diff --git a/src/build/ptex-config.cmake b/src/build/ptex-config.cmake index dfe2851..5e63f07 100644 --- a/src/build/ptex-config.cmake +++ b/src/build/ptex-config.cmake @@ -8,9 +8,7 @@ set(THREADS_PREFER_PTHREAD_FLAG ON)

find_dependency(Threads REQUIRED)

-# Provide PkgConfig::ZLIB to downstream dependents -find_dependency(PkgConfig REQUIRED) -pkg_checK_modules(Ptex_ZLIB REQUIRED zlib IMPORTED_TARGET) +find_package(ZLIB REQUIRED)

set_and_check(Ptex_DIR @PACKAGE_CMAKE_INSTALL_PREFIX@) set_and_check(Ptex_LIBRARY_DIRS @PACKAGE_CMAKE_INSTALL_LIBDIR@) diff --git a/src/ptex/CMakeLists.txt b/src/ptex/CMakeLists.txt index e2e1bfd..4dfe372 100644 --- a/src/ptex/CMakeLists.txt +++ b/src/ptex/CMakeLists.txt @@ -23,7 +23,7 @@ if(PTEX_BUILD_STATIC_LIBS) PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(Ptex_static

  •    PUBLIC Threads::Threads PkgConfig::Ptex_ZLIB)
    
  •    PUBLIC Threads::Threads ZLIB::ZLIB)
    
    install(TARGETS Ptex_static EXPORT Ptex DESTINATION ${CMAKE_INSTALL_LIBDIR}) endif()

@@ -39,7 +39,7 @@ if(PTEX_BUILD_SHARED_LIBS) ${CMAKE_CURRENT_SOURCE_DIR}) target_compile_definitions(Ptex_dynamic PRIVATE PTEX_EXPORTS) target_link_libraries(Ptex_dynamic

  •    PUBLIC Threads::Threads PkgConfig::Ptex_ZLIB)
    
  •    PUBLIC Threads::Threads ZLIB::ZLIB)
    
    install(TARGETS Ptex_dynamic EXPORT Ptex DESTINATION ${CMAKE_INSTALL_LIBDIR}) endif()

diff --git a/src/utils/CMakeLists.txt b/src/utils/CMakeLists.txt index d0295cb..f6bd83d 100644 --- a/src/utils/CMakeLists.txt +++ b/src/utils/CMakeLists.txt @@ -4,6 +4,6 @@ if (PTEX_BUILD_STATIC_LIBS) add_definitions(-DPTEX_STATIC) endif()

-target_link_libraries(ptxinfo ${PTEX_LIBRARY} PkgConfig::Ptex_ZLIB) +target_link_libraries(ptxinfo ${PTEX_LIBRARY} ZLIB::ZLIB)

install(TARGETS ptxinfo DESTINATION ${CMAKE_INSTALL_BINDIR})

On Sun, 6 Nov 2022 at 08:57, Matt Pharr @.***> wrote:

It would be good to update pbrt to use a modern version of ptex, but unfortunately it's not straightforward. I've pushed a branch, update-ptex, that updates Ptex to v2.4.2 but it causes build breakage on windows: https://github.com/mmp/pbrt-v4/actions/runs/3401367217 (though works elsewhere). I don't have the time dig into the further at the moment, but would happily accept a PR that got it all working!

— Reply to this email directly, view it on GitHub https://github.com/mmp/pbrt-v4/issues/297#issuecomment-1304624241, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOYQXJHWLCAMCHQFH5M3Q3WG23Y7ANCNFSM6AAAAAARD4OT5I . You are receiving this because you are subscribed to this thread.Message ID: @.***>

anderslanglands avatar Nov 05 '22 22:11 anderslanglands

Ah no my bad, I did upstream it and the version you’re using includes this fix.

On Sun, 6 Nov 2022 at 11:32, Anders Langlands @.***> wrote:

This patch should fix it. I thought I upstreamed it but maybe not

diff --git a/CMakeLists.txt b/CMakeLists.txt index 4c7e0c3..a933eea 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -33,9 +33,7 @@ enable_testing()

Setup platform-specific threading flags.

find_package(Threads REQUIRED)

-# Use pkg-config to create a PkgConfig::Ptex_ZLIB imported target -find_package(PkgConfig REQUIRED) -pkg_checK_modules(Ptex_ZLIB REQUIRED zlib IMPORTED_TARGET) +find_package(ZLIB REQUIRED)

if (NOT DEFINED PTEX_SHA) diff --git a/src/build/ptex-config.cmake b/src/build/ptex-config.cmake index dfe2851..5e63f07 100644 --- a/src/build/ptex-config.cmake +++ b/src/build/ptex-config.cmake @@ -8,9 +8,7 @@ set(THREADS_PREFER_PTHREAD_FLAG ON)

find_dependency(Threads REQUIRED)

-# Provide PkgConfig::ZLIB to downstream dependents -find_dependency(PkgConfig REQUIRED) -pkg_checK_modules(Ptex_ZLIB REQUIRED zlib IMPORTED_TARGET) +find_package(ZLIB REQUIRED)

set_and_check(Ptex_DIR @PACKAGE_CMAKE_INSTALL_PREFIX@) set_and_check(Ptex_LIBRARY_DIRS @PACKAGE_CMAKE_INSTALL_LIBDIR@) diff --git a/src/ptex/CMakeLists.txt b/src/ptex/CMakeLists.txt index e2e1bfd..4dfe372 100644 --- a/src/ptex/CMakeLists.txt +++ b/src/ptex/CMakeLists.txt @@ -23,7 +23,7 @@ if(PTEX_BUILD_STATIC_LIBS) PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) target_link_libraries(Ptex_static

  •    PUBLIC Threads::Threads PkgConfig::Ptex_ZLIB)
    
  •    PUBLIC Threads::Threads ZLIB::ZLIB)
    
    install(TARGETS Ptex_static EXPORT Ptex DESTINATION ${CMAKE_INSTALL_LIBDIR}) endif()

@@ -39,7 +39,7 @@ if(PTEX_BUILD_SHARED_LIBS) ${CMAKE_CURRENT_SOURCE_DIR}) target_compile_definitions(Ptex_dynamic PRIVATE PTEX_EXPORTS) target_link_libraries(Ptex_dynamic

  •    PUBLIC Threads::Threads PkgConfig::Ptex_ZLIB)
    
  •    PUBLIC Threads::Threads ZLIB::ZLIB)
    
    install(TARGETS Ptex_dynamic EXPORT Ptex DESTINATION ${CMAKE_INSTALL_LIBDIR}) endif()

diff --git a/src/utils/CMakeLists.txt b/src/utils/CMakeLists.txt index d0295cb..f6bd83d 100644 --- a/src/utils/CMakeLists.txt +++ b/src/utils/CMakeLists.txt @@ -4,6 +4,6 @@ if (PTEX_BUILD_STATIC_LIBS) add_definitions(-DPTEX_STATIC) endif()

-target_link_libraries(ptxinfo ${PTEX_LIBRARY} PkgConfig::Ptex_ZLIB) +target_link_libraries(ptxinfo ${PTEX_LIBRARY} ZLIB::ZLIB)

install(TARGETS ptxinfo DESTINATION ${CMAKE_INSTALL_BINDIR})

On Sun, 6 Nov 2022 at 08:57, Matt Pharr @.***> wrote:

It would be good to update pbrt to use a modern version of ptex, but unfortunately it's not straightforward. I've pushed a branch, update-ptex, that updates Ptex to v2.4.2 but it causes build breakage on windows: https://github.com/mmp/pbrt-v4/actions/runs/3401367217 (though works elsewhere). I don't have the time dig into the further at the moment, but would happily accept a PR that got it all working!

— Reply to this email directly, view it on GitHub https://github.com/mmp/pbrt-v4/issues/297#issuecomment-1304624241, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOYQXJHWLCAMCHQFH5M3Q3WG23Y7ANCNFSM6AAAAAARD4OT5I . You are receiving this because you are subscribed to this thread.Message ID: @.***>

anderslanglands avatar Nov 05 '22 22:11 anderslanglands

AFAICT this requires adding an EXPORT parameter to zlib's install command. I've made a PR to mitsuba's zlib fork to add that here: https://github.com/mitsuba-renderer/zlib/pull/1

I've tested it on your branch and the configuration succeeds, but the build fails with an unrelated(?) error:

FAILED: CMakeFiles/pbrt_lib.dir/src/pbrt/textures.cpp.obj C:\PROGRA~2\MICROS~4\2019\COMMUN~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe /nologo /TP -DNOMINMAX -DPBRT_DEBUG_BUILD -DPBRT_HAS_INTRIN_H -DPBRT_HAVE__ALIGNED_MALLOC -DPBRT_IS_MSVC -DPBRT_IS_WINDOWS -DPBRT_NOINLINE=__declspec(noinline) -DPBRT_RESTRICT=__restrict -DPTEX_STATIC -D_CRT_SECURE_NO_WARNINGS -D_ENABLE_EXTENDED_ALIGNED_STORAGE -I..\src -I..\src\ext -I..\src\ext\stb -I..\src\ext\qoi -I..\src\ext\openexr\IlmBase\Imath -I..\src\ext\openexr\IlmBase\Half -I..\src\ext\openexr\IlmBase\Iex -I..\src\ext\openexr\OpenEXR\IlmImf -Isrc\ext\openexr\IlmBase\config -Isrc\ext\openexr\OpenEXR\config -I..\src\ext\zlib -Isrc\ext\zlib -I..\src\ext\libdeflate -I..\src\ext\filesystem -I..\src\ext\ptex\src\ptex -I..\src\ext\double-conversion -I..\src\ext\openvdb\nanovdb -I. -I..\src\ext\glfw\include -I..\src\ext\glad\include /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /Zi /Ob0 /Od /RTC1 /wd4244 /wd4267 /wd4305 /wd4552 /wd4838 /wd4843 /wd26451 /wd26495 /wd4334 -std:c++17 /showIncludes /FoCMakeFiles\pbrt_lib.dir\src\pbrt\textures.cpp.obj /FdCMakeFiles\pbrt_lib.dir\pbrt_lib.pdb /FS -c ..\src\pbrt\textures.cpp ..\src\pbrt\textures.cpp(836): error C2679: binary '=': no operator found which takes a right-hand operand of type 'COLORREF' (or there is no acceptable conversion) C:\Users\anders\code\pbrt-v4\src\pbrt/util/color.h(152): note: could be 'pbrt::RGB &pbrt::RGB::operator =(pbrt::RGB &&)' C:\Users\anders\code\pbrt-v4\src\pbrt/util/color.h(152): note: or 'pbrt::RGB &pbrt::RGB::operator =(const pbrt::RGB &)' ..\src\pbrt\textures.cpp(836): note: while trying to match the argument list '(pbrt::RGB, COLORREF)'

On Sun, 6 Nov 2022 at 08:57, Matt Pharr @.***> wrote:

It would be good to update pbrt to use a modern version of ptex, but unfortunately it's not straightforward. I've pushed a branch, update-ptex, that updates Ptex to v2.4.2 but it causes build breakage on windows: https://github.com/mmp/pbrt-v4/actions/runs/3401367217 (though works elsewhere). I don't have the time dig into the further at the moment, but would happily accept a PR that got it all working!

— Reply to this email directly, view it on GitHub https://github.com/mmp/pbrt-v4/issues/297#issuecomment-1304624241, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOYQXJHWLCAMCHQFH5M3Q3WG23Y7ANCNFSM6AAAAAARD4OT5I . You are receiving this because you are subscribed to this thread.Message ID: @.***>

anderslanglands avatar Nov 06 '22 02:11 anderslanglands

Hi.. if merge the latest Ptex repo is a problem, one possible alternative is apply only the MinGW patch changes to the current code.

diff --git "a/src/ptex/PtexInt.h" "b/src/ptex/PtexInt.h"
index cae8743..797a111 100644
--- "a/src/ptex/PtexInt.h"
+++ "b/src/ptex/PtexInt.h"
@@ -43,7 +43,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGES.
 
 #if defined(_WIN32) || defined(_WINDOWS) || defined(_MSC_VER)
 
-#if defined(_MSC_VER) && _MSC_VER >= 1600
+#if defined(_MSC_VER) && _MSC_VER >= 1600 || defined(__MINGW64__)
 #include <stdint.h>
 #else
 typedef __int8            int8_t;

I applied to my local branch and is working fine. Cheers..

pbrt4bounty avatar Nov 06 '22 11:11 pbrt4bounty

@anderslanglands I bet you a dollar that if you add

#ifdef RGB
#undef RGB
#endif

after the last #include in textures.cpp, the build will work; can you try? On Windows, the windgi.h header has the lovely habit of #defineing RGB to something or other, which causes errors like that.

Assuming @wjakob merges that zlib PR soon, I'll make another try given all of the above and hopefully that will do it.

(@pbrt4bounty I'd rather get up to date with ptex in general rather than just fix this one issue, since pbrt currently is stuck on a pretty old version of it...)

mmp avatar Nov 07 '22 13:11 mmp

Seems that the CI build fail.. I need to made more test. I'm sorry

pbrt4bounty avatar Nov 07 '22 14:11 pbrt4bounty