From 80a0398f9d987aa37c21c1f1b0c19ec6a4817ae8 Mon Sep 17 00:00:00 2001 From: scrawl Date: Tue, 7 Mar 2017 00:25:04 +0100 Subject: [PATCH] Load LandData into the LandObject to avoid threading conflicts when the same data is being loaded by two threads --- apps/opencs/view/render/terrainstorage.cpp | 4 +- components/esm/loadland.cpp | 44 +++++++++++----------- components/esm/loadland.hpp | 13 +++---- components/esmterrain/storage.cpp | 8 ++-- components/esmterrain/storage.hpp | 21 ++++++----- 5 files changed, 43 insertions(+), 47 deletions(-) diff --git a/apps/opencs/view/render/terrainstorage.cpp b/apps/opencs/view/render/terrainstorage.cpp index 9894ce17c..c63d41be3 100644 --- a/apps/opencs/view/render/terrainstorage.cpp +++ b/apps/opencs/view/render/terrainstorage.cpp @@ -21,9 +21,7 @@ namespace CSVRender return NULL; const ESM::Land& land = mData.getLand().getRecord(index).get(); - int mask = ESM::Land::DATA_VHGT | ESM::Land::DATA_VNML | ESM::Land::DATA_VCLR | ESM::Land::DATA_VTEX; - land.loadData (mask); - return new ESMTerrain::LandObject(&land, 0); + return new ESMTerrain::LandObject(&land, ESM::Land::DATA_VHGT | ESM::Land::DATA_VNML | ESM::Land::DATA_VCLR | ESM::Land::DATA_VTEX); } const ESM::LandTexture* TerrainStorage::getLandTexture(int index, short plugin) diff --git a/components/esm/loadland.cpp b/components/esm/loadland.cpp index 858b45704..8d57c72eb 100644 --- a/components/esm/loadland.cpp +++ b/components/esm/loadland.cpp @@ -2,8 +2,6 @@ #include -#include - #include "esmreader.hpp" #include "esmwriter.hpp" #include "defs.hpp" @@ -177,45 +175,48 @@ namespace ESM } - void Land::loadData(int flags) const + void Land::loadData(int flags, LandData* target) const { - OpenThreads::ScopedLock lock(mMutex); + // Create storage if nothing is loaded + if (!target && !mLandData) + { + mLandData = new LandData; + } + + if (!target) + target = mLandData; // Try to load only available data flags = flags & mDataTypes; // Return if all required data is loaded - if (mLandData && (mLandData->mDataLoaded & flags) == flags) { + if ((target->mDataLoaded & flags) == flags) { return; } - // Create storage if nothing is loaded - if (mLandData == NULL) { - mLandData = new LandData; - } ESM::ESMReader reader; reader.restoreContext(mContext); if (reader.isNextSub("VNML")) { - condLoad(reader, flags, DATA_VNML, mLandData->mNormals, sizeof(mLandData->mNormals)); + condLoad(reader, flags, target->mDataLoaded, DATA_VNML, target->mNormals, sizeof(target->mNormals)); } if (reader.isNextSub("VHGT")) { VHGT vhgt; - if (condLoad(reader, flags, DATA_VHGT, &vhgt, sizeof(vhgt))) { + if (condLoad(reader, flags, target->mDataLoaded, DATA_VHGT, &vhgt, sizeof(vhgt))) { float rowOffset = vhgt.mHeightOffset; for (int y = 0; y < LAND_SIZE; y++) { rowOffset += vhgt.mHeightData[y * LAND_SIZE]; - mLandData->mHeights[y * LAND_SIZE] = rowOffset * HEIGHT_SCALE; + target->mHeights[y * LAND_SIZE] = rowOffset * HEIGHT_SCALE; float colOffset = rowOffset; for (int x = 1; x < LAND_SIZE; x++) { colOffset += vhgt.mHeightData[y * LAND_SIZE + x]; - mLandData->mHeights[x + y * LAND_SIZE] = colOffset * HEIGHT_SCALE; + target->mHeights[x + y * LAND_SIZE] = colOffset * HEIGHT_SCALE; } } - mLandData->mUnk1 = vhgt.mUnk1; - mLandData->mUnk2 = vhgt.mUnk2; + target->mUnk1 = vhgt.mUnk1; + target->mUnk2 = vhgt.mUnk2; } } @@ -223,11 +224,11 @@ namespace ESM reader.skipHSub(); if (reader.isNextSub("VCLR")) - condLoad(reader, flags, DATA_VCLR, mLandData->mColours, 3 * LAND_NUM_VERTS); + condLoad(reader, flags, target->mDataLoaded, DATA_VCLR, target->mColours, 3 * LAND_NUM_VERTS); if (reader.isNextSub("VTEX")) { uint16_t vtex[LAND_NUM_TEXTURES]; - if (condLoad(reader, flags, DATA_VTEX, vtex, sizeof(vtex))) { - transposeTextureData(vtex, mLandData->mTextures); + if (condLoad(reader, flags, target->mDataLoaded, DATA_VTEX, vtex, sizeof(vtex))) { + transposeTextureData(vtex, target->mTextures); } } } @@ -241,11 +242,11 @@ namespace ESM } } - bool Land::condLoad(ESM::ESMReader& reader, int flags, int dataFlag, void *ptr, unsigned int size) const + bool Land::condLoad(ESM::ESMReader& reader, int flags, int& targetFlags, int dataFlag, void *ptr, unsigned int size) const { - if ((mLandData->mDataLoaded & dataFlag) == 0 && (flags & dataFlag) != 0) { + if ((targetFlags & dataFlag) == 0 && (flags & dataFlag) != 0) { reader.getHExact(ptr, size); - mLandData->mDataLoaded |= dataFlag; + targetFlags |= dataFlag; return true; } reader.skipHSubSize(size); @@ -254,7 +255,6 @@ namespace ESM bool Land::isDataLoaded(int flags) const { - OpenThreads::ScopedLock lock(mMutex); return mLandData && (mLandData->mDataLoaded & flags) == (flags & mDataTypes); } diff --git a/components/esm/loadland.hpp b/components/esm/loadland.hpp index eaffb2e46..261708893 100644 --- a/components/esm/loadland.hpp +++ b/components/esm/loadland.hpp @@ -3,8 +3,6 @@ #include -#include - #include "esmcommon.hpp" namespace ESM @@ -117,12 +115,13 @@ struct Land void blank() {} /** - * Actually loads data + * Actually loads data into target + * If target is NULL, assumed target is mLandData */ - void loadData(int flags) const; + void loadData(int flags, LandData* target = NULL) const; /** - * Frees memory allocated for land data + * Frees memory allocated for mLandData */ void unloadData() const; @@ -160,9 +159,7 @@ struct Land /// Loads data and marks it as loaded /// \return true if data is actually loaded from file, false otherwise /// including the case when data is already loaded - bool condLoad(ESM::ESMReader& reader, int flags, int dataFlag, void *ptr, unsigned int size) const; - - mutable OpenThreads::Mutex mMutex; + bool condLoad(ESM::ESMReader& reader, int flags, int& targetFlags, int dataFlag, void *ptr, unsigned int size) const; mutable LandData *mLandData; }; diff --git a/components/esmterrain/storage.cpp b/components/esmterrain/storage.cpp index 5d1591e79..3ce8f4882 100644 --- a/components/esmterrain/storage.cpp +++ b/components/esmterrain/storage.cpp @@ -25,7 +25,7 @@ namespace ESMTerrain : mLand(land) , mLoadFlags(loadFlags) { - mLand->loadData(mLoadFlags); + mLand->loadData(mLoadFlags, &mData); } LandObject::LandObject(const LandObject ©, const osg::CopyOp ©op) @@ -34,13 +34,13 @@ namespace ESMTerrain LandObject::~LandObject() { - if (mLand && mLoadFlags) // only unload if we were responsible for loading to begin with. - mLand->unloadData(); } const ESM::Land::LandData *LandObject::getData(int flags) const { - return mLand->getLandData(flags); + if ((mData.mDataLoaded & flags) != flags) + return NULL; + return &mData; } int LandObject::getPlugin() const diff --git a/components/esmterrain/storage.hpp b/components/esmterrain/storage.hpp index 7502f44cd..e159313c5 100644 --- a/components/esmterrain/storage.hpp +++ b/components/esmterrain/storage.hpp @@ -16,24 +16,25 @@ namespace VFS namespace ESMTerrain { - /// @brief Wrapper around ESM::Land with reference counting. The wrapper needs to be held as long as the data is still in use - /// Data will be unloaded when wrapper object is deleted + /// @brief Wrapper around Land Data with reference counting. The wrapper needs to be held as long as the data is still in use class LandObject : public osg::Object { - private: - const ESM::Land* mLand; - int mLoadFlags; - public: + LandObject(); + LandObject(const ESM::Land* land, int loadFlags); + LandObject(const LandObject& copy, const osg::CopyOp& copyop); + virtual ~LandObject(); + META_Object(ESMTerrain, LandObject) const ESM::Land::LandData* getData(int flags) const; int getPlugin() const; - LandObject(); - LandObject(const ESM::Land* land, int loadFlags); - LandObject(const LandObject& copy, const osg::CopyOp& copyop); - virtual ~LandObject(); + private: + const ESM::Land* mLand; + int mLoadFlags; + + ESM::Land::LandData mData; }; /// @brief Feeds data from ESM terrain records (ESM::Land, ESM::LandTexture)