Merge branch 'dontcrashonerror' into 'master'

Ensure error marker assignment is thread safe

Closes #8176

See merge request OpenMW/openmw!4928

apps/components_tests/CMakeLists.txt

@@ -84,6 +84,7 @@ file(GLOB UNITTEST_SRC_FILES
     esmterrain/testgridsampling.cpp
 
     resource/testobjectcache.cpp
+    resource/testresourcesystem.cpp
 
     vfs/testpathutil.cpp
 

apps/components_tests/resource/testresourcesystem.cpp

@@ -0,0 +1,32 @@
+#include <components/resource/resourcesystem.hpp>
+#include <components/resource/scenemanager.hpp>
+#include <components/toutf8/toutf8.hpp>
+#include <components/vfs/manager.hpp>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include <thread>
+
+namespace
+{
+    using namespace testing;
+
+    TEST(ResourceResourceSystem, scenemanager_getinstance_should_be_thread_safe)
+    {
+        const VFS::Manager vfsManager;
+        const ToUTF8::Utf8Encoder encoder(ToUTF8::WINDOWS_1252);
+        Resource::ResourceSystem resourceSystem(&vfsManager, 1.0, &encoder.getStatelessEncoder());
+        Resource::SceneManager* sceneManager = resourceSystem.getSceneManager();
+
+        constexpr VFS::Path::NormalizedView noSuchPath("meshes/whatever.nif");
+        std::vector<std::thread> threads;
+
+        for (int i = 0; i < 50; ++i)
+        {
+            threads.emplace_back([=]() { sceneManager->getInstance(noSuchPath); });
+        }
+        for (std::thread& thread : threads)
+            thread.join();
+    }
+}

components/resource/scenemanager.cpp

@@ -446,15 +446,6 @@ namespace Resource
         Resource::NifFileManager* nifFileManager, Resource::BgsmFileManager* bgsmFileManager, double expiryDelay)
         : ResourceManager(vfs, expiryDelay)
         , mShaderManager(new Shader::ShaderManager)
-        , mForceShaders(false)
-        , mClampLighting(true)
-        , mAutoUseNormalMaps(false)
-        , mAutoUseSpecularMaps(false)
-        , mApplyLightingToEnvMaps(false)
-        , mLightingMethod(SceneUtil::LightingMethod::FFP)
-        , mConvertAlphaTestToAlphaToCoverage(false)
-        , mAdjustCoverageForAlphaTest(false)
-        , mSupportsNormalsRT(false)
         , mSharedStateManager(new SharedStateManager)
         , mImageManager(imageManager)
         , mNifFileManager(nifFileManager)
@@ -462,8 +453,8 @@ namespace Resource
         , mMinFilter(osg::Texture::LINEAR_MIPMAP_LINEAR)
         , mMagFilter(osg::Texture::LINEAR)
         , mMaxAnisotropy(1)
-        , mUnRefImageDataAfterApply(false)
         , mParticleSystemMask(~0u)
+        , mLightingMethod(SceneUtil::LightingMethod::FFP)
     {
     }
 
@@ -988,8 +979,7 @@ namespace Resource
 
     osg::ref_ptr<osg::Node> SceneManager::cloneErrorMarker()
     {
-        if (!mErrorMarker)
+        std::call_once(mErrorMarkerFlag, [this] { mErrorMarker = loadErrorMarker(); });
-            mErrorMarker = loadErrorMarker();
 
         return static_cast<osg::Node*>(mErrorMarker->clone(osg::CopyOp::DEEP_COPY_ALL));
     }

components/resource/scenemanager.hpp

@@ -237,42 +237,43 @@ namespace Resource
         osg::ref_ptr<osg::Node> loadErrorMarker();
         osg::ref_ptr<osg::Node> cloneErrorMarker();
 
+        mutable std::mutex mSharedStateMutex;
+
         std::unique_ptr<Shader::ShaderManager> mShaderManager;
-        bool mForceShaders;
-        bool mClampLighting;
-        bool mAutoUseNormalMaps;
         std::string mNormalMapPattern;
         std::string mNormalHeightMapPattern;
-        bool mAutoUseSpecularMaps;
         std::string mSpecularMapPattern;
-        bool mApplyLightingToEnvMaps;
-        SceneUtil::LightingMethod mLightingMethod;
-        SceneUtil::LightManager::SupportedMethods mSupportedLightingMethods;
-        bool mConvertAlphaTestToAlphaToCoverage;
-        bool mAdjustCoverageForAlphaTest;
-        bool mSupportsNormalsRT;
         std::array<osg::ref_ptr<osg::Texture>, 2> mOpaqueDepthTex;
-        bool mWeatherParticleOcclusion = false;
 
         osg::ref_ptr<Resource::SharedStateManager> mSharedStateManager;
-        mutable std::mutex mSharedStateMutex;
 
         Resource::ImageManager* mImageManager;
         Resource::NifFileManager* mNifFileManager;
         Resource::BgsmFileManager* mBgsmFileManager;
+        osg::ref_ptr<osgUtil::IncrementalCompileOperation> mIncrementalCompileOperation;
+        mutable osg::ref_ptr<osg::Node> mErrorMarker;
+        mutable std::once_flag mErrorMarkerFlag;
 
         osg::Texture::FilterMode mMinFilter;
         osg::Texture::FilterMode mMagFilter;
         int mMaxAnisotropy;
-        bool mUnRefImageDataAfterApply;
-
-        osg::ref_ptr<osgUtil::IncrementalCompileOperation> mIncrementalCompileOperation;
 
         unsigned int mParticleSystemMask;
-        mutable osg::ref_ptr<osg::Node> mErrorMarker;
+        SceneUtil::LightingMethod mLightingMethod;
+        SceneUtil::LightManager::SupportedMethods mSupportedLightingMethods;
+        bool mForceShaders = false;
+        bool mClampLighting = true;
+        bool mAutoUseNormalMaps = false;
+        bool mAutoUseSpecularMaps = false;
+        bool mApplyLightingToEnvMaps = false;
+        bool mConvertAlphaTestToAlphaToCoverage = false;
+        bool mAdjustCoverageForAlphaTest = false;
+        bool mSupportsNormalsRT = false;
+        bool mWeatherParticleOcclusion = false;
+        bool mUnRefImageDataAfterApply = false;
 
-        SceneManager(const SceneManager&);
+        SceneManager(const SceneManager&) = delete;
-        void operator=(const SceneManager&);
+        void operator=(const SceneManager&) = delete;
     };
 }