Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

renamed "Uniform" to "DescriptorBuffer" #956

Closed
wants to merge 3 commits into from
Closed

renamed "Uniform" to "DescriptorBuffer" #956

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ccc46e
renamed uniformBinding to bufferBinding to allow for either uniform o…
theodoregoetz Sep 4, 2023
0b5ab79
refactor to ShaderSet::DescriptorBindings
theodoregoetz Sep 5, 2023
177522d
now using Descriptor and DescriptorBuffer everywhere in GraphicsPipel…
theodoregoetz Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions include/vsg/utils/GraphicsPipelineConfigurator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,13 @@ namespace vsg
/// call after all the textures and buffers have been explictly assigned to add in texture/buffer descriptors that are enabled by default (define == "").
bool assignDefaults();

/// deprecated. use enableBuffer() instead
[[deprecated("use enableBuffer()")]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be enableDescriptor()?

W.r.t [[deprecated]], do you know whether doxygen can handle it in a sensible way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. GraphicsPipelineConfigurator::enableBuffer() is still good in my opinion. see my previous comment posted at the same time as yours just now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think enableBuffer() is more confusing that the original enableUniform(). Buffer has a distinct mean in VSG and Vulkan, it refers to a whole buffer not a subset that is can be as a uniform and storage.

enableDescriptor() is more generic than enableTexture()/enableUniform() but it does at least still map to DescriptorBuffer/DescriptorImage. If we want to replace enableUniform()/enableTexture() then it would probably be best as enableDescriptorBuffer()/enableDescriptorImage().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I came to nearly the same conclusion but decided at the time that shorter method names would be better. Note that the word "descriptor" shows up twice in DescriptorConfigurator::enableDescriptorBuffer() so why not just make it DescriptorConfigurator::enableBuffer()?

But really I could have gone either way. DescriptorConfigurator::enableDescriptorBuffer() is not too bad in length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I want to keep this PR to the uniform/storage issue and follow-up with a rename for the texture/image methods right after.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps just DescriptorConfigurator::enable(..) then?

Though, as we'll need to keep it consistent with the GraphicsPipelineConfigurator API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can stand the slight inconsistency for this PR, I'd like to make this change first, keeping it as is at the moment (enableDescriptorBuffer(), etc) and come back with another PR to rename enableTexture() to enableDescriptorImage(). This is because the down-stream changes are many and separating this out keeps it all straight in my head.

bool enableUniform(const std::string& name);
/// deprecated. use assignBuffer() instead

[[deprecated("use assignBuffer()")]]
bool assignUniform(const std::string& name, ref_ptr<Data> data = {}, uint32_t dstArrayElement = 0);
/// deprecated. use assignBuffer() instead

[[deprecated("use assignBuffer()")]]
bool assignUniform(const std::string& name, const BufferInfoList& bufferInfoList, uint32_t dstArrayElement = 0);

std::set<std::string> assigned;
Expand Down Expand Up @@ -135,9 +137,10 @@ namespace vsg
ref_ptr<GraphicsPipeline> graphicsPipeline;
ref_ptr<BindGraphicsPipeline> bindGraphicsPipeline;

/// deprecated. use enableUniform() instead
[[deprecated("use enableBuffer()")]]
bool enableUniform(const std::string& name);
/// deprecated. use assignBuffer() instead

[[deprecated("use assignBuffer()")]]
bool assignUniform(const std::string& name, ref_ptr<Data> data = {});

protected:
Expand Down
40 changes: 24 additions & 16 deletions include/vsg/utils/ShaderSet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ namespace vsg
};
VSG_type_name(vsg::AttributeBinding);

struct VSG_DECLSPEC BufferBinding
struct VSG_DECLSPEC DescriptorBinding
{
std::string name;
std::string define;
Expand All @@ -46,11 +46,11 @@ namespace vsg
VkShaderStageFlags stageFlags = 0;
ref_ptr<Data> data;

int compare(const BufferBinding& rhs) const;
int compare(const DescriptorBinding& rhs) const;

explicit operator bool() const noexcept { return !name.empty(); }
};
VSG_type_name(vsg::BufferBinding);
VSG_type_name(vsg::DescriptorBinding);

