From 1c5a3f7ef53140f1d04ffd77b4cbd321f5177ed7 Mon Sep 17 00:00:00 2001 From: Daviw Williams Date: Fri, 25 Oct 2013 12:44:40 +0200 Subject: [PATCH 1/3] Reverted attempts to suppress GCC warnings and just removed the offending asserts instead. Revert "Added asserts to catch invalid material/density values." This reverts commit 89438220464778d167d86bb59e095a85ccdba080. --- .../include/PolyVoxCore/Impl/ErrorHandling.h | 17 +----------- .../include/PolyVoxCore/MaterialDensityPair.h | 27 ++----------------- 2 files changed, 3 insertions(+), 41 deletions(-) diff --git a/library/PolyVoxCore/include/PolyVoxCore/Impl/ErrorHandling.h b/library/PolyVoxCore/include/PolyVoxCore/Impl/ErrorHandling.h index b6c656eb..5c6f6b0f 100644 --- a/library/PolyVoxCore/include/PolyVoxCore/Impl/ErrorHandling.h +++ b/library/PolyVoxCore/include/PolyVoxCore/Impl/ErrorHandling.h @@ -48,12 +48,7 @@ freely, subject to the following restrictions: // Macros cannot contain #ifdefs, but some of our macros need to disable warnings and such warning supression is // platform specific. But macros can contain other macros, so we create macros to control the warnings and use -// those instead. -// -// Note that we have seperate macros for POLYVOX_MSC_..., POLYVOX_GCC_..., etc. In princpiple we could have just one -// as compilers should ignore pragmas they don't recognise, but in practice at least MSVC complains about this as -// well. So in practice it's just eaier to have seperate macros. We could look into the compiler switch to not warn -// on unrecognised pragmas though. +// those instead. This set of warning supression macros can be extended to GCC/Clang when required. #if defined(_MSC_VER) #define POLYVOX_MSC_WARNING_PUSH __pragma(warning(push)) #define POLYVOX_DISABLE_MSC_WARNING(x) __pragma(warning(disable:x)) @@ -64,16 +59,6 @@ freely, subject to the following restrictions: #define POLYVOX_MSC_WARNING_POP #endif -#if defined(__GNUC__) - #define POLYVOX_GCC_WARNING_PUSH #pragma GCC diagnostic push - #define POLYVOX_DISABLE_GCC_WARNING(x) #pragma GCC diagnostic ignored x - #define POLYVOX_GCC_WARNING_POP #pragma GCC diagnostic pop -#else - #define POLYVOX_GCC_WARNING_PUSH - #define POLYVOX_DISABLE_GCC_WARNING(x) - #define POLYVOX_GCC_WARNING_POP -#endif - #define POLYVOX_UNUSED(x) do { (void)sizeof(x); } while(0) /* diff --git a/library/PolyVoxCore/include/PolyVoxCore/MaterialDensityPair.h b/library/PolyVoxCore/include/PolyVoxCore/MaterialDensityPair.h index a051f74e..cdde07f7 100644 --- a/library/PolyVoxCore/include/PolyVoxCore/MaterialDensityPair.h +++ b/library/PolyVoxCore/include/PolyVoxCore/MaterialDensityPair.h @@ -75,31 +75,8 @@ namespace PolyVox Type getDensity() const { return m_uDensity; } Type getMaterial() const { return m_uMaterial; } - void setDensity(Type uDensity) - { - // Depending on our underlying type it may be impossible for the assert below to be triggered (i.e. if density is stored as - // Type, rather than just using a few bits of Type). GCC will warn about this but it's redundant so we diable the warning. - POLYVOX_GCC_WARNING_PUSH - POLYVOX_DISABLE_GCC_WARNING("-Wtype-limits") - POLYVOX_ASSERT(uDensity >= getMinDensity(), "Density out of range"); - POLYVOX_ASSERT(uDensity <= getMaxDensity(), "Density out of range"); - POLYVOX_GCC_WARNING_POP - - m_uDensity = uDensity; - } - - void setMaterial(Type uMaterial) - { - // Depending on our underlying type it may be impossible for the assert below to be triggered (i.e. if material is stored as - // Type, rather than just using a few bits of Type). GCC will warn about this but it's redundant so we diable the warning. - POLYVOX_GCC_WARNING_PUSH - POLYVOX_DISABLE_GCC_WARNING("-Wtype-limits") - POLYVOX_ASSERT(uMaterial >= 0, "Material out of range"); - POLYVOX_ASSERT(uMaterial <= (0x01 << NoOfMaterialBits) - 1, "Material out of range"); - POLYVOX_GCC_WARNING_POP - - m_uMaterial = uMaterial; - } + void setDensity(Type uDensity) { m_uDensity = uDensity; } + void setMaterial(Type uMaterial) { m_uMaterial = uMaterial; } static Type getMaxDensity() { return (0x01 << NoOfDensityBits) - 1; } static Type getMinDensity() { return 0; } From 85d8bdb30c40e74b19338bc0055b689e98ad5b4c Mon Sep 17 00:00:00 2001 From: Daviw Williams Date: Tue, 12 Nov 2013 16:47:57 +0100 Subject: [PATCH 2/3] Rearranged some code to try and improve the robustness of block paging and compression. --- .../include/PolyVoxCore/LargeVolume.h | 4 +- .../include/PolyVoxCore/LargeVolume.inl | 57 +++++++------------ 2 files changed, 24 insertions(+), 37 deletions(-) diff --git a/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.h b/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.h index a26bc29b..5d02efea 100644 --- a/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.h +++ b/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.h @@ -319,8 +319,8 @@ namespace PolyVox uint32_t calculateBlockMemoryUsage(void) const; - void flushOldestExcessiveBlocks(void) const; - void flushExcessiveCacheEntries(void) const; + void ensureCompressedBlockMapHasFreeSpace(void) const; + void ensureUncompressedBlockMapHasFreeSpace(void) const; void initialise(); diff --git a/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.inl b/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.inl index 133a6ef8..32e92fa1 100644 --- a/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.inl +++ b/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.inl @@ -625,17 +625,14 @@ namespace PolyVox if(itBlock != m_pBlocks.end()) { pBlock = itBlock->second; - pBlock->m_uBlockLastAccessed = ++m_uTimestamper; - return pBlock; } else { + ensureCompressedBlockMapHasFreeSpace(); + // The block wasn't found so we create a new one pBlock = new CompressedBlock; - // It's important to set the timestamp before we flush later. - pBlock->m_uBlockLastAccessed = ++m_uTimestamper; - // Pass the block to the Pager to give it a chance to initialise it with any data Vector3DInt32 v3dLower(v3dBlockPos.getX() << m_uBlockSideLengthPower, v3dBlockPos.getY() << m_uBlockSideLengthPower, v3dBlockPos.getZ() << m_uBlockSideLengthPower); Vector3DInt32 v3dUpper = v3dLower + Vector3DInt32(m_uBlockSideLength-1, m_uBlockSideLength-1, m_uBlockSideLength-1); @@ -644,12 +641,10 @@ namespace PolyVox // Add the block to the map itBlock = m_pBlocks.insert(std::make_pair(v3dBlockPos, pBlock)).first; - - // Paging in this new block may mean we are now using too much memory. If necessary, flush some old blocks. - flushOldestExcessiveBlocks(); - - return pBlock; } + + pBlock->m_uBlockLastAccessed = ++m_uTimestamper; + return pBlock; } template @@ -666,30 +661,22 @@ namespace PolyVox return m_pLastAccessedBlock; } + UncompressedBlock* pUncompressedBlock = 0; + typename UncompressedBlockMap::iterator itUncompressedBlock = m_pUncompressedBlockCache.find(v3dBlockPos); // check whether the block is already loaded if(itUncompressedBlock != m_pUncompressedBlockCache.end()) { - UncompressedBlock* pUncompressedBlock = itUncompressedBlock->second; - - pUncompressedBlock->m_uBlockLastAccessed = ++m_uTimestamper; - m_pLastAccessedBlock = pUncompressedBlock; - m_v3dLastAccessedBlockPos = v3dBlockPos; - - return pUncompressedBlock; + pUncompressedBlock = itUncompressedBlock->second; } else { - // At this point we just create a new block. - UncompressedBlock* pUncompressedBlock = new UncompressedBlock(m_uBlockSideLength); - - // It's important to set the timestamp before we flush later. - pUncompressedBlock->m_uBlockLastAccessed = ++m_uTimestamper; + // At this point we know that the uncompresed block did not exist in the cache. We will + // create it and add it to the cache, which means we need to make sure there is space. + ensureUncompressedBlockMapHasFreeSpace(); - // We set these before flushing because the flush can cause block to be erased, and there - // is a test to make sure the block which is being erase is not the last accessed block. - m_pLastAccessedBlock = pUncompressedBlock; - m_v3dLastAccessedBlockPos = v3dBlockPos; + // We can now create a new block. + pUncompressedBlock = new UncompressedBlock(m_uBlockSideLength); // An uncompressed bock is always backed by a compressed one, and this is created by getCompressedBlock() if it doesn't // already exist. If it does already exist and has data then we bring this across into the ucompressed version. @@ -702,14 +689,14 @@ namespace PolyVox } // Add our new block to the map. - m_pUncompressedBlockCache.insert(std::make_pair(v3dBlockPos, pUncompressedBlock)); - - // Our block cache may now have grown too large. Flush some entries if necessary. - // FIXME - Watch out for flushing the block we just created! - flushExcessiveCacheEntries(); - - return pUncompressedBlock; + m_pUncompressedBlockCache.insert(std::make_pair(v3dBlockPos, pUncompressedBlock)); } + + pUncompressedBlock->m_uBlockLastAccessed = ++m_uTimestamper; + m_pLastAccessedBlock = pUncompressedBlock; + m_v3dLastAccessedBlockPos = v3dBlockPos; + + return pUncompressedBlock; } //////////////////////////////////////////////////////////////////////////////// @@ -762,7 +749,7 @@ namespace PolyVox } template - void LargeVolume::flushOldestExcessiveBlocks(void) const + void LargeVolume::ensureCompressedBlockMapHasFreeSpace(void) const { while(m_pBlocks.size() > m_uMaxNumberOfBlocksInMemory) { @@ -784,7 +771,7 @@ namespace PolyVox } template - void LargeVolume::flushExcessiveCacheEntries(void) const + void LargeVolume::ensureUncompressedBlockMapHasFreeSpace(void) const { while(m_pUncompressedBlockCache.size() > m_uMaxNumberOfUncompressedBlocks) { From 5294efc473e54e9104e51ecd5bdc1ff2404dcece Mon Sep 17 00:00:00 2001 From: Daviw Williams Date: Tue, 12 Nov 2013 16:57:11 +0100 Subject: [PATCH 3/3] Clear the 'last accessed block' flag before flushing the volume. --- .../include/PolyVoxCore/LargeVolume.inl | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.inl b/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.inl index 32e92fa1..ab863dc6 100644 --- a/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.inl +++ b/library/PolyVoxCore/include/PolyVoxCore/LargeVolume.inl @@ -436,6 +436,9 @@ namespace PolyVox { typename CompressedBlockMap::iterator i; + // Flushing will remove the most accessed block, so invalidate the pointer. + m_pLastAccessedBlock = 0; + //Replaced the for loop here as the call to //eraseBlock was invalidating the iterator. while(m_pUncompressedBlockCache.size() > 0) @@ -585,14 +588,10 @@ namespace PolyVox { UncompressedBlock* pUncompressedBlock = itUncompressedBlock->second; - // This should not often happen as blocks are normally deleted based on being least recently used. - // However, I can imagine that flushing a large number of blocks could cause this to occur. Just - // to be safe we handle it by invalidating the last accessed block pointer. - if(pUncompressedBlock == m_pLastAccessedBlock) - { - logWarning() << "The last accessed block is being erased from the uncompressed cache."; - m_pLastAccessedBlock = 0; - } + // This should never happen as blocks are deleted based on being least recently used. + // I the case that we are flushing we delete all blocks, but the flush function will + // reset the 'm_pLastAccessedBlock' anyway to prevent it being accidentally reused. + POLYVOX_ASSERT(pUncompressedBlock != m_pLastAccessedBlock, "Attempted to delete last accessed block."); // Before deleting the block we may need to recompress its data. We // only do this if the data has been modified since it was decompressed.