Skip to content

Commit

Permalink
Fix static-analysis issues
Browse files Browse the repository at this point in the history
  • Loading branch information
egorodet committed Dec 28, 2024
1 parent 690ccfd commit 6d02e75
Show file tree
Hide file tree
Showing 10 changed files with 120 additions and 85 deletions.
30 changes: 17 additions & 13 deletions Apps/07-ParallelRendering/ParallelRenderingApp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ static const std::map<pin::Keyboard::State, ParallelRenderingAppAction> g_parall
};

#ifdef ROOT_CONSTANTS_ENABLED
#define APP_VARIANT_NAME "Root Constants"
constexpr char const* g_app_variant_name = "Root Constants";
#else
#define APP_VARIANT_NAME "Buffer Views"
constexpr char const* g_app_variant_name = "Buffer Views";
#endif

bool ParallelRenderingApp::Settings::operator==(const Settings& other) const noexcept
Expand All @@ -93,7 +93,8 @@ uint32_t ParallelRenderingApp::Settings::GetActiveRenderThreadCount() const noex

ParallelRenderingApp::ParallelRenderingApp()
: UserInterfaceApp(
GetGraphicsTutorialAppSettings("Methane Parallel Rendering (" APP_VARIANT_NAME ")", AppOptions::GetDefaultWithColorDepthAndAnim()),
GetGraphicsTutorialAppSettings(fmt::format("Methane Parallel Rendering ({})", g_app_variant_name),
AppOptions::GetDefaultWithColorDepthAndAnim()),
GetUserInterfaceTutorialAppSettings(AppOptions::GetDefaultWithColorDepthAndAnim()),
"Methane tutorial of parallel rendering")
{
Expand Down Expand Up @@ -196,7 +197,7 @@ void ParallelRenderingApp::Init()
);

// Create frame buffer resources
const Data::Size uniform_data_size = MeshBuffers::GetUniformSize();

tf::Taskflow program_bindings_task_flow;
for(ParallelRenderingFrame& frame : GetFrames())
{
Expand All @@ -218,6 +219,7 @@ void ParallelRenderingApp::Init()
frame.cubes_uniform_argument_binding_ptrs[0] = &frame.cubes_program_bindings[0].Get({ rhi::ShaderType::All, "g_uniforms" });
frame.cubes_program_bindings[0].SetName(fmt::format("Cube 0 Bindings {}", frame.index));
#else
const Data::Size uniform_data_size = MeshBuffers::GetUniformSize();
frame.cubes_array.program_bindings_per_instance.resize(cubes_count);
frame.cubes_array.program_bindings_per_instance[0] = render_state_settings.program.CreateBindings({
{
Expand All @@ -229,21 +231,22 @@ void ParallelRenderingApp::Init()
{ { rhi::ShaderType::Pixel, "g_sampler" }, m_texture_sampler.GetResourceView() },
}, frame.index);
frame.cubes_array.program_bindings_per_instance[0].SetName(fmt::format("Cube 0 Bindings {}", frame.index));

const MeshBuffers& cube_array_buffers = *m_cube_array_buffers_ptr;
#endif

MeshBuffers& cube_array_buffers = *m_cube_array_buffers_ptr;
program_bindings_task_flow.for_each_index(1U, cubes_count, 1U,
[&frame, &cube_array_buffers, uniform_data_size](const uint32_t cube_index)
{
// NOTE: workaround for Clang error unused-lambda-capture uniform_data_size (false positive)
META_UNUSED(uniform_data_size);

#ifdef ROOT_CONSTANTS_ENABLED
META_UNUSED(cube_array_buffers);
[&frame](const uint32_t cube_index)
{
rhi::ProgramBindings& cube_program_bindings = frame.cubes_program_bindings[cube_index];
cube_program_bindings = rhi::ProgramBindings(frame.cubes_program_bindings[0], {}, frame.index);
frame.cubes_uniform_argument_binding_ptrs[cube_index] = &cube_program_bindings.Get({ rhi::ShaderType::All, "g_uniforms" });
cube_program_bindings.SetName(fmt::format("Cube {} Bindings {}", cube_index, frame.index));
}
#else
[&frame, &cube_array_buffers, uniform_data_size](const uint32_t cube_index)
{
rhi::ProgramBindings& cube_program_bindings = frame.cubes_array.program_bindings_per_instance[cube_index];
cube_program_bindings = rhi::ProgramBindings(frame.cubes_array.program_bindings_per_instance[0], {
{
Expand All @@ -253,9 +256,10 @@ void ParallelRenderingApp::Init()
uniform_data_size)
}
}, frame.index);
#endif
cube_program_bindings.SetName(fmt::format("Cube {} Bindings {}", cube_index, frame.index));
});
}
#endif
);

