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 2 commits
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
2 changes: 1 addition & 1 deletion include/vsg/state/DescriptorBuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace vsg
{

/// DescriptorBuffer is a Descriptor class that encapsulates the bufferInfoList used to set VkWriteDescriptorSet::pBufferInfo settings
/// DescriptorBuffer is a means for passing uniforms to shaders.
/// DescriptorBuffer is a means for passing uniform and storage buffers to shaders.
class VSG_DECLSPEC DescriptorBuffer : public Inherit<Descriptor, DescriptorBuffer>
{
public:
Expand Down
3 changes: 2 additions & 1 deletion include/vsg/state/StateCommand.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
namespace vsg
{

/// Base class for Vulkan commands associated with state, such as binding graphics pipelines and descriptor sets (textures and uniforms).
/// Base class for Vulkan commands associated with state, such as binding graphics
/// pipelines and descriptor sets for textures, uniform buffers and storage buffers.
/// StateCommands can be attached directly as nodes in the scene graph, or more typically assigned to StateGroup nodes to enable push/popping of state.
class VSG_DECLSPEC StateCommand : public Inherit<Command, StateCommand>
{
Expand Down
27 changes: 21 additions & 6 deletions include/vsg/utils/GraphicsPipelineConfigurator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,28 @@ namespace vsg
bool two_sided = false;

bool enableTexture(const std::string& name);
bool enableUniform(const std::string& name);
bool enableBuffer(const std::string& name);

bool assignTexture(const std::string& name, ref_ptr<Data> textureData = {}, ref_ptr<Sampler> sampler = {}, uint32_t dstArrayElement = 0);
bool assignTexture(const std::string& name, const ImageInfoList& imageInfoList, uint32_t dstArrayElement = 0);

bool assignUniform(const std::string& name, ref_ptr<Data> data = {}, uint32_t dstArrayElement = 0);
bool assignUniform(const std::string& name, const BufferInfoList& bufferInfoList, uint32_t dstArrayElement = 0);
bool assignBuffer(const std::string& name, ref_ptr<Data> data = {}, uint32_t dstArrayElement = 0);
bool assignBuffer(const std::string& name, const BufferInfoList& bufferInfoList, uint32_t dstArrayElement = 0);

bool assignDescriptor(uint32_t set, uint32_t binding, VkDescriptorType descriptorType, uint32_t descriptorCount, VkShaderStageFlags stageFlags, ref_ptr<Descriptor> descriptor);

/// call after all the textures/uniforms have been explictly assigned to add in textures/uniforms descriptors that are enabled by default (define == "").
/// 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()")]]
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()")]]
bool assignUniform(const std::string& name, ref_ptr<Data> data = {}, uint32_t dstArrayElement = 0);

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

std::set<std::string> assigned;
std::set<std::string> defines;
std::vector<ref_ptr<DescriptorSet>> descriptorSets;
Expand Down Expand Up @@ -105,11 +114,11 @@ namespace vsg

bool enableArray(const std::string& name, VkVertexInputRate vertexInputRate, uint32_t stride, VkFormat format = VK_FORMAT_UNDEFINED);
bool enableTexture(const std::string& name);
bool enableUniform(const std::string& name);
bool enableBuffer(const std::string& name);

bool assignArray(DataList& arrays, const std::string& name, VkVertexInputRate vertexInputRate, ref_ptr<Data> array);
bool assignTexture(const std::string& name, ref_ptr<Data> textureData = {}, ref_ptr<Sampler> sampler = {});
bool assignUniform(const std::string& name, ref_ptr<Data> data = {});
bool assignBuffer(const std::string& name, ref_ptr<Data> data = {});

// setup by assign calls
ref_ptr<ShaderCompileSettings> shaderHints;
Expand All @@ -128,6 +137,12 @@ namespace vsg
ref_ptr<GraphicsPipeline> graphicsPipeline;
ref_ptr<BindGraphicsPipeline> bindGraphicsPipeline;

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

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

protected:
void _assignShaderSetSettings();
};
Expand Down
39 changes: 27 additions & 12 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 UniformBinding
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 UniformBinding& rhs) const;
int compare(const DescriptorBinding& rhs) const;

explicit operator bool() const noexcept { return !name.empty(); }
};
VSG_type_name(vsg::UniformBinding);
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<UniformBinding> uniformBindings;
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 @@ -133,31 +133,31 @@ namespace vsg
/// add an attribute binding, Not thread safe, should only be called when initially setting up the ShaderSet
void addAttributeBinding(std::string name, std::string define, uint32_t location, VkFormat format, ref_ptr<Data> data);

/// add an uniform binding. Not thread safe, should only be called when initially setting up the ShaderSet
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);
/// add a uniform, storage or texture binding. Not thread safe, should only be called when initially setting up the ShaderSet
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 UniformBinding associated with name
UniformBinding& getUniformBinding(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 UniformBinding associated with name
const UniformBinding& getUniformBinding(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 UniformBindings
/// 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,11 +178,23 @@ namespace vsg
void read(Input& input) override;
void write(Output& output) const override;

/// 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 getDescriptorBinding()")]]
DescriptorBinding& getUniformBinding(const std::string& name);

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

protected:
virtual ~ShaderSet();

AttributeBinding _nullAttributeBinding;
UniformBinding _nullUniformBinding;
DescriptorBinding _nullDescriptorBinding;
};
VSG_type_name(vsg::ShaderSet);

Expand All @@ -195,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
4 changes: 2 additions & 2 deletions src/vsg/io/tile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,14 +327,14 @@ void tile::init(vsg::ref_ptr<const vsg::Options> options)
_graphicsPipelineConfig->enableTexture("displacementMap");
}
#endif
_graphicsPipelineConfig->enableUniform("material");
_graphicsPipelineConfig->enableBuffer("material");