struct VSG_DECLSPEC PushConstantRange
{
Expand Down Expand Up @@ -116,7 +116,7 @@ namespace vsg
ShaderStages stages;

std::vector<AttributeBinding> attributeBindings;
std::vector<BufferBinding> bufferBindings;
std::vector<DescriptorBinding> descriptorBindings;
std::vector<PushConstantRange> pushConstantRanges;
std::vector<DefinesArrayState> definesArrayStates; // put more constrained ArrayState matches first so they are matched first.
std::set<std::string> optionalDefines;
Expand All @@ -134,30 +134,30 @@ namespace vsg
void addAttributeBinding(std::string name, std::string define, uint32_t location, VkFormat format, ref_ptr<Data> data);

/// add a uniform, storage or texture binding. Not thread safe, should only be called when initially setting up the ShaderSet
void addBufferBinding(std::string name, std::string define, uint32_t set, uint32_t binding, VkDescriptorType descriptorType, uint32_t descriptorCount, VkShaderStageFlags stageFlags, ref_ptr<Data> data);
void addDescriptorBinding(std::string name, std::string define, uint32_t set, uint32_t binding, VkDescriptorType descriptorType, uint32_t descriptorCount, VkShaderStageFlags stageFlags, ref_ptr<Data> data);

/// add a push constant range. Not thread safe, should only be called when initially setting up the ShaderSet
void addPushConstantRange(std::string name, std::string define, VkShaderStageFlags stageFlags, uint32_t offset, uint32_t size);

/// get the AttributeBinding associated with name
AttributeBinding& getAttributeBinding(const std::string& name);

/// get the BufferBinding (uniform, storage or texture) associated with name
BufferBinding& getBufferBinding(const std::string& name);
/// get the DescriptorBinding (uniform, storage or texture) associated with name
DescriptorBinding& getDescriptorBinding(const std::string& name);

/// get the const AttributeBinding associated with name
const AttributeBinding& getAttributeBinding(const std::string& name) const;

/// get the const BufferBinding (uniform, storage or texture) associated with name
const BufferBinding& getBufferBinding(const std::string& name) const;
/// get the const DescriptorBinding (uniform, storage or texture) associated with name
const DescriptorBinding& getDescriptorBinding(const std::string& name) const;

/// get the first ArrayState that has matches with defines in the specified list of defines.
ref_ptr<ArrayState> getSuitableArrayState(const std::set<std::string>& defines) const;

/// get the ShaderStages variant that uses specified ShaderCompileSettings.
ShaderStages getShaderStages(ref_ptr<ShaderCompileSettings> scs = {});

/// return the <minimum_set, maximum_set+1> range of set numbers encompassing BufferBindings
/// return the <minimum_set, maximum_set+1> range of set numbers encompassing DescriptorBindings
std::pair<uint32_t, uint32_t> descriptorSetRange() const;

/// create the descriptor set layout.
Expand All @@ -178,18 +178,23 @@ namespace vsg
void read(Input& input) override;
void write(Output& output) const override;

/// deprecated. use addBufferBinding() instead
/// deprecated. use descriptorBindings
std::vector<DescriptorBinding>& uniformBindings = descriptorBindings;

[[deprecated("use addDescriptorBinding()")]]
void addUniformBinding(std::string name, std::string define, uint32_t set, uint32_t binding, VkDescriptorType descriptorType, uint32_t descriptorCount, VkShaderStageFlags stageFlags, ref_ptr<Data> data);
/// deprecated. use getBufferBinding() instead
BufferBinding& getUniformBinding(const std::string& name);
/// deprecated. use getBufferBinding() instead
const BufferBinding& getUniformBinding(const std::string& name) const;

[[deprecated("use getDescriptorBinding()")]]
DescriptorBinding& getUniformBinding(const std::string& name);

[[deprecated("use getDescriptorBinding()")]]
const DescriptorBinding& getUniformBinding(const std::string& name) const;

protected:
virtual ~ShaderSet();

AttributeBinding _nullAttributeBinding;
BufferBinding _nullBufferBinding;
DescriptorBinding _nullDescriptorBinding;
};
VSG_type_name(vsg::ShaderSet);

