search

CesiumGS / cesium-omniverse

Bringing the 3D geospatial ecosystem to Omniverse
https://cesium.com/platform/cesium-for-omniverse/
Apache License 2.0
58 stars 7 forks source link

Remove _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR #727

Closed corybarr closed 2 months ago

corybarr commented 2 months ago

Visual Studio 17.10 created some issues with mutexes and memory access in Windows builds. Details are here:https://developercommunity.visualstudio.com/t/Visual-Studio-17100-Update-leads-to-Pr/10669759?sort=newest

Ultimately, we shouldn't need to use the _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR escape hatch.

lilleyse commented 2 months ago

This hack has also been added to the branch of cesium-native that cesium-omniverse uses: https://github.com/CesiumGS/cesium-native/compare/main...cesium-omniverse-build?expand=1

lilleyse commented 2 months ago

We should pay attention to the VC redistributables that ship with kit, and remove this workaround once they've been updated to a newer version.

kring commented 2 months ago

IMO you should do one of these:

  1. Compile Cesium for Omniverse using the same version of Visual Studio that NVidia uses to compile Omniverse itself. This is pretty easy to set up on Github Actions (if you know what version to use), so let me know if you need help. That way, the redistributable installed with Omniverse will work for Cesium, too. This is what we do in Cesium for Unreal.
  2. Use the statically-linked version of the MSVCRT, so that the redistributable isn't need at all. This is what we do in Cesium for Unity.
  3. Install the right MSVCRT redistributable as part of the the Cesium for Omniverse installation. Not a real strong reason to go with this one that I can think of, but it's an option if you want to use a particular compiler version and don't want to use the static CRT.

Having mismatched CRT versions is likely to cause problems even beyond the very obvious one introduced in 17.10, so I don't think it's a good idea to work around it with _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR.