USD Fixes for building on windows.
AbandonedPublic
Actions

Authored by Ray Molenkamp (LazyDodo) on Jul 28 2019, 11:11 PM.

Details

Reviewers

Sybren A. Stüvel (sybren)
Brecht Van Lommel (brecht)

Summary

Mostly minor stuff, due to the use of thread_local in usd, i had to delay load opengl32 since when you use mesa it'll spin off a bunch of threads before the crt on the main thread is initialized leading to a crash due to uninitialized locks.

debug_timer.h used some none ISO code which MSVC didn't support and it just stubbed it out for windows.

Diff Detail

Repository

rB Blender

Branch

tmp_usd_winfix2 (branched from master)

Build Status

Buildable 4243
Build 4243: arc lint + arc unit

Event Timeline

Ray Molenkamp (LazyDodo) created this revision.Jul 28 2019, 11:11 PM

Harbormaster completed remote builds in B4187: Diff 16642.Jul 28 2019, 11:11 PM

Thanks for the patch.

due to the use of thread_local in usd, i had to delay load opengl32 since when you use mesa it'll spin off a bunch of threads before the crt on the main thread is initialized leading to a crash due to uninitialized locks.

Can you add this to a comment in the file? That way this knowledge doesn't get lost.

debug_timer.h used some none ISO code which MSVC didn't support and it just stubbed it out for windows.

That's fine, the code will probably be removed before merging to master.

This revision now requires changes to proceed.Jul 30 2019, 4:39 PM

In D5359#121423, @Sybren A. Stüvel (sybren) wrote:

due to the use of thread_local in usd, i had to delay load opengl32 since when you use mesa it'll spin off a bunch of threads before the crt on the main thread is initialized leading to a crash due to uninitialized locks.

Can you add this to a comment in the file? That way this knowledge doesn't get lost.

I'd rather land that bit (with comment) of the patch in master then, the change is not USD related, any C++11 code that uses thread_local can trigger the problem. USD is just the first to run into it.

removed delayloading

Harbormaster completed remote builds in B4243: Diff 16750.Aug 1 2019, 3:34 AM

separated out the delay load for master in D5387

Brecht Van Lommel (brecht) added a subscriber: Brecht Van Lommel (brecht).Aug 12 2019, 11:22 AM

Brecht Van Lommel (brecht) added inline comments.

source/creator/CMakeLists.txt
1053	What is this and why is it not needed for debug builds and non-Windows platforms?

Ray Molenkamp (LazyDodo) added inline comments.Aug 12 2019, 3:12 PM

source/creator/CMakeLists.txt
1053	It's a bunch of json USD insists on having, currently windows only, since i'm not sure how other platforms deal with this, debug is left out since it's a sloppy copy/paste job.

USD/Win: Copy usd registry for debug builds as well

Harbormaster completed remote builds in B4384: Diff 17046.Aug 12 2019, 4:11 PM

Retry, fixing last update

Harbormaster completed remote builds in B4385: Diff 17047.Aug 12 2019, 4:12 PM

Fixed debug configuration missing the registry, left it for win32 since it's unknown how other platforms deal with this

Looks good to me but this is for @Sybren A. Stüvel (sybren) to handle really.

source/creator/CMakeLists.txt
1053	Nitpick for comment style: `# USD registry`

Sybren A. Stüvel (sybren) added inline comments.Aug 16 2019, 2:19 PM

source/blender/usd/CMakeLists.txt
51	Why is this necessary? Can we use something that's given to us by `${USD_PATH}/pxrConfig.cmake` to configure the required definitions?

Ray Molenkamp (LazyDodo) added inline comments.Aug 16 2019, 2:34 PM

source/blender/usd/CMakeLists.txt
51	usd drags in the tbb headers and tbb in turn drags in `windows.h` which by default defines `min` and `max` macro's causing issues with c++'s `std::min` and `std::max` unless you define this symbol. Given USD technically doesn't cause the issue we can't really expect `${USD_PATH}/pxrConfig.cmake` to fix it. One option would be to define it at a global scope, but there are some deps that do fix this issue them selves and that'll cause annoying NOMINMAX redefinition warns, which aren't great either. soo yeah.. we're kinda stuck with sprinkling this nonsense wherever we need it on a case by case basis.