Expand All @@ -202,4 +207,7 @@ namespace vsg
/// create a ShaderSet for Physics Based Rendering
extern VSG_DECLSPEC ref_ptr<ShaderSet> createPhysicsBasedRenderingShaderSet(ref_ptr<const Options> options = {});

/// deprecated. use vsg::DescriptorBinding
using UniformBinding = DescriptorBinding;

} // namespace vsg
2 changes: 1 addition & 1 deletion src/vsg/io/tile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ void tile::init(vsg::ref_ptr<const vsg::Options> options)
_graphicsPipelineConfig->enableArray("vsg_TexCoord0", VK_VERTEX_INPUT_RATE_VERTEX, 8, VK_FORMAT_R32G32_SFLOAT);
_graphicsPipelineConfig->enableArray("vsg_Color", VK_VERTEX_INPUT_RATE_INSTANCE, 16, VK_FORMAT_R32G32B32A32_SFLOAT);

if (auto& materialBinding = _shaderSet->getBufferBinding("material"))
if (auto& materialBinding = _shaderSet->getDescriptorBinding("material"))
{
ref_ptr<Data> mat = materialBinding.data;
if (!mat) mat = vsg::PhongMaterialValue::create();
Expand Down
2 changes: 1 addition & 1 deletion src/vsg/utils/Builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ ref_ptr<StateGroup> Builder::createStateGroup(const StateInfo& stateInfo)
graphicsPipelineConfig->assignTexture("displacementMap", stateInfo.displacementMap, sampler);
}

if (auto& materialBinding = activeShaderSet->getBufferBinding("material"))
if (auto& materialBinding = activeShaderSet->getDescriptorBinding("material"))
{
ref_ptr<Data> mat = materialBinding.data;
if (!mat) mat = vsg::PhongMaterialValue::create();
Expand Down
34 changes: 17 additions & 17 deletions src/vsg/utils/GraphicsPipelineConfigurator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ void DescriptorConfigurator::reset()

bool DescriptorConfigurator::enableTexture(const std::string& name)
{
if (auto& textureBinding = shaderSet->getBufferBinding(name))
if (auto& textureBinding = shaderSet->getDescriptorBinding(name))
{
assigned.insert(name);

Expand All @@ -118,7 +118,7 @@ bool DescriptorConfigurator::enableTexture(const std::string& name)

bool DescriptorConfigurator::assignTexture(const std::string& name, ref_ptr<Data> textureData, ref_ptr<Sampler> sampler, uint32_t dstArrayElement)
{
if (auto& textureBinding = shaderSet->getBufferBinding(name))
if (auto& textureBinding = shaderSet->getDescriptorBinding(name))
{
assigned.insert(name);

Expand All @@ -135,7 +135,7 @@ bool DescriptorConfigurator::assignTexture(const std::string& name, ref_ptr<Data

bool DescriptorConfigurator::assignTexture(const std::string& name, const ImageInfoList& imageInfoList, uint32_t dstArrayElement)
{
if (auto& textureBinding = shaderSet->getBufferBinding(name))
if (auto& textureBinding = shaderSet->getDescriptorBinding(name))
{
assigned.insert(name);

Expand All @@ -151,7 +151,7 @@ bool DescriptorConfigurator::assignTexture(const std::string& name, const ImageI

bool DescriptorConfigurator::enableBuffer(const std::string& name)
theodoregoetz marked this conversation as resolved.
Show resolved Hide resolved
{
if (auto& bufferBinding = shaderSet->getBufferBinding(name))
if (auto& bufferBinding = shaderSet->getDescriptorBinding(name))
{
assigned.insert(name);

Expand All @@ -176,7 +176,7 @@ bool DescriptorConfigurator::enableUniform(const std::string& name)

bool DescriptorConfigurator::assignBuffer(const std::string& name, ref_ptr<Data> data, uint32_t dstArrayElement)
{
if (auto& bufferBinding = shaderSet->getBufferBinding(name))
if (auto& bufferBinding = shaderSet->getDescriptorBinding(name))
{
assigned.insert(name);

Expand All @@ -200,7 +200,7 @@ bool DescriptorConfigurator::assignUniform(const std::string& name, ref_ptr<Data

bool DescriptorConfigurator::assignBuffer(const std::string& name, const BufferInfoList& bufferInfoList, uint32_t dstArrayElement)
theodoregoetz marked this conversation as resolved.
Show resolved Hide resolved
{
if (auto& bufferBinding = shaderSet->getBufferBinding(name))
if (auto& bufferBinding = shaderSet->getDescriptorBinding(name))
{
assigned.insert(name);

Expand Down Expand Up @@ -248,14 +248,14 @@ bool DescriptorConfigurator::assignDefaults()
bool assignedDefault = false;
if (shaderSet)
{
for (auto& bufferBinding : shaderSet->bufferBindings)
for (auto& descriptorBinding : shaderSet->descriptorBindings)
{
if (bufferBinding.define.empty() && assigned.count(bufferBinding.name) == 0)
if (descriptorBinding.define.empty() && assigned.count(descriptorBinding.name) == 0)
{
bool set_matched = false;
for (auto& cds : shaderSet->customDescriptorSetBindings)
{
if (cds->set == bufferBinding.set)
if (cds->set == descriptorBinding.set)
{
set_matched = true;
break;
Expand All @@ -264,7 +264,7 @@ bool DescriptorConfigurator::assignDefaults()
if (!set_matched)
{
bool isTexture = false;
switch (bufferBinding.descriptorType)
switch (descriptorBinding.descriptorType)
{
case (VK_DESCRIPTOR_TYPE_SAMPLER):
case (VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER):
Expand All @@ -278,16 +278,16 @@ bool DescriptorConfigurator::assignDefaults()

if (isTexture)
{
assignDescriptor(bufferBinding.set, bufferBinding.binding, bufferBinding.descriptorType, bufferBinding.descriptorCount, bufferBinding.stageFlags,
DescriptorImage::create(Sampler::create(), bufferBinding.data, bufferBinding.binding, 0, bufferBinding.descriptorType));
assignDescriptor(descriptorBinding.set, descriptorBinding.binding, descriptorBinding.descriptorType, descriptorBinding.descriptorCount, descriptorBinding.stageFlags,
DescriptorImage::create(Sampler::create(), descriptorBinding.data, descriptorBinding.binding, 0, descriptorBinding.descriptorType));
}
else
{
assignDescriptor(bufferBinding.set, bufferBinding.binding, bufferBinding.descriptorType, bufferBinding.descriptorCount, bufferBinding.stageFlags,
DescriptorBuffer::create(bufferBinding.data, bufferBinding.binding, 0, bufferBinding.descriptorType));
assignDescriptor(descriptorBinding.set, descriptorBinding.binding, descriptorBinding.descriptorType, descriptorBinding.descriptorCount, descriptorBinding.stageFlags,
DescriptorBuffer::create(descriptorBinding.data, descriptorBinding.binding, 0, descriptorBinding.descriptorType));
}

assigned.insert(bufferBinding.name);
assigned.insert(descriptorBinding.name);
assignedDefault = true;
}
}
Expand Down Expand Up @@ -471,8 +471,8 @@ bool GraphicsPipelineConfigurator::assignBuffer(const std::string& name, ref_ptr
/// deprecated. use GraphicsPipelineConfigurator::assignBuffer() instead
bool GraphicsPipelineConfigurator::assignUniform(const std::string& name, ref_ptr<Data> data)
{
warn("GraphicsPipelineConfigurator::enableUniform() has been deprecated."
" use GraphicsPipelineConfigurator::enableBuffer() instead.");
warn("GraphicsPipelineConfigurator::assignUniform() has been deprecated."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the warning redundant if we are using the [[deprecated]] compiler hint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. I don't know how this attribute and how VSG's logging plays with release and debug. I figured you might have some opinion on which is best. I can take this out if you think it best.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this PR is settled a bit further I'll merge this PR as a branch and then do testing with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for deprecations, I believe it best to be obnoxious about it. Otherwise it's easier to ignore.

" use GraphicsPipelineConfigurator::assignBuffer() instead.");
return assignBuffer(name, data);
}

Expand Down
Loading