Skip to content

Commit ac4e041

Browse files
AnastaZIukAnastaZIuk
committed
fix bugs with cached creation params & bindings validation, perform tests & make it work with current content, update examples_tests submodule - back to resolving the review comments
1 parent 114c079 commit ac4e041

File tree

3 files changed

+44
-45
lines changed

3 files changed

+44
-45
lines changed

‎examples_tests

‎include/nbl/ext/ImGui/ImGui.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ class UI final : public core::IReferenceCounted
7676
SBindingInfo texturesInfo, samplersInfo;
7777

7878
private:
79-
uint32_t texturesCount, samplersCount;
79+
uint32_t texturesCount = {}, samplersCount = {};
8080

8181
friend class UI;
8282
};
@@ -169,11 +169,11 @@ class UI final : public core::IReferenceCounted
169169
//! ImGUI context, you are supposed to cast it, eg. reinterpret_cast<ImGuiContext*>(this->getContext());
170170
void* getContext();
171171
private:
172-
void createPipeline();
173-
void createMDIBuffer();
172+
void createPipeline(SCreationParameters& creationParams);
173+
void createMDIBuffer(SCreationParameters& creationParams);
174174
void handleMouseEvents(const SUpdateParameters& params) const;
175175
void handleKeyEvents(const SUpdateParameters& params) const;
176-
video::ISemaphore::future_t<video::IQueue::RESULT> createFontAtlasTexture(video::IGPUCommandBuffer* cmdBuffer);
176+
video::ISemaphore::future_t<video::IQueue::RESULT> createFontAtlasTexture(video::IGPUCommandBuffer* cmdBuffer, SCreationParameters& creationParams);
177177

178178
SCachedCreationParams m_cachedCreationParams;
179179

‎src/nbl/ext/ImGui/ImGui.cpp

Lines changed: 39 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -114,10 +114,8 @@ namespace nbl::ext::imgui
114114
return utilities->getLogicalDevice()->createPipelineLayout(PushConstantRanges, std::move(layouts[0u]), std::move(layouts[1u]), std::move(layouts[2u]), std::move(layouts[3u]));
115115
}
116116

117-
void UI::createPipeline()
117+
void UI::createPipeline(SCreationParameters& creationParams)
118118
{
119-
auto& creationParams = reinterpret_cast<SCreationParameters&>(m_cachedCreationParams);
120-
121119
auto pipelineLayout = smart_refctd_ptr<IGPUPipelineLayout>(creationParams.pipelineLayout);
122120

123121
if (!pipelineLayout)
@@ -307,10 +305,8 @@ namespace nbl::ext::imgui
307305
}
308306
}
309307

310-
ISemaphore::future_t<IQueue::RESULT> UI::createFontAtlasTexture(video::IGPUCommandBuffer* cmdBuffer)
308+
ISemaphore::future_t<IQueue::RESULT> UI::createFontAtlasTexture(video::IGPUCommandBuffer* cmdBuffer, SCreationParameters& creationParams)
311309
{
312-
auto& creationParams = reinterpret_cast<SCreationParameters&>(m_cachedCreationParams);
313-
314310
// Load Fonts
315311
// - If no fonts are loaded, dear imgui will use the default font. You can also load multiple fonts and use ImGui::PushFont()/PopFont() to select them.
316312
// - AddFontFromFileTTF() will return the ImFont* so you can store it if you need to select the font among multiple.
@@ -379,29 +375,29 @@ namespace nbl::ext::imgui
379375
region->imageExtent = { params.extent.width, params.extent.height, 1u };
380376
}
381377

382-
auto image = m_cachedCreationParams.utilities->getLogicalDevice()->createImage(std::move(params));
378+
auto image = creationParams.utilities->getLogicalDevice()->createImage(std::move(params));
383379

384380
if (!image)
385381
{
386-
m_cachedCreationParams.utilities->getLogger()->log("Could not create font image!", system::ILogger::ELL_ERROR);
382+
creationParams.utilities->getLogger()->log("Could not create font image!", system::ILogger::ELL_ERROR);
387383
return IQueue::RESULT::OTHER_ERROR;
388384
}
389385
image->setObjectDebugName("Nabla ImGUI default font");
390386

391-
if (!m_cachedCreationParams.utilities->getLogicalDevice()->allocate(image->getMemoryReqs(), image.get()).isValid())
387+
if (!creationParams.utilities->getLogicalDevice()->allocate(image->getMemoryReqs(), image.get()).isValid())
392388
{
393-
m_cachedCreationParams.utilities->getLogger()->log("Could not allocate memory for font image!", system::ILogger::ELL_ERROR);
389+
creationParams.utilities->getLogger()->log("Could not allocate memory for font image!", system::ILogger::ELL_ERROR);
394390
return IQueue::RESULT::OTHER_ERROR;
395391
}
396392

