From efce1033bc3b1306e705420a03d264bf22a84a8b Mon Sep 17 00:00:00 2001 From: dmcdiar Date: Fri, 17 Sep 2021 16:11:33 -0700 Subject: [PATCH 1/3] Instanced the RayTracingBLAS objects to enable re-use when the same model is added multiple times. Signed-off-by: dmcdiar --- .../Code/Source/Mesh/MeshFeatureProcessor.cpp | 2 +- .../RayTracing/RayTracingFeatureProcessor.cpp | 54 ++++++++++++++++--- .../RayTracing/RayTracingFeatureProcessor.h | 15 +++++- 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/Gems/Atom/Feature/Common/Code/Source/Mesh/MeshFeatureProcessor.cpp b/Gems/Atom/Feature/Common/Code/Source/Mesh/MeshFeatureProcessor.cpp index 8ab324cdf7..e243687b1c 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Mesh/MeshFeatureProcessor.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/Mesh/MeshFeatureProcessor.cpp @@ -952,7 +952,7 @@ namespace AZ subMeshes.push_back(subMesh); } - rayTracingFeatureProcessor->SetMesh(m_objectId, subMeshes); + rayTracingFeatureProcessor->SetMesh(m_objectId, m_model->GetModelAsset()->GetId(), subMeshes); } void MeshDataInstance::SetSortKey(RHI::DrawItemSortKey sortKey) diff --git a/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.cpp b/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.cpp index ea89f64b73..d10da818ee 100644 --- a/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.cpp @@ -79,7 +79,7 @@ namespace AZ AZ_Assert(m_rayTracingMaterialSrg, "Failed to create RayTracingMaterialSrg"); } - void RayTracingFeatureProcessor::SetMesh(const ObjectId objectId, const SubMeshVector& subMeshes) + void RayTracingFeatureProcessor::SetMesh(const ObjectId objectId, const AZ::Data::AssetId& assetId, const SubMeshVector& subMeshes) { if (!m_rayTracingEnabled) { @@ -92,7 +92,7 @@ namespace AZ MeshMap::iterator itMesh = m_meshes.find(objectIndex); if (itMesh == m_meshes.end()) { - m_meshes.insert(AZStd::make_pair(objectIndex, Mesh{ subMeshes })); + m_meshes.insert(AZStd::make_pair(objectIndex, Mesh{ assetId, subMeshes })); } else { @@ -102,9 +102,12 @@ namespace AZ m_meshes[objectIndex].m_subMeshes = subMeshes; } - // create the BLAS buffers for each sub-mesh + // create the BLAS buffers for each sub-mesh, or re-use existing BLAS objects if they were already created. + // Note: all sub-meshes must either create new BLAS objects or re-use existing ones, otherwise it's an error (it's the same model in both cases) // Note: the buffer is just reserved here, the BLAS is built in the RayTracingAccelerationStructurePass Mesh& mesh = m_meshes[objectIndex]; + bool blasInstanceFound = false; + for (auto& subMesh : mesh.m_subMeshes) { RHI::RayTracingBlasDescriptor blasDescriptor; @@ -115,11 +118,37 @@ namespace AZ ->IndexBuffer(subMesh.m_indexBufferView) ; - // create the BLAS object - subMesh.m_blas = AZ::RHI::RayTracingBlas::CreateRHIRayTracingBlas(); + // search for an existing BLAS object for this model + RayTracingBlasMap::iterator itBlas = m_blasMap.find(assetId); + if (itBlas != m_blasMap.end()) + { + // re-use existing BLAS + subMesh.m_blas = itBlas->second.m_blas; + itBlas->second.m_count++; - // create the buffers from the descriptor - subMesh.m_blas->CreateBuffers(*device, &blasDescriptor, *m_bufferPools); + // keep track of the fact that we re-used a BLAS + blasInstanceFound = true; + } + else + { + AZ_Assert(blasInstanceFound == false, "Partial set of RayTracingBlas objects found for mesh"); + + // create the BLAS object + subMesh.m_blas = AZ::RHI::RayTracingBlas::CreateRHIRayTracingBlas(); + + // create the buffers from the descriptor + subMesh.m_blas->CreateBuffers(*device, &blasDescriptor, *m_bufferPools); + + // store the BLAS in the side list + RayTracingBlasInstance blasInstance = { subMesh.m_blas, 1 }; + m_blasMap.insert({ assetId, blasInstance }); + } + } + + if (blasInstanceFound) + { + // set the mesh BLAS flag so we don't try to rebuild it in the RayTracingAccelerationStructurePass + mesh.m_blasBuilt = true; } // set initial transform @@ -146,6 +175,17 @@ namespace AZ m_subMeshCount -= aznumeric_cast(itMesh->second.m_subMeshes.size()); m_meshes.erase(itMesh); m_revision++; + + // decrement the count from the BLAS instance, and check to see if we can remove it + RayTracingBlasMap::iterator itBlas = m_blasMap.find(itMesh->second.m_assetId); + if (itBlas != m_blasMap.end()) + { + itBlas->second.m_count--; + if (itBlas->second.m_count == 0) + { + m_blasMap.erase(itBlas); + } + } } m_meshInfoBufferNeedsUpdate = true; diff --git a/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.h b/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.h index 6bd9829c7e..ee0a27b814 100644 --- a/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.h +++ b/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.h @@ -116,6 +116,9 @@ namespace AZ //! Contains data for the top level mesh, including the list of sub-meshes struct Mesh { + // assetId of the model + AZ::Data::AssetId m_assetId = AZ::Data::AssetId{}; + // sub-mesh list SubMeshVector m_subMeshes; @@ -134,7 +137,7 @@ namespace AZ //! Sets ray tracing data for a mesh. //! This will cause an update to the RayTracing acceleration structure on the next frame - void SetMesh(const ObjectId objectId, const SubMeshVector& subMeshes); + void SetMesh(const ObjectId objectId, const AZ::Data::AssetId& assetId, const SubMeshVector& subMeshes); //! Removes ray tracing data for a mesh. //! This will cause an update to the RayTracing acceleration structure on the next frame @@ -260,6 +263,16 @@ namespace AZ // flag indicating we need to update the materialInfo buffer bool m_materialInfoBufferNeedsUpdate = false; + + // side list for looking up existing BLAS objects so they can be re-used when the same mesh is added multiple times + struct RayTracingBlasInstance + { + RHI::Ptr m_blas; + uint32_t m_count = 0; + }; + + using RayTracingBlasMap = AZStd::unordered_map; + RayTracingBlasMap m_blasMap; }; } } From 154b6dc461815a79a958fc8f9d9ca3a2b0cf6827 Mon Sep 17 00:00:00 2001 From: dmcdiar Date: Mon, 20 Sep 2021 12:13:29 -0700 Subject: [PATCH 2/3] Added a mutex to protect the mesh and BLAS lists in the RayTracingFeatureProcessor Signed-off-by: dmcdiar --- .../Code/Source/RayTracing/RayTracingFeatureProcessor.cpp | 6 ++++++ .../Code/Source/RayTracing/RayTracingFeatureProcessor.h | 3 +++ 2 files changed, 9 insertions(+) diff --git a/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.cpp b/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.cpp index d10da818ee..a17ecaf1aa 100644 --- a/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.cpp @@ -89,6 +89,9 @@ namespace AZ RHI::Ptr device = RHI::RHISystemInterface::Get()->GetDevice(); uint32_t objectIndex = objectId.GetIndex(); + // lock the mutex to protect the mesh and BLAS lists + AZStd::unique_lock lock(m_mutex); + MeshMap::iterator itMesh = m_meshes.find(objectIndex); if (itMesh == m_meshes.end()) { @@ -169,6 +172,9 @@ namespace AZ return; } + // lock the mutex to protect the mesh and BLAS lists + AZStd::unique_lock lock(m_mutex); + MeshMap::iterator itMesh = m_meshes.find(objectId.GetIndex()); if (itMesh != m_meshes.end()) { diff --git a/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.h b/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.h index ee0a27b814..be75f0fac9 100644 --- a/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.h +++ b/Gems/Atom/Feature/Common/Code/Source/RayTracing/RayTracingFeatureProcessor.h @@ -223,6 +223,9 @@ namespace AZ // cached TransformServiceFeatureProcessor TransformServiceFeatureProcessor* m_transformServiceFeatureProcessor = nullptr; + // mutex for the mesh and BLAS lists + AZStd::mutex m_mutex; + // structure for data in the m_meshInfoBuffer, shaders that use the buffer must match this type struct MeshInfo { From b9d1af9f46cbd0c9ef97156b3bd71bafbdc1ee0e Mon Sep 17 00:00:00 2001 From: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> Date: Mon, 20 Sep 2021 15:44:07 -0500 Subject: [PATCH 3/3] Fixed calculation of each archive file data offset. (#4204) * Fixed calculation of each archive file data offset. The location of a file entry in a Zip file is calculated after the local file header + the length of the filename + the length of the extra field length per the ZIP file spec. https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> * Removed bIndependentBlocks variable Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> --- .../AzFramework/Archive/ZipDirCache.cpp | 2 +- .../Archive/ZipDirCacheFactory.cpp | 5 ++-- .../AzFramework/Archive/ZipDirStructures.cpp | 28 ++++++++----------- .../AzFramework/Archive/ZipDirStructures.h | 2 +- 4 files changed, 16 insertions(+), 21 deletions(-) diff --git a/Code/Framework/AzFramework/AzFramework/Archive/ZipDirCache.cpp b/Code/Framework/AzFramework/AzFramework/Archive/ZipDirCache.cpp index 328541c4d0..d17dbd0837 100644 --- a/Code/Framework/AzFramework/AzFramework/Archive/ZipDirCache.cpp +++ b/Code/Framework/AzFramework/AzFramework/Archive/ZipDirCache.cpp @@ -774,7 +774,7 @@ namespace AZ::IO::ZipDir return ZD_ERROR_INVALID_CALL; } - if (pFileEntry->nFileDataOffset != pFileEntry->INVALID_DATA_OFFSET) + if (pFileEntry->nFileDataOffset != FileEntryBase::INVALID_DATA_OFFSET) { return ZD_ERROR_SUCCESS; // the data offset has been successfully read.. } diff --git a/Code/Framework/AzFramework/AzFramework/Archive/ZipDirCacheFactory.cpp b/Code/Framework/AzFramework/AzFramework/Archive/ZipDirCacheFactory.cpp index 6adab594ed..5c5e93d441 100644 --- a/Code/Framework/AzFramework/AzFramework/Archive/ZipDirCacheFactory.cpp +++ b/Code/Framework/AzFramework/AzFramework/Archive/ZipDirCacheFactory.cpp @@ -553,7 +553,7 @@ namespace AZ::IO::ZipDir ////////////////////////////////////////////////////////////////////////// // give the CDR File Header entry, reads the local file header to validate - // and determine where the actual file lies + // and determine where the actual file resides void CacheFactory::AddFileEntry(char* strFilePath, const ZipFile::CDRFileHeader* pFileHeader, const SExtraZipFileData& extra) { if (pFileHeader->lLocalHeaderOffset > m_CDREnd.lCDROffset) @@ -600,8 +600,7 @@ namespace AZ::IO::ZipDir if (m_encryptedHeaders != ZipFile::HEADERS_NOT_ENCRYPTED) { // use CDR instead of local header - // The pak encryption tool asserts that there is no extra data at the end of the local file header, so don't add any extra data from the CDR header. - fileEntry.nFileDataOffset = pFileHeader->lLocalHeaderOffset + sizeof(ZipFile::LocalFileHeader) + pFileHeader->nFileNameLength; + fileEntry.nFileDataOffset = pFileHeader->lLocalHeaderOffset + sizeof(ZipFile::LocalFileHeader) + pFileHeader->nFileNameLength + pFileHeader->nExtraFieldLength; } else { diff --git a/Code/Framework/AzFramework/AzFramework/Archive/ZipDirStructures.cpp b/Code/Framework/AzFramework/AzFramework/Archive/ZipDirStructures.cpp index f9aa249339..cea517decd 100644 --- a/Code/Framework/AzFramework/AzFramework/Archive/ZipDirStructures.cpp +++ b/Code/Framework/AzFramework/AzFramework/Archive/ZipDirStructures.cpp @@ -187,8 +187,7 @@ namespace AZ::IO::ZipDir::ZipDirStructuresInternal // If src/dst overlap (in place decompress), then inflate in chunks, copying src locally to ensure // pointers don't foul each other. - bool bIndependantBlocks = ((pInput + nInputLen) <= pOutput) || (pInput >= (pOutput + nOutputLen)); - if (bIndependantBlocks) + if ((pInput + nInputLen) <= pOutput || pInput >= (pOutput + nOutputLen)) { pZStream->next_in = (Bytef*)pInput; pZStream->avail_in = aznumeric_cast(nInputLen); @@ -260,8 +259,7 @@ namespace AZ::IO::ZipDir::ZipDirStructuresInternal // If src/dst overlap (in place decompress), then inflate in chunks, copying src locally to ensure // pointers don't foul each other. - bool bIndependantBlocks = ((pIn + nIn) <= stream.next_out) || (pIn >= (stream.next_out + stream.avail_out)); - if (bIndependantBlocks) + if ((pIn + nIn) <= stream.next_out || pIn >= (stream.next_out + stream.avail_out)) { stream.next_in = pIn; stream.avail_in = nIn; @@ -498,18 +496,18 @@ namespace AZ::IO::ZipDir ////////////////////////////////////////////////////////////////////////// FileEntryBase::FileEntryBase(const ZipFile::CDRFileHeader& header, const SExtraZipFileData& extra) { - this->desc = header.desc; - this->nFileHeaderOffset = header.lLocalHeaderOffset; - //this->nFileDataOffset = INVALID_DATA_OFFSET; // we don't know yet - this->nMethod = header.nMethod; - this->nNameOffset = 0; // we don't know yet - this->nLastModTime = header.nLastModTime; - this->nLastModDate = header.nLastModDate; - this->nNTFS_LastModifyTime = extra.nLastModifyTime; + desc = header.desc; + nFileHeaderOffset = header.lLocalHeaderOffset; + + nMethod = header.nMethod; + nNameOffset = 0; // we don't know yet + nLastModTime = header.nLastModTime; + nLastModDate = header.nLastModDate; + nNTFS_LastModifyTime = extra.nLastModifyTime; // make an estimation (at least this offset should be there), but we don't actually know yet - this->nFileDataOffset = header.lLocalHeaderOffset + sizeof(ZipFile::LocalFileHeader) + header.nFileNameLength; - this->nEOFOffset = header.lLocalHeaderOffset + sizeof(ZipFile::LocalFileHeader) + header.nFileNameLength + header.desc.lSizeCompressed; + nFileDataOffset = header.lLocalHeaderOffset + sizeof(ZipFile::LocalFileHeader) + header.nFileNameLength + header.nExtraFieldLength; + nEOFOffset = nFileDataOffset + header.desc.lSizeCompressed; } // Uncompresses raw (without wrapping) data that is compressed with method 8 (deflated) in the Zip file @@ -817,8 +815,6 @@ namespace AZ::IO::ZipDir header.nFileNameLength = aznumeric_cast(nFileNameLength); header.nExtraFieldLength = 0; - pFileEntry->nFileDataOffset = pFileEntry->nFileHeaderOffset + sizeof(header) + header.nFileNameLength; - pFileEntry->nEOFOffset = pFileEntry->nFileDataOffset + pFileEntry->desc.lSizeCompressed; if (!AZ::IO::FileIOBase::GetDirectInstance()->Write(fileHandle, &header, sizeof(header))) { return ZD_ERROR_IO_FAILED; diff --git a/Code/Framework/AzFramework/AzFramework/Archive/ZipDirStructures.h b/Code/Framework/AzFramework/AzFramework/Archive/ZipDirStructures.h index 7e1d54b405..9295a7dd95 100644 --- a/Code/Framework/AzFramework/AzFramework/Archive/ZipDirStructures.h +++ b/Code/Framework/AzFramework/AzFramework/Archive/ZipDirStructures.h @@ -169,7 +169,7 @@ namespace AZ::IO::ZipDir inline static constexpr uint32_t INVALID_DATA_OFFSET = 0xFFFFFFFF; ZipFile::DataDescriptor desc{}; - uint32_t nFileDataOffset{}; // offset of the packed info inside the file; NOTE: this can be INVALID_DATA_OFFSET, if not calculated yet! + uint32_t nFileDataOffset{ INVALID_DATA_OFFSET }; // offset of the packed info inside the file; NOTE: this can be INVALID_DATA_OFFSET, if not calculated yet! uint32_t nFileHeaderOffset{ INVALID_DATA_OFFSET }; // offset of the local file header uint32_t nNameOffset{}; // offset of the file name in the name pool for the directory