_graphicsPipelineConfig->enableArray("vsg_Vertex", VK_VERTEX_INPUT_RATE_VERTEX, 12, VK_FORMAT_R32G32B32_SFLOAT);
_graphicsPipelineConfig->enableArray("vsg_Normal", VK_VERTEX_INPUT_RATE_VERTEX, 12, VK_FORMAT_R32G32B32_SFLOAT);
_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->getUniformBinding("material"))
if (auto& materialBinding = _shaderSet->getDescriptorBinding("material"))
{
ref_ptr<Data> mat = materialBinding.data;
if (!mat) mat = vsg::PhongMaterialValue::create();
Expand Down
2 changes: 2 additions & 0 deletions src/vsg/state/BufferInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ bool vsg::createBufferAndTransferData(Context& context, const BufferInfoList& bu

VkDeviceSize alignment = 4;
if (usage == VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT) alignment = device->getPhysicalDevice()->getProperties().limits.minUniformBufferOffsetAlignment;
else if (usage == VK_BUFFER_USAGE_STORAGE_BUFFER_BIT) alignment = device->getPhysicalDevice()->getProperties().limits.minStorageBufferOffsetAlignment;

VkDeviceSize totalSize = 0;
VkDeviceSize offset = 0;
Expand Down Expand Up @@ -303,6 +304,7 @@ BufferInfoList vsg::createHostVisibleBuffer(Device* device, const DataList& data

VkDeviceSize alignment = 4;
if (usage == VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT) alignment = device->getPhysicalDevice()->getProperties().limits.minUniformBufferOffsetAlignment;
else if (usage == VK_BUFFER_USAGE_STORAGE_BUFFER_BIT) alignment = device->getPhysicalDevice()->getProperties().limits.minStorageBufferOffsetAlignment;

VkDeviceSize totalSize = 0;
VkDeviceSize offset = 0;
Expand Down
4 changes: 2 additions & 2 deletions src/vsg/text/GpuLayoutTechnique.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ void GpuLayoutTechnique::setup(Text* text, uint32_t minimumAllocation, ref_ptr<c
config->assignTexture("textureAtlas", text->font->atlas, sampler);
config->assignTexture("glyphMetrics", glyphMetricsProxy, glyphMetricSampler);

config->assignUniform("textLayout", layoutValue);
config->assignUniform("text", textArray);
config->assignBuffer("textLayout", layoutValue);
config->assignBuffer("text", textArray);

// Set the InputAssemblyState.topology
struct SetPipelineStates : public Visitor
Expand Down
4 changes: 2 additions & 2 deletions src/vsg/utils/Builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ ref_ptr<StateGroup> Builder::createStateGroup(const StateInfo& stateInfo)
graphicsPipelineConfig->assignTexture("displacementMap", stateInfo.displacementMap, sampler);
}

if (auto& materialBinding = activeShaderSet->getUniformBinding("material"))
if (auto& materialBinding = activeShaderSet->getDescriptorBinding("material"))
{
ref_ptr<Data> mat = materialBinding.data;
if (!mat) mat = vsg::PhongMaterialValue::create();
graphicsPipelineConfig->assignUniform("material", mat);
graphicsPipelineConfig->assignBuffer("material", mat);
}

graphicsPipelineConfig->enableArray("vsg_Vertex", VK_VERTEX_INPUT_RATE_VERTEX, 12);
Expand Down
Loading