397393
SIntendedSubmitInfo sInfo;
398394
{
399395
IQueue::SSubmitInfo::SCommandBufferInfo cmdInfo = { cmdBuffer };
400396

401-
auto scratchSemaphore = m_cachedCreationParams.utilities->getLogicalDevice()->createSemaphore(0);
397+
auto scratchSemaphore = creationParams.utilities->getLogicalDevice()->createSemaphore(0);
402398
if (!scratchSemaphore)
403399
{
404-
m_cachedCreationParams.utilities->getLogger()->log("Could not create scratch semaphore", system::ILogger::ELL_ERROR);
400+
creationParams.utilities->getLogger()->log("Could not create scratch semaphore", system::ILogger::ELL_ERROR);
405401
return IQueue::RESULT::OTHER_ERROR;
406402
}
407403
scratchSemaphore->setObjectDebugName("Nabla IMGUI extension Scratch Semaphore");
@@ -438,9 +434,9 @@ namespace nbl::ext::imgui
438434
cmdBuffer->pipelineBarrier(E_DEPENDENCY_FLAGS::EDF_NONE,{.imgBarriers=barriers});
439435
// We cannot use the `AutoSubmit` variant of the util because we need to add a pipeline barrier with a transition onto the command buffer after the upload.
440436
// old layout is UNDEFINED because we don't want a content preserving transition, we can just put ourselves in transfer right away
441-
if (!m_cachedCreationParams.utilities->updateImageViaStagingBuffer(sInfo,pixels,image->getCreationParameters().format,image.get(),transferLayout,regions.range))
437+
if (!creationParams.utilities->updateImageViaStagingBuffer(sInfo,pixels,image->getCreationParameters().format,image.get(),transferLayout,regions.range))
442438
{
443-
m_cachedCreationParams.utilities->getLogger()->log("Could not upload font image contents", system::ILogger::ELL_ERROR);
439+
creationParams.utilities->getLogger()->log("Could not upload font image contents", system::ILogger::ELL_ERROR);
444440
return IQueue::RESULT::OTHER_ERROR;
445441
}
446442

@@ -455,7 +451,7 @@ namespace nbl::ext::imgui
455451
const auto submit = sInfo.popSubmit({});
456452
if (creationParams.transfer->submit(submit)!=IQueue::RESULT::SUCCESS)
457453
{
458-
m_cachedCreationParams.utilities->getLogger()->log("Could not submit workload for font texture upload.", system::ILogger::ELL_ERROR);
454+
creationParams.utilities->getLogger()->log("Could not submit workload for font texture upload.", system::ILogger::ELL_ERROR);
459455
return IQueue::RESULT::OTHER_ERROR;
460456
}
461457
}
@@ -467,7 +463,7 @@ namespace nbl::ext::imgui
467463
params.subresourceRange = regions.subresource;
468464
params.image = core::smart_refctd_ptr(image);
469465

470-
m_fontAtlasTexture = m_cachedCreationParams.utilities->getLogicalDevice()->createImageView(std::move(params));
466+
m_fontAtlasTexture = creationParams.utilities->getLogicalDevice()->createImageView(std::move(params));
471467
}
472468

473469
ISemaphore::future_t<IQueue::RESULT> retval(IQueue::RESULT::SUCCESS);
@@ -683,11 +679,8 @@ namespace nbl::ext::imgui
683679
}
684680
}
685681

