Skip to content

Commit 8bb4a2c

Browse files
AnastaZIukAnastaZIuk
committed
Fix messy PLY SVertexInputParams caching bug, change it's metadata, make it work
1 parent 86b0088 commit 8bb4a2c

File tree

5 files changed

+34
-41
lines changed

5 files changed

+34
-41
lines changed

‎examples_tests/27.LoaderFixes/main.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ using namespace core;
1717
Define below to write assets
1818
*/
1919

20-
//#define WRITE_ASSETS
20+
// #define WRITE_ASSETS
2121

2222
int main()
2323
{

‎include/nbl/asset/metadata/CPLYMetadata.h

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,31 +18,22 @@ class CPLYMetadata final : public IAssetMetadata
1818
class CRenderpassIndependentPipeline : public IRenderpassIndependentPipelineMetadata
1919
{
2020
public:
21-
CRenderpassIndependentPipeline() : IRenderpassIndependentPipelineMetadata(), m_hash(0xdeadbeefu) {}
22-
CRenderpassIndependentPipeline(CRenderpassIndependentPipeline&& other)
21+
CRenderpassIndependentPipeline(CRenderpassIndependentPipeline&& _other) : CRenderpassIndependentPipeline()
2322
{
24-
operator=(std::move(other));
23+
CRenderpassIndependentPipeline::operator=(std::move(_other));
2524
}
25+
template<typename... Args>
26+
CRenderpassIndependentPipeline(Args&&... args) : IRenderpassIndependentPipelineMetadata(std::forward<Args>(args)...) {}
2627

27-
CRenderpassIndependentPipeline& operator=(const CRenderpassIndependentPipeline&) = delete;
2828
inline CRenderpassIndependentPipeline& operator=(CRenderpassIndependentPipeline&& other)
2929
{
3030
IRenderpassIndependentPipelineMetadata::operator=(std::move(other));
31-
std::swap(m_hash,other.m_hash);
3231
return *this;
3332
}
34-
35-
uint32_t m_hash;
3633
};
37-
38-
template<class InContainer>
39-
CPLYMetadata(InContainer&& hashContainer, core::smart_refctd_dynamic_array<IRenderpassIndependentPipelineMetadata::ShaderInputSemantic>&& _semanticStorage) :
40-
IAssetMetadata(), m_metaStorage(createContainer<CRenderpassIndependentPipeline>(hashContainer.size())), m_semanticStorage(std::move(_semanticStorage))
41-
{
42-
auto metaIt = m_metaStorage->begin();
43-
for (auto hash : hashContainer)
44-
(metaIt++)->m_hash = hash;
45-
}
34+
35+
CPLYMetadata(uint32_t pplnCount, core::smart_refctd_dynamic_array<IRenderpassIndependentPipelineMetadata::ShaderInputSemantic>&& _semanticStorage) :
36+
IAssetMetadata(), m_metaStorage(createContainer<CRenderpassIndependentPipeline>(pplnCount)), m_semanticStorage(std::move(_semanticStorage)) {}
4637

4738
_NBL_STATIC_INLINE_CONSTEXPR const char* LoaderName = "CPLYMeshFileLoader";
4839
const char* getLoaderName() const override { return LoaderName; }

‎src/nbl/asset/interchange/CPLYMeshFileLoader.cpp

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ CPLYMeshFileLoader::CPLYMeshFileLoader(IAssetManager* _am)
2626
: IRenderpassIndependentPipelineLoader(_am)
2727
{
2828
initialize();
29-
IRenderpassIndependentPipelineLoader::initialize();
3029
}
3130

3231
CPLYMeshFileLoader::~CPLYMeshFileLoader() {}
@@ -64,6 +63,8 @@ bool CPLYMeshFileLoader::isALoadableFileFormat(io::IReadFile* _file) const
6463

6564
void CPLYMeshFileLoader::initialize()
6665
{
66+
IRenderpassIndependentPipelineLoader::initialize();
67+
6768
auto precomputeAndCachePipeline = [&](CPLYMeshFileLoader::E_TYPE type, bool indexBufferBindingAvailable)
6869
{
6970
constexpr std::array<std::pair<uint8_t, std::pair<std::string_view, std::string_view>>, 3> avaiableOptionsForShaders
@@ -77,6 +78,7 @@ void CPLYMeshFileLoader::initialize()
7778
{
7879
switch (type)
7980
{
81+
case ET_POS:
8082
case ET_COL:
8183
return std::make_pair("nbl/builtin/material/debug/vertex_color/specialized_shader.vert", "nbl/builtin/material/debug/vertex_color/specialized_shader.frag");
8284
case ET_UV:
@@ -122,7 +124,10 @@ void CPLYMeshFileLoader::initialize()
122124

123125
SVertexInputParams inputParams;
124126

125-
std::vector<uint8_t> availableAttributes = { ET_POS, static_cast<uint8_t>(type) };
127+
std::vector<uint8_t> availableAttributes = { ET_POS };
128+
if (type != ET_POS)
129+
availableAttributes.push_back(static_cast<uint8_t>(type));
130+
126131
for (auto& attrib : availableAttributes)
127132
{
128133
const auto currentBitmask = core::createBitmask({ attrib });
@@ -158,10 +163,12 @@ void CPLYMeshFileLoader::initialize()
158163
Pipeline permutations are cached
159164
*/
160165

166+
precomputeAndCachePipeline(ET_POS, false);
161167
precomputeAndCachePipeline(ET_COL, false);
162168
precomputeAndCachePipeline(ET_UV, false);
163169
precomputeAndCachePipeline(ET_NORM, false);
164170

171+
precomputeAndCachePipeline(ET_POS, true);
165172
precomputeAndCachePipeline(ET_COL, true);
166173
precomputeAndCachePipeline(ET_UV, true);
167174
precomputeAndCachePipeline(ET_NORM, true);
@@ -407,14 +414,10 @@ asset::SAssetBundle CPLYMeshFileLoader::loadAsset(io::IReadFile* _file, const as
407414
}
408415
}
409416

410-
constexpr uint32_t WHAT_IS_THE_HASH_SUPPOSED_TO_BE = 69u; // TODO: @Crisspl / @Anastazluk figure it out!
411-
412-
auto meta = core::make_smart_refctd_ptr<CPLYMetadata>(std::move(ctx.hashes4pplns),core::smart_refctd_ptr(m_basicViewParamsSemantics));
413-
{
414-
uint32_t offset = 0u;
415-
for (auto meshbuffer : mesh->getMeshBuffers())
416-
meta->placeMeta(offset++,meshbuffer->getPipeline());
417-
}
417+
auto* mbPipeline = mesh->getMeshBuffers().begin()[0]->getPipeline();
418+
auto meta = core::make_smart_refctd_ptr<CPLYMetadata>(1u, std::move(m_basicViewParamsSemantics));
419+
meta->placeMeta(0u, mbPipeline);
420+
418421
return SAssetBundle(std::move(meta),{ std::move(mesh) });
419422
}
420423

@@ -743,10 +746,7 @@ bool CPLYMeshFileLoader::genVertBuffersForMBuffer(
743746
}
744747

745748
if(!mbPipeline)
746-
mbPipeline = fetchPipelineFromCache(ET_COL);
747-
748-
auto meta = core::make_smart_refctd_ptr<CPLYMetadata>(1u, std::move(m_basicViewParamsSemantics));
749-
meta->placeMeta(0u, mbPipeline.get());
749+
mbPipeline = fetchPipelineFromCache(ET_POS);
750750
}
751751

752752
return mbPipeline;
@@ -766,9 +766,6 @@ bool CPLYMeshFileLoader::genVertBuffersForMBuffer(
766766
_mbuf->setVertexBufferBinding({ 0ul, std::move(buffer) }, index);
767767
}
768768
}
769-
770-
constexpr uint32_t hash = 1u; // TODO: @Crisspl why is it always 1?
771-
context.hashes4pplns.emplace_back(hash); // TODO
772769

773770
_mbuf->setPipeline(std::move(mbPipeline));
774771

‎src/nbl/asset/interchange/CPLYMeshFileLoader.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,14 +58,16 @@ class CPLYMeshFileLoader : public IAssetLoader, public IRenderpassIndependentPip
5858

5959
void initialize();
6060

61-
const std::string getPipelineCacheKey(E_TYPE type, bool indexBufferBindingAvailable)
61+
static const std::string getPipelineCacheKey(E_TYPE type, bool indexBufferBindingAvailable)
6262
{
6363
auto getTypeHash = [&]() -> std::string
6464
{
6565
bool status = true;
6666

6767
switch (type)
6868
{
69+
case ET_POS:
70+
return "nbl/builtin/pipeline/loader/PLY/only_position_attribute/";
6971
case ET_COL:
7072
return "nbl/builtin/pipeline/loader/PLY/color_attribute/";
7173
case ET_UV:
@@ -172,8 +174,6 @@ class CPLYMeshFileLoader : public IAssetLoader, public IRenderpassIndependentPip
172174
IAssetLoader::SAssetLoadContext inner;
173175
uint32_t topHierarchyLevel;
174176
IAssetLoader::IAssetLoaderOverride* loaderOverride;
175-
176-
core::vector<uint32_t> hashes4pplns;
177177

178178
core::vector<std::unique_ptr<SPLYElement>> ElementList;
179179

‎src/nbl/asset/interchange/CSTLMeshFileLoader.cpp

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,12 @@ CSTLMeshFileLoader::CSTLMeshFileLoader(asset::IAssetManager* _m_assetMgr)
2828
: IRenderpassIndependentPipelineLoader(_m_assetMgr), m_assetMgr(_m_assetMgr)
2929
{
3030
initialize();
31-
IRenderpassIndependentPipelineLoader::initialize();
3231
}
3332

3433
void CSTLMeshFileLoader::initialize()
3534
{
35+
IRenderpassIndependentPipelineLoader::initialize();
36+
3637
auto precomputeAndCachePipeline = [&](bool withColorAttribute)
3738
{
3839
auto getShaderDefaultPaths = [&]() -> std::pair<std::string_view, std::string_view>
@@ -102,7 +103,7 @@ void CSTLMeshFileLoader::initialize()
102103

103104
SRasterizationParams rastarizationParmas;
104105

105-
auto mbPipeline = core::make_smart_refctd_ptr<ICPURenderpassIndependentPipeline>(std::move(mbPipelineLayout), nullptr, nullptr, mbInputParams, blendParams, primitiveAssemblyParams, rastarizationParmas); // TODO: @Crisspl/@Anastazluk pipeline should also be builtin because no need to customize the metadata anymore
106+
auto mbPipeline = core::make_smart_refctd_ptr<ICPURenderpassIndependentPipeline>(std::move(mbPipelineLayout), nullptr, nullptr, mbInputParams, blendParams, primitiveAssemblyParams, rastarizationParmas);
106107
{
107108
mbPipeline->setShaderAtIndex(ICPURenderpassIndependentPipeline::ESSI_VERTEX_SHADER_IX, mbVertexShader.get());
108109
mbPipeline->setShaderAtIndex(ICPURenderpassIndependentPipeline::ESSI_FRAGMENT_SHADER_IX, mbFragmentShader.get());
@@ -253,14 +254,18 @@ SAssetBundle CSTLMeshFileLoader::loadAsset(IReadFile* _file, const IAssetLoader:
253254
const size_t vtxSize = hasColor ? (3 * sizeof(float) + 4 + 4) : (3 * sizeof(float) + 4);
254255
auto vertexBuf = core::make_smart_refctd_ptr<asset::ICPUBuffer>(vtxSize * positions.size());
255256

256-
uint32_t normal{};
257+
using quant_normal_t = CQuantNormalCache::value_type_t<EF_A2B10G10R10_SNORM_PACK32>;
258+
259+
quant_normal_t normal;
257260
for (size_t i = 0u; i < positions.size(); ++i)
258261
{
259262
if (i % 3 == 0)
260263
normal = quantNormalCache->quantize<EF_A2B10G10R10_SNORM_PACK32>(normals[i / 3]);
261264
uint8_t* ptr = ((uint8_t*)(vertexBuf->getPointer())) + i * vtxSize;
262265
memcpy(ptr, positions[i].pointer, 3 * 4);
263-
((uint32_t*)(ptr + 12))[0] = normal;
266+
267+
((quant_normal_t*)((uint32_t*)(ptr + 12)))[0] = normal;
268+
264269
if (hasColor)
265270
memcpy(ptr + 16, colors.data() + i / 3, 4);
266271
}

0 commit comments

Comments
 (0)