Skip to content

Commit bad473b

Browse files
FletterioFletterio
committed
Refactor following PR review done!
1 parent f6c07d6 commit bad473b

File tree

10 files changed

+74
-46
lines changed

10 files changed

+74
-46
lines changed

‎include/nbl/asset/ICPUDescriptorSet.h

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,10 +99,6 @@ class NBL_API2 ICPUDescriptorSet final : public IDescriptorSet<ICPUDescriptorSet
9999
virtual ~ICPUDescriptorSet() = default;
100100

101101
private:
102-
static inline IDescriptor::E_CATEGORY getCategoryFromType(const IDescriptor::E_TYPE type)
103-
{
104-
return IDescriptor::GetTypeCategory(type);
105-
}
106102

107103
core::smart_refctd_dynamic_array<ICPUDescriptorSet::SDescriptorInfo> m_descriptorInfos[static_cast<uint32_t>(IDescriptor::E_TYPE::ET_COUNT)];
108104
};

‎include/nbl/asset/IDescriptorSet.h

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -95,18 +95,23 @@ class IDescriptorSet : public virtual core::IReferenceCounted // TODO: try to re
9595
}
9696
~SDescriptorInfo()
9797
{
98+
if (desc && desc->getTypeCategory() == IDescriptor::EC_IMAGE)
99+
info.combinedImageSampler.sampler = nullptr;
98100
}
99101

100102
inline SDescriptorInfo& operator=(const SDescriptorInfo& other)
101103
{
102104
if (desc and desc->getTypeCategory()==IDescriptor::EC_IMAGE)
103105
info.combinedImageSampler.sampler = nullptr;
104106
desc = other.desc;
105-
const auto type = desc->getTypeCategory();
106-
if (type!=IDescriptor::EC_IMAGE)
107-
info.buffer = other.info.buffer;
108-
else
109-
info.combinedImageSampler = other.info.combinedImageSampler;
107+
if (desc)
108+
{
109+
const auto type = desc->getTypeCategory();
110+
if (type != IDescriptor::EC_IMAGE)
111+
info.buffer = other.info.buffer;
112+
else
113+
info.combinedImageSampler = other.info.combinedImageSampler;
114+
}
110115
return *this;
111116
}
112117
inline SDescriptorInfo& operator=(SDescriptorInfo&& other)

‎include/nbl/asset/IDescriptorSetLayout.h

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,9 @@ class IDescriptorSetLayout : public virtual core::IReferenceCounted // TODO: tr
5757
core::bitflag<E_CREATE_FLAGS> createFlags;
5858
core::bitflag<IShader::E_SHADER_STAGE> stageFlags;
5959
uint32_t count;
60-
// Use this if you want an immutable sampler that is baked into the DS layout itself.
61-
// If its `nullptr` then the sampler used is mutable and can be specified while writing the image descriptor to a binding while updating the DS.
62-
const core::smart_refctd_ptr<sampler_type>* samplers;
60+
// Use this if you want immutable samplers that are baked into the DS layout itself.
61+
// If it's `nullptr` then the samplers used are mutable and can be specified while writing the image descriptor to a binding while updating the DS.
62+
const core::smart_refctd_ptr<sampler_type>* immutableSamplers;
6363
};
6464