686-
UI::UI(SCreationParameters&& params)
687-
: m_cachedCreationParams(std::move(params))
682+
UI::UI(SCreationParameters&& creationParams)
688683
{
689-
auto& creationParams = reinterpret_cast<SCreationParameters&>(m_cachedCreationParams);
690-
691684
auto validateResourcesInfo = [&]() -> bool
692685
{
693686
auto* pipelineLayout = creationParams.pipelineLayout.get();
@@ -700,31 +693,35 @@ namespace nbl::ext::imgui
700693
ixLiteral = descriptorType == IDescriptor::E_TYPE::ET_SAMPLED_IMAGE ? "texturesBindingIx" : "samplersBindingIx";
701694

702695
// we need to check if there is at least single "descriptorType" resource, if so we can validate the resource further
703-
auto anyBindingCount = [&m_cachedCreationParams = m_cachedCreationParams, &log = std::as_const(typeLiteral)](const IDescriptorSetLayoutBase::CBindingRedirect* redirect) -> bool
696+
auto anyBindingCount = [&creationParams = creationParams, &log = std::as_const(typeLiteral)](const IDescriptorSetLayoutBase::CBindingRedirect* redirect, bool logError = true) -> bool
704697
{
705-
if (redirect->getBindingCount())
698+
bool ok = redirect->getBindingCount();
699+
700+
if (!ok && logError)
706701
{
707-
m_cachedCreationParams.utilities->getLogger()->log("Provided descriptor set layout has no bindings for IDescriptor::E_TYPE::%s, you are required to provide at least single default ImGUI Font Atlas texture resource & corresponsing sampler resource!", system::ILogger::ELL_ERROR, log.data());
702+
creationParams.utilities->getLogger()->log("Provided descriptor set layout has no bindings for IDescriptor::E_TYPE::%s, you are required to provide at least single default ImGUI Font Atlas texture resource & corresponsing sampler resource!", system::ILogger::ELL_ERROR, log.data());
708703
return false;
709704
}
710705

711-
return true;
706+
return ok;
712707
};
713708

714709
if(!descriptorSetLayout)
715710
{
716-
m_cachedCreationParams.utilities->getLogger()->log("Provided descriptor set layout for IDescriptor::E_TYPE::%s is nullptr!", system::ILogger::ELL_ERROR, typeLiteral.data());
711+
creationParams.utilities->getLogger()->log("Provided descriptor set layout for IDescriptor::E_TYPE::%s is nullptr!", system::ILogger::ELL_ERROR, typeLiteral.data());
717712
return false;
718713
}
719714

720715
const auto* redirect = &descriptorSetLayout->getDescriptorRedirect(descriptorType);
721716

722717
if constexpr (descriptorType == IDescriptor::E_TYPE::ET_SAMPLED_IMAGE)
718+
{
723719
if (!anyBindingCount(redirect))
724720
return false;
721+
}
725722
else
726723
{
727-
if (!anyBindingCount(redirect))
724+
if (!anyBindingCount(redirect, false))
728725
{
729726
redirect = &descriptorSetLayout->getImmutableSamplerRedirect(); // we must give it another try & request to look for immutable samplers
730727

@@ -740,36 +737,36 @@ namespace nbl::ext::imgui
740737
{
741738
const auto rangeStorageIndex = IDescriptorSetLayoutBase::CBindingRedirect::storage_range_index_t(i);
742739
const auto binding = redirect->getBinding(rangeStorageIndex);
743-
const auto requestedBindingIx = descriptorType == IDescriptor::E_TYPE::ET_SAMPLED_IMAGE ? m_cachedCreationParams.resources.texturesInfo.bindingIx : m_cachedCreationParams.resources.samplersInfo.bindingIx;
740+
const auto requestedBindingIx = descriptorType == IDescriptor::E_TYPE::ET_SAMPLED_IMAGE ? creationParams.resources.texturesInfo.bindingIx : creationParams.resources.samplersInfo.bindingIx;
744741

745742
if (binding.data == requestedBindingIx)
746743
{
747744
const auto count = redirect->getCount(binding);
748745

749746
if(!count)
750747
{
751-
m_cachedCreationParams.utilities->getLogger()->log("Provided descriptor set layout has IDescriptor::E_TYPE::%s binding for requested `m_cachedCreationParams.resources.%s` index but the binding resource count == 0u!", system::ILogger::ELL_ERROR, typeLiteral.data(), ixLiteral.data());
748+
creationParams.utilities->getLogger()->log("Provided descriptor set layout has IDescriptor::E_TYPE::%s binding for requested `creationParams.resources.%s` index but the binding resource count == 0u!", system::ILogger::ELL_ERROR, typeLiteral.data(), ixLiteral.data());
752749
return false;
753750
}
754751

755752
if constexpr (descriptorType == IDescriptor::E_TYPE::ET_SAMPLED_IMAGE)
756-
m_cachedCreationParams.resources.texturesCount = count;
753+
creationParams.resources.texturesCount = count;
757754
else
758-
m_cachedCreationParams.resources.samplersCount = count;
755+
creationParams.resources.samplersCount = count;
759756

760757
const auto stage = redirect->getStageFlags(binding);
761758

762-
if(!stage.hasFlags(m_cachedCreationParams.resources.RequiredShaderStageFlags))
759+
if(!stage.hasFlags(creationParams.resources.RequiredShaderStageFlags))
763760
{
764-
m_cachedCreationParams.utilities->getLogger()->log("Provided descriptor set layout has IDescriptor::E_TYPE::%s binding for requested `m_cachedCreationParams.resources.%s` index but doesn't meet stage flags requirements!", system::ILogger::ELL_ERROR, typeLiteral.data(), ixLiteral.data());
761+
creationParams.utilities->getLogger()->log("Provided descriptor set layout has IDescriptor::E_TYPE::%s binding for requested `creationParams.resources.%s` index but doesn't meet stage flags requirements!", system::ILogger::ELL_ERROR, typeLiteral.data(), ixLiteral.data());
765762
return false;
766763
}
767764

768765
const auto creation = redirect->getCreateFlags(rangeStorageIndex);
769766

770-
if (!creation.hasFlags(descriptorType == IDescriptor::E_TYPE::ET_SAMPLED_IMAGE ? m_cachedCreationParams.resources.TexturesRequiredCreateFlags : m_cachedCreationParams.resources.SamplersRequiredCreateFlags))
767+
if (!creation.hasFlags(descriptorType == IDescriptor::E_TYPE::ET_SAMPLED_IMAGE ? creationParams.resources.TexturesRequiredCreateFlags : creationParams.resources.SamplersRequiredCreateFlags))
771768
{
772-
m_cachedCreationParams.utilities->getLogger()->log("Provided descriptor set layout has IDescriptor::E_TYPE::%s binding for requested `m_cachedCreationParams.resources.%s` index but doesn't meet create flags requirements!", system::ILogger::ELL_ERROR, typeLiteral.data(), ixLiteral.data());
769+
creationParams.utilities->getLogger()->log("Provided descriptor set layout has IDescriptor::E_TYPE::%s binding for requested `creationParams.resources.%s` index but doesn't meet create flags requirements!", system::ILogger::ELL_ERROR, typeLiteral.data(), ixLiteral.data());
773770
return false;
774771
}
775772

@@ -780,15 +777,15 @@ namespace nbl::ext::imgui
780777

781778
if (!ok)
782779
{
783-
m_cachedCreationParams.utilities->getLogger()->log("Provided descriptor set layout has no IDescriptor::E_TYPE::%s binding for requested `m_cachedCreationParams.resources.%s` index or it is invalid!", system::ILogger::ELL_ERROR, typeLiteral.data(), ixLiteral.data());
780+
creationParams.utilities->getLogger()->log("Provided descriptor set layout has no IDescriptor::E_TYPE::%s binding for requested `creationParams.resources.%s` index or it is invalid!", system::ILogger::ELL_ERROR, typeLiteral.data(), ixLiteral.data());
784781
return false;
785782
}
786783

787784
return true;
788785
};
789786

790787
const auto& layouts = pipelineLayout->getDescriptorSetLayouts();
791-
const bool ok = validateResource.template operator() < IDescriptor::E_TYPE::ET_SAMPLED_IMAGE > (layouts[m_cachedCreationParams.resources.texturesInfo.setIx]) && validateResource.template operator() < IDescriptor::E_TYPE::ET_SAMPLER > (layouts[m_cachedCreationParams.resources.samplersInfo.setIx]);
788+
const bool ok = validateResource.template operator() < IDescriptor::E_TYPE::ET_SAMPLED_IMAGE > (layouts[creationParams.resources.texturesInfo.setIx]) && validateResource.template operator() < IDescriptor::E_TYPE::ET_SAMPLER > (layouts[creationParams.resources.samplersInfo.setIx]);
792789

793790
if (!ok)
794791
return false;
@@ -840,17 +837,19 @@ namespace nbl::ext::imgui
840837
IMGUI_CHECKVERSION();
841838
ImGui::CreateContext();
842839

843-
createPipeline();
844-
createMDIBuffer();
845-
createFontAtlasTexture(transistentCMD.get());
840+
createPipeline(creationParams);
841+
createMDIBuffer(creationParams);
842+
createFontAtlasTexture(transistentCMD.get(), creationParams);
846843

847844
auto & io = ImGui::GetIO();
848845
io.BackendUsingLegacyKeyArrays = 0; // using AddKeyEvent() - it's new way of handling ImGUI events our backends supports
846+
847+
m_cachedCreationParams = std::move(creationParams);
849848
}
850849

851850
UI::~UI() = default;
852851

853-
void UI::createMDIBuffer()
852+
void UI::createMDIBuffer(SCreationParameters& m_cachedCreationParams)
854853
{
855854
constexpr static uint32_t minStreamingBufferAllocationSize = 32u, maxStreamingBufferAllocationAlignment = 1024u * 64u, mdiBufferDefaultSize = /* 2MB */ 1024u * 1024u * 2u;
856855

0 commit comments

Comments
 (0)