if (m_settings.parallel_rendering_enabled)
{
Expand Down
27 changes: 16 additions & 11 deletions Modules/Data/Types/Include/Methane/Data/Chunk.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ Data chunk representing owning or non-owning memory container

#include "Types.h"

#include <type_traits>

namespace Methane::Data
{

Expand All @@ -43,15 +45,18 @@ class Chunk // NOSONAR - rule of zero is not applicable
, m_data_size(static_cast<Size>(m_data_storage.size()))
{ }

template<typename T>
template<typename T,
typename = std::enable_if_t<!std::is_base_of_v<Chunk, std::remove_cv_t<std::remove_reference_t<T>>>>>
explicit Chunk(T& value)
: m_data_ptr(GetByteAddress(value))
, m_data_size(static_cast<Size>(sizeof(T)))
{ }

template<typename T>
template<typename T,
typename = std::enable_if_t<!std::is_base_of_v<Chunk, std::remove_cv_t<std::remove_reference_t<T>>>>>
explicit Chunk(T&& value)
: m_data_storage(GetByteAddress(value), GetByteAddress(value) + sizeof(T))
: m_data_storage(GetByteAddress(std::forward<T>(value)),
GetByteAddress(std::forward<T>(value)) + sizeof(T))
, m_data_ptr(m_data_storage.data())
, m_data_size(static_cast<Size>(m_data_storage.size()))
{ }
Expand Down Expand Up @@ -84,19 +89,19 @@ class Chunk // NOSONAR - rule of zero is not applicable
return *this;
}

bool operator==(const Chunk& other) const noexcept
friend bool operator==(const Chunk& left, const Chunk& right) noexcept
{
return m_data_size == other.m_data_size &&
(m_data_ptr == other.m_data_ptr ||
memcmp(m_data_ptr, other.m_data_ptr, m_data_size) == 0);
return left.m_data_size == right.m_data_size &&
(left.m_data_ptr == right.m_data_ptr ||
memcmp(left.m_data_ptr, right.m_data_ptr, left.m_data_size) == 0);
}

bool operator!=(const Chunk& other) const noexcept
friend bool operator!=(const Chunk& left, const Chunk& right) noexcept
{
return !operator==(other);
return !(left == right);
}

operator bool() const noexcept
explicit operator bool() const noexcept
{
return !IsEmptyOrNull();
}
Expand Down Expand Up @@ -135,7 +140,7 @@ class Chunk // NOSONAR - rule of zero is not applicable

private:
template<typename T>
static const std::byte* GetByteAddress(T&& value)
static const std::byte* GetByteAddress(const T& value)
{
return reinterpret_cast<const std::byte*>(std::addressof(value)); // NOSONAR
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class DescriptorManager

// IDescriptorManager interface
void AddProgramBindings(Rhi::IProgramBindings& program_bindings) override;
void RemoveProgramBindings(Rhi::IProgramBindings&) override {}
void RemoveProgramBindings(Rhi::IProgramBindings&) override { /* intentionally unimplemented */}
void CompleteInitialization() override;
void Release() override;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ class Program
const Settings& GetSettings() const noexcept final { return m_settings; }
const Rhi::ShaderTypes& GetShaderTypes() const noexcept final { return m_shader_types; }
const Ptr<Rhi::IShader>& GetShader(Rhi::ShaderType shader_type) const final;
bool HasShader(Rhi::ShaderType shader_type) const { return !!GetShader(shader_type); }
Data::Size GetBindingsCount() const noexcept final { return m_bindings_count; }
bool HasShader(Rhi::ShaderType shader_type) const { return !!GetShader(shader_type); }
Data::Size GetBindingsCount() const noexcept final { return m_bindings_count; }

// IObject overrides
bool SetName(std::string_view name) override;
Expand Down Expand Up @@ -98,6 +98,12 @@ class Program
private:
using RootFrameConstantBuffers = std::vector<UniquePtr<RootConstantBuffer>>;

void ExtractShaderTypesByArgumentName(Rhi::ShaderTypes& all_shader_types,
std::map<std::string_view, Rhi::ShaderTypes, std::less<>>& shader_types_by_argument_name_map);
void MergeAllShaderBindings(const Rhi::ShaderTypes& all_shader_types,
const std::map<std::string_view, Rhi::ShaderTypes, std::less<>>& shader_types_by_argument_name_map);
void InitFrameConstantArgumentBindings();

const Context& m_context;
Settings m_settings;
const ShadersByType m_shaders_by_type;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class ProgramBindings;
class RootConstantAccessor;
class RootConstantBuffer;

class ProgramArgumentBinding
class ProgramArgumentBinding // NOSONAR - destructor is defaulted in CPP, to allow deleting of incomplete type
: public Rhi::IProgramArgumentBinding
, public Data::Emitter<Rhi::IProgramArgumentBindingCallback>
, public std::enable_shared_from_this<ProgramArgumentBinding>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ namespace Methane::Graphics::Base

class RootConstantStorage;

class RootConstantAccessor
class RootConstantAccessor // NOSONAR - custom destructor is required
{
public:
using Range = Data::Range<Data::Index>;
Expand Down Expand Up @@ -97,7 +97,7 @@ class RootConstantStorage
using Mutex = std::mutex;
#endif

std::lock_guard<Mutex> GetLockGuard();
std::scoped_lock<Mutex> GetLockGuard();
bool IsDataResizeRequired() const noexcept { return m_data_resize_required.load(); }

private:
Expand Down Expand Up @@ -142,17 +142,17 @@ class RootConstantBuffer final
[[nodiscard]] Rhi::ResourceView GetResourceView(Data::Size offset, Data::Size size);

void SetBufferName(std::string_view buffer_name);
std::string_view GetBufferName() { return m_buffer_name; }
std::string_view GetBufferName() const { return m_buffer_name; }

private:
using RangeSet = Data::RangeSet<Data::Index>;

void UpdateGpuBuffer(Rhi::ICommandQueue& target_cmd_queue);

// Rhi::IContextCallback overrides
void OnContextUploadingResources(Rhi::IContext& context) final;
void OnContextReleased(Rhi::IContext&) final { /* event not handled */ }
void OnContextInitialized(Rhi::IContext&) final { /* event not handled */ }
void OnContextUploadingResources(Rhi::IContext& context) override;
void OnContextReleased(Rhi::IContext&) override { /* event not handled */ }
void OnContextInitialized(Rhi::IContext&) override { /* event not handled */ }

Context& m_context;
std::string m_buffer_name;
Expand Down
87 changes: 52 additions & 35 deletions Modules/Graphics/RHI/Base/Sources/Methane/Graphics/Base/Program.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,22 @@ const Ptr<Rhi::IShader>& Program::GetShader(Rhi::ShaderType shader_type) const
void Program::InitArgumentBindings()
{
META_FUNCTION_TASK();
Rhi::ShaderTypes all_shader_types;

Rhi::ShaderTypes all_shader_types;
std::map<std::string_view, Rhi::ShaderTypes, std::less<>> shader_types_by_argument_name_map;
std::map<Argument, Ptr<ArgumentBinding>> binding_by_argument;
ExtractShaderTypesByArgumentName(all_shader_types, shader_types_by_argument_name_map);

if (all_shader_types.size() > 1)
{
MergeAllShaderBindings(all_shader_types, shader_types_by_argument_name_map);
}

InitFrameConstantArgumentBindings();
}

void Program::ExtractShaderTypesByArgumentName(Rhi::ShaderTypes& all_shader_types,
std::map<std::string_view, Rhi::ShaderTypes, std::less<>>& shader_types_by_argument_name_map)
{
m_binding_by_argument.clear();
for (const Ptr<Rhi::IShader>& shader_ptr : m_settings.shaders)
{
Expand All @@ -104,49 +116,53 @@ void Program::InitArgumentBindings()
}
}
}
}

if (all_shader_types.size() > 1)
void Program::MergeAllShaderBindings(const Rhi::ShaderTypes& all_shader_types,
const std::map<std::string_view, Rhi::ShaderTypes, std::less<>>& shader_types_by_argument_name_map)
{
// Replace bindings for argument set for all shader types in program to one binding set for argument with ShaderType::All
for (const auto& [argument_name, shader_types]: shader_types_by_argument_name_map)
{
// Replace bindings for argument set for all shader types in program to one binding set for argument with ShaderType::All
for (const auto& [argument_name, shader_types]: shader_types_by_argument_name_map)
if (shader_types != all_shader_types)
{
if (shader_types != all_shader_types)
for(Rhi::ShaderType shader_type : shader_types)
{
for(Rhi::ShaderType shader_type : shader_types)
{
const Argument shader_argument(shader_type, argument_name);
const auto argument_and_binding_it = m_binding_by_argument.find(shader_argument);
META_CHECK_TRUE(argument_and_binding_it != m_binding_by_argument.end() && argument_and_binding_it->second);
m_settings.argument_accessors.emplace(argument_and_binding_it->second->GetSettings().argument);
}
continue;
const Argument shader_argument(shader_type, argument_name);
const auto argument_and_binding_it = m_binding_by_argument.find(shader_argument);
META_CHECK_TRUE(argument_and_binding_it != m_binding_by_argument.end() && argument_and_binding_it->second);
m_settings.argument_accessors.emplace(argument_and_binding_it->second->GetSettings().argument);
}
continue;
}

Ptr<ProgramBindings::ArgumentBinding> argument_binding_ptr;
for (Rhi::ShaderType shader_type: all_shader_types)
Ptr<ProgramBindings::ArgumentBinding> argument_binding_ptr;
for (Rhi::ShaderType shader_type: all_shader_types)
{
const Argument argument{ shader_type, argument_name };
auto binding_by_argument_it = m_binding_by_argument.find(argument);
META_CHECK_DESCR(argument, binding_by_argument_it != m_binding_by_argument.end(),
"Resource binding was not initialized for for argument");
if (argument_binding_ptr)
{
const Argument argument{ shader_type, argument_name };
auto binding_by_argument_it = m_binding_by_argument.find(argument);
META_CHECK_DESCR(argument, binding_by_argument_it != m_binding_by_argument.end(),
"Resource binding was not initialized for for argument");
if (argument_binding_ptr)
{
argument_binding_ptr->MergeSettings(*binding_by_argument_it->second);
}
else
{
argument_binding_ptr = binding_by_argument_it->second;
}
m_binding_by_argument.erase(binding_by_argument_it);
argument_binding_ptr->MergeSettings(*binding_by_argument_it->second);
}

META_CHECK_NOT_NULL_DESCR(argument_binding_ptr, "failed to create resource binding for argument '{}'", argument_name);
const Argument all_shaders_argument{ Rhi::ShaderType::All, argument_name };
m_binding_by_argument.try_emplace(all_shaders_argument, argument_binding_ptr);
m_settings.argument_accessors.emplace(all_shaders_argument, argument_binding_ptr->GetSettings().argument.GetAccessorType());
else
{
argument_binding_ptr = binding_by_argument_it->second;
}
m_binding_by_argument.erase(binding_by_argument_it);
}

META_CHECK_NOT_NULL_DESCR(argument_binding_ptr, "failed to create resource binding for argument '{}'", argument_name);
const Argument all_shaders_argument{ Rhi::ShaderType::All, argument_name };
m_binding_by_argument.try_emplace(all_shaders_argument, argument_binding_ptr);
m_settings.argument_accessors.emplace(all_shaders_argument, argument_binding_ptr->GetSettings().argument.GetAccessorType());
}
}

void Program::InitFrameConstantArgumentBindings()
{
if (m_context.GetType() != Rhi::IContext::Type::Render)
{
const auto frame_constant_binding_by_arg_it =
Expand All @@ -160,7 +176,7 @@ void Program::InitArgumentBindings()

// Create frame-constant argument bindings only when program is created in render context
m_frame_bindings_by_argument.clear();
const auto& render_context = static_cast<const RenderContext&>(m_context);
const auto& render_context = static_cast<const RenderContext&>(m_context);
const uint32_t frame_buffers_count = render_context.GetSettings().frame_buffers_count;
META_CHECK_GREATER_OR_EQUAL(frame_buffers_count, 2);

Expand All @@ -177,6 +193,7 @@ void Program::InitArgumentBindings()
}
m_frame_bindings_by_argument.try_emplace(program_argument, std::move(per_frame_argument_bindings));
}
return;
}

bool Program::SetName(std::string_view name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ void ProgramBindings::SetResourcesForArguments(const BindingValueByArgument& bin
auto& argument_binding = dynamic_cast<ArgumentBinding&>(Get(program_argument));
argument_binding.SetEmitCallbackEnabled(false); // do not emit callback during initialization
std::visit(
[&argument_binding](auto&& value)
[&argument_binding](auto& value)
{
using T = std::decay_t<decltype(value)>;
if constexpr (std::is_same_v<T, Rhi::RootConstant>)
Expand Down
Loading

0 comments on commit 6d02e75

Please sign in to comment.