6565
// Maps a binding to a local (to descriptor set layout) offset.
@@ -412,13 +412,13 @@ class IDescriptorSetLayout : public virtual core::IReferenceCounted // TODO: tr
412412
{
413413
switch (b.type) {
414414
case IDescriptor::E_TYPE::ET_SAMPLER:
415-
if (b.samplers)
415+
if (b.immutableSamplers)
416416
buildInfo_immutableSamplers.emplace_back(b.binding, b.createFlags, b.stageFlags, b.count);
417417
else
418418
buildInfo_descriptors[size_t(IDescriptor::E_TYPE::ET_SAMPLER)].emplace_back(b.binding, b.createFlags, b.stageFlags, b.count);
419419
break;
420420
case IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER:
421-
if (b.samplers)
421+
if (b.immutableSamplers)
422422
buildInfo_immutableSamplers.emplace_back(b.binding, b.createFlags, b.stageFlags, b.count);
423423
else
424424
buildInfo_mutableSamplers.emplace_back(b.binding, b.createFlags, b.stageFlags, b.count);
@@ -440,13 +440,13 @@ class IDescriptorSetLayout : public virtual core::IReferenceCounted // TODO: tr
440440

441441
for (const auto& b : _bindings)
442442
{
443-
if ((b.type == IDescriptor::E_TYPE::ET_SAMPLER or b.type == IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER) and b.samplers)
443+
if ((b.type == IDescriptor::E_TYPE::ET_SAMPLER or b.type == IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER) and b.immutableSamplers)
444444
{
445445
const auto localOffset = m_immutableSamplerRedirect.getStorageOffset(typename CBindingRedirect::binding_number_t(b.binding)).data;
446446
assert(localOffset != m_immutableSamplerRedirect.Invalid);
447447

448448
auto* dst = m_immutableSamplers->begin() + localOffset;
449-
std::copy_n(b.samplers, b.count, dst);
449+
std::copy_n(b.immutableSamplers, b.count, dst);
450450
}
451451
}
452452
}

‎include/nbl/asset/utils/IVirtualTexture.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1049,7 +1049,7 @@ class IVirtualTexture : public core::IReferenceCounted, public IVirtualTextureBa
10491049
bnd.count = _count;
10501050
bnd.stageFlags = asset::IShader::ESS_ALL;
10511051
bnd.type = asset::IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER;
1052-
bnd.samplers = _samplers;
1052+
bnd.immutableSamplers = _samplers;
10531053
};
10541054

10551055
fillBinding(bindings[0], _pgtBinding, 1u, samplers);

‎include/nbl/scene/ILevelOfDetailLibrary.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ class ILevelOfDetailLibrary : public virtual core::IReferenceCounted
213213
bindings[i].type = asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER;
214214
bindings[i].count = 1u;
215215
bindings[i].stageFlags = asset::IShader::ESS_COMPUTE;
216-
bindings[i].samplers = nullptr;
216+
bindings[i].immutableSamplers = nullptr;
217217
}
218218
return device->createDescriptorSetLayout(bindings);
219219
}

‎src/nbl/asset/IAssetManager.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ void IAssetManager::insertBuiltinAssets()
245245
binding3.type = IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER;
246246
binding3.count = 1u;
247247
binding3.stageFlags = static_cast<asset::ICPUShader::E_SHADER_STAGE>(asset::ICPUShader::ESS_FRAGMENT);
248-
binding3.samplers = nullptr;
248+
binding3.immutableSamplers = nullptr;
249249

250250
auto ds3Layout = core::make_smart_refctd_ptr<asset::ICPUDescriptorSetLayout>(&binding3, &binding3 + 1);
251251
addBuiltInToCaches(ds3Layout, "nbl/builtin/material/lambertian/singletexture/descriptor_set_layout/3"); // TODO find everything what has been using it so far

‎src/nbl/asset/ICPUDescriptorSet.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ core::smart_refctd_ptr<IAsset> ICPUDescriptorSet::clone(uint32_t _depth) const
5757
const auto& srcDescriptorInfo = m_descriptorInfos[t]->begin()[i];
5858
auto& dstDescriptorInfo = cp->m_descriptorInfos[t]->begin()[i];
5959

60-
auto category = getCategoryFromType(type);
60+
auto category = IDescriptor::GetTypeCategory(type);
6161

6262
if (category == IDescriptor::E_CATEGORY::EC_IMAGE)
6363
dstDescriptorInfo.info.combinedImageSampler = srcDescriptorInfo.info.combinedImageSampler;
@@ -127,7 +127,7 @@ void ICPUDescriptorSet::convertToDummyObject(uint32_t referenceLevelsBelowToConv
127127
auto descriptorInfos = m_descriptorInfos[t]->begin();
128128
assert(descriptorInfos);
129129

130-
const auto category = getCategoryFromType(type);
130+
const auto category = IDescriptor::GetTypeCategory(type);
131131
for (uint32_t i = 0u; i < descriptorCount; ++i)
132132
{
133133
switch (category)
@@ -183,7 +183,7 @@ void ICPUDescriptorSet::restoreFromDummy_impl(IAsset* _other, uint32_t _levelsBe
183183

184184
auto otherDescriptorInfos = other->m_descriptorInfos[t]->begin();
185185

186-
const auto category = getCategoryFromType(type);
186+
const auto category = IDescriptor::GetTypeCategory(type);
187187
for (uint32_t i = 0u; i < descriptorCount; ++i)
188188
{
189189
switch (category)
@@ -234,7 +234,7 @@ bool ICPUDescriptorSet::isAnyDependencyDummy_impl(uint32_t _levelsBelow) const
234234
auto descriptorInfos = m_descriptorInfos[t]->begin();
235235
assert(descriptorInfos);
236236

237-
const auto category = getCategoryFromType(type);
237+
const auto category = IDescriptor::GetTypeCategory(type);
238238
for (uint32_t i = 0u; i < descriptorCount; ++i)
239239
{
240240
switch (category)

‎src/nbl/video/CVulkanLogicalDevice.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -551,13 +551,13 @@ core::smart_refctd_ptr<IGPUDescriptorSetLayout> CVulkanLogicalDevice::createDesc
551551
vkDescSetLayoutBinding.stageFlags = getVkShaderStageFlagsFromShaderStage(binding.stageFlags);
552552
vkDescSetLayoutBinding.pImmutableSamplers = nullptr;
553553

554-
if ((binding.type == asset::IDescriptor::E_TYPE::ET_SAMPLER or binding.type==asset::IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER) and binding.samplers and binding.count)
554+
if ((binding.type == asset::IDescriptor::E_TYPE::ET_SAMPLER or binding.type==asset::IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER) and binding.immutableSamplers and binding.count)
555555
{
556556
// If descriptorType is VK_DESCRIPTOR_TYPE_SAMPLER or VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, and descriptorCount is not 0 and pImmutableSamplers is not NULL:
557557
// pImmutableSamplers must be a valid pointer to an array of descriptorCount valid VkSampler handles.
558558
const uint32_t samplerOffset = vk_samplers.size();
559559
for (uint32_t i=0u; i<binding.count; ++i)
560-
vk_samplers.push_back(static_cast<const CVulkanSampler*>(binding.samplers[i].get())->getInternalObject());
560+
vk_samplers.push_back(static_cast<const CVulkanSampler*>(binding.immutableSamplers[i].get())->getInternalObject());
561561
vkDescSetLayoutBinding.pImmutableSamplers = vk_samplers.data()+samplerOffset;
562562
}
563563
}

‎src/nbl/video/IGPUDescriptorSet.cpp

Lines changed: 46 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,18 @@ asset::IDescriptor::E_TYPE IGPUDescriptorSet::validateWrite(const IGPUDescriptor
4949
// screw it, we'll need to replace the descriptor writing with update templates of descriptor buffer soon anyway
5050
redirect_t::storage_range_index_t descriptorRedirectBindingIndex;
5151
const auto descriptorType = getBindingType(bindingNumber, descriptorRedirectBindingIndex);
52+
if (asset::IDescriptor::E_TYPE::ET_COUNT == descriptorType)
53+
{
54+
if (debugName)
55+
m_pool->m_logger.log("Descriptor set (%s, %p) has no binding %u.", system::ILogger::ELL_ERROR, debugName, this, write.binding);
56+
else
57+
m_pool->m_logger.log("Descriptor set (%p) has no binding %u.", system::ILogger::ELL_ERROR, this, write.binding);
58+
59+
return asset::IDescriptor::E_TYPE::ET_COUNT;
60+
}
5261

5362
auto* descriptors = getDescriptors(descriptorType, descriptorRedirectBindingIndex);
63+
// Left this check, but if the above passed then this should never be nullptr I believe
5464
if (!descriptors)
5565
{
5666
if (debugName)
@@ -61,49 +71,52 @@ asset::IDescriptor::E_TYPE IGPUDescriptorSet::validateWrite(const IGPUDescriptor
6171
return asset::IDescriptor::E_TYPE::ET_COUNT;
6272
}
6373

64-
core::smart_refctd_ptr<video::IGPUSampler>* mutableSamplers = nullptr;
74+
// Possible TODO: ensure the types are the same, not just categories! Requires IDescriptor to provide a virtual getType() method
75+
for (uint32_t i = 0; i < write.count; ++i)
76+
{
77+
if (asset::IDescriptor::GetTypeCategory(descriptorType) != write.info[i].desc->getTypeCategory())
78+
{
79+
if (debugName)
80+
m_pool->m_logger.log("Descriptor set (%s, %p) doesn't allow descriptor of such type category at binding %u.", system::ILogger::ELL_ERROR, debugName, this, write.binding);
81+
else
82+
m_pool->m_logger.log("Descriptor set (%p) doesn't allow descriptor of such type category at binding %u.", system::ILogger::ELL_ERROR, this, write.binding);
83+
84+
return asset::IDescriptor::E_TYPE::ET_COUNT;
85+
}
86+
}
87+
6588
if (descriptorType == asset::IDescriptor::E_TYPE::ET_SAMPLER or (descriptorType == asset::IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER and write.info->info.combinedImageSampler.sampler))
6689
{
6790
if (m_layout->getImmutableSamplerRedirect().getCount(IGPUDescriptorSetLayout::CBindingRedirect::binding_number_t{ write.binding }) != 0)
6891
{
6992
if (debugName)
70-
m_pool->m_logger.log("Trying to write samplers at binding %u of Descriptor set (%s, %p), but those are immutable.", system::ILogger::ELL_ERROR, debugName, this, write.binding);
93+
m_pool->m_logger.log("Trying to write samplers at binding %u of Descriptor set (%s, %p), but those are immutable.", system::ILogger::ELL_ERROR, write.binding, debugName, this);
7194
else
72-
m_pool->m_logger.log("Trying to write samplers at binding %u of Descriptor set (%p), but those are immutable.", system::ILogger::ELL_ERROR, this, write.binding);
95+
m_pool->m_logger.log("Trying to write samplers at binding %u of Descriptor set (%p), but those are immutable.", system::ILogger::ELL_ERROR, write.binding, this);
7396
return asset::IDescriptor::E_TYPE::ET_COUNT;
7497
}
7598

7699
for (uint32_t i=0; i<write.count; ++i)
77100
{
78-
if (asset::IDescriptor::GetTypeCategory(descriptorType) != write.info[i].desc->getTypeCategory())
79-
{
80-
if (debugName)
81-
m_pool->m_logger.log("Descriptor set (%s, %p) doesn't allow descriptor of such type category at binding %u.", system::ILogger::ELL_ERROR, debugName, this, write.binding);
82-
else
83-
m_pool->m_logger.log("Descriptor set (%p) doesn't allow descriptor of such type category at binding %u.", system::ILogger::ELL_ERROR, this, write.binding);
84-
85-
return asset::IDescriptor::E_TYPE::ET_COUNT;
86-
}
87101
auto* sampler = descriptorType == asset::IDescriptor::E_TYPE::ET_SAMPLER ? reinterpret_cast<IGPUSampler*>(write.info[i].desc.get()) : write.info[i].info.combinedImageSampler.sampler.get();
88102
if (not sampler) {
89103
if (debugName)
90-
m_pool->m_logger.log("Null sampler provided when trying to write descriptor set (%s, %p). All writes should provide a sampler.", system::ILogger::ELL_ERROR, debugName, write.dstSet);
104+
m_pool->m_logger.log("Null sampler provided when trying to write descriptor set (%s, %p) at binding %u. All writes should provide a sampler.", system::ILogger::ELL_ERROR, debugName, this, write.binding);
91105
else
92-
m_pool->m_logger.log("Null sampler provided when trying to write descriptor set (%p). All writes should provide a sampler.", system::ILogger::ELL_ERROR, write.dstSet);
106+
m_pool->m_logger.log("Null sampler provided when trying to write descriptor set (%p) at binding %u. All writes should provide a sampler.", system::ILogger::ELL_ERROR, this, write.binding);
93107
return asset::IDescriptor::E_TYPE::ET_COUNT;
94108
}
95-
else if (not sampler->isCompatibleDevicewise(write.dstSet)) {
109+
if (not sampler->isCompatibleDevicewise(write.dstSet)) {
96110
const char* samplerDebugName = sampler->getDebugName();
97111
if (samplerDebugName && debugName)
98-
m_pool->m_logger.log("Sampler (%s, %p) does not exist or is not device-compatible with descriptor set (%s, %p).", system::ILogger::ELL_ERROR, samplerDebugName, sampler, debugName, write.dstSet);
112+
m_pool->m_logger.log("Sampler (%s, %p) does not exist or is not device-compatible with descriptor set (%s, %p).", system::ILogger::ELL_ERROR, samplerDebugName, sampler, debugName, this);
99113
else
100-
m_pool->m_logger.log("Sampler (%p) does not exist or is not device-compatible with descriptor set (%p).", system::ILogger::ELL_ERROR, sampler, write.dstSet);
114+
m_pool->m_logger.log("Sampler (%p) does not exist or is not device-compatible with descriptor set (%p).", system::ILogger::ELL_ERROR, sampler, this);
101115
return asset::IDescriptor::E_TYPE::ET_COUNT;
102116
}
103117
}
104-
105118
if (descriptorType == asset::IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER) {
106-
mutableSamplers = getMutableCombinedSamplers(bindingNumber);
119+
core::smart_refctd_ptr<video::IGPUSampler>* mutableSamplers = getMutableCombinedSamplers(bindingNumber);
107120
// Should never reach here, the GetTypeCategory check earlier should ensure that
108121
if (!mutableSamplers)
109122
{
@@ -117,6 +130,20 @@ asset::IDescriptor::E_TYPE IGPUDescriptorSet::validateWrite(const IGPUDescriptor
117130
}
118131
}
119132

133+
// https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#descriptorsets-combinedimagesampler
134+
if (asset::IDescriptor::GetTypeCategory(descriptorType) == asset::IDescriptor::EC_IMAGE)
135+
for (uint32_t i = 0; i < write.count; ++i)
136+
{
137+
auto layout = write.info[i].info.image.imageLayout;
138+
if (not (asset::IImage::LAYOUT::GENERAL == layout or asset::IImage::LAYOUT::SHARED_PRESENT == layout or asset::IImage::LAYOUT::READ_ONLY_OPTIMAL == layout))
139+
{
140+
if (debugName)
141+
m_pool->m_logger.log("When writing to descriptor set (%s, %p), an image was provided with a layout that isn't GENERAL, SHARED_PRESENT or READ_ONLY_OPTIMAL", system::ILogger::ELL_ERROR, debugName, this);
142+
else
143+
m_pool->m_logger.log("When writing to descriptor set (%p), an image was provided with a layout that isn't GENERAL, SHARED_PRESENT or READ_ONLY_OPTIMAL", system::ILogger::ELL_ERROR, this);
144+
}
145+
}
146+
120147
return descriptorType;
121148
}
122149

‎src/nbl/video/ILogicalDevice.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -394,9 +394,9 @@ core::smart_refctd_ptr<IGPUDescriptorSetLayout> ILogicalDevice::createDescriptor
394394
else if (binding.type==asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER_DYNAMIC)
395395
dynamicUBOCount++;
396396
// If binding comes with samplers, we're specifying that this binding corresponds to immutable samplers
397-
else if ((binding.type == asset::IDescriptor::E_TYPE::ET_SAMPLER or binding.type==asset::IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER) and binding.samplers)
397+
else if ((binding.type == asset::IDescriptor::E_TYPE::ET_SAMPLER or binding.type==asset::IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER) and binding.immutableSamplers)
398398
{
399-
auto* samplers = binding.samplers;
399+
auto* samplers = binding.immutableSamplers;
400400
for (uint32_t i=0u; i<binding.count; ++i)
401401
if ((not samplers[i]) or (not samplers[i]->wasCreatedBy(this)))
402402
return nullptr;

0 commit comments

Comments
 (0)