From c7dc2c7829b2fe1afa30a1884fec7c23bde1359a Mon Sep 17 00:00:00 2001 From: johndoe6345789 Date: Wed, 7 Jan 2026 11:59:16 +0000 Subject: [PATCH] feat(shader): Enhance shader attribute location handling and add logging for conflicts in MaterialX shader generation --- src/services/impl/bgfx_graphics_backend.cpp | 34 +++++---- src/services/impl/bgfx_gui_service.cpp | 69 +++++++++++++++++-- src/services/impl/graphics_service.cpp | 23 ++++++- .../impl/materialx_shader_generator.cpp | 28 +++++++- 4 files changed, 135 insertions(+), 19 deletions(-) diff --git a/src/services/impl/bgfx_graphics_backend.cpp b/src/services/impl/bgfx_graphics_backend.cpp index 60aa4c9..a938b43 100644 --- a/src/services/impl/bgfx_graphics_backend.cpp +++ b/src/services/impl/bgfx_graphics_backend.cpp @@ -282,10 +282,10 @@ BgfxGraphicsBackend::BgfxGraphicsBackend(std::shared_ptr configS ", platformService=" + std::string(platformService_ ? "set" : "null")); } vertexLayout_.begin() - .add(bgfx::Attrib::Position, 3, bgfx::AttribType::Float) - .add(bgfx::Attrib::Normal, 3, bgfx::AttribType::Float) - .add(bgfx::Attrib::TexCoord0, 2, bgfx::AttribType::Float) - .add(bgfx::Attrib::Color0, 3, bgfx::AttribType::Float) + .add(bgfx::Attrib::Position, 3, bgfx::AttribType::Float) // location 0 + .add(bgfx::Attrib::Normal, 3, bgfx::AttribType::Float) // location 1 + .add(bgfx::Attrib::TexCoord0, 2, bgfx::AttribType::Float) // location 2 + .add(bgfx::Attrib::Color0, 3, bgfx::AttribType::Float) // location 3 .end(); const std::array identity = { @@ -1056,22 +1056,32 @@ void BgfxGraphicsBackend::Draw(GraphicsDeviceHandle device, GraphicsPipelineHand const auto& vb = vertexIt->second; const auto& ib = indexIt->second; - uint32_t startVertex = static_cast(std::max(0, vertexOffset)); - uint32_t availableVertices = vb->vertexCount > startVertex - ? vb->vertexCount - startVertex - : 0; - if (availableVertices == 0) { - return; + if (logger_) { + logger_->Trace("BgfxGraphicsBackend", "Draw", + "vertexOffset=" + std::to_string(vertexOffset) + + ", indexOffset=" + std::to_string(indexOffset) + + ", indexCount=" + std::to_string(indexCount) + + ", totalVertices=" + std::to_string(vb->vertexCount)); } - bgfx::setTransform(modelMatrix.data()); + // When using indexed rendering with a vertex offset, bgfx expects: + // - setVertexBuffer: (handle, startVertex=0, numVertices=all) + // - setIndexBuffer: (handle, firstIndex, numIndices) + // The indices in the index buffer are already adjusted to reference the correct vertices + // in the combined vertex buffer, so we should NOT apply vertexOffset again here. + // Using the full vertex buffer ensures all vertex data is accessible. + + // NOTE: Do NOT call bgfx::setTransform() when using MaterialX shaders with explicit uniforms. + // MaterialX shaders receive transformation matrices via explicit uniforms (u_worldMatrix, + // u_worldViewProjectionMatrix, etc.) set in ApplyMaterialXUniforms(). Calling setTransform() + // causes conflicts, especially with Vulkan, resulting in garbage/rainbow artifacts. ApplyMaterialXUniforms(modelMatrix); for (const auto& binding : pipelineIt->second->textures) { if (bgfx::isValid(binding.sampler) && bgfx::isValid(binding.texture)) { bgfx::setTexture(binding.stage, binding.sampler, binding.texture); } } - bgfx::setVertexBuffer(0, vb->handle, startVertex, availableVertices); + bgfx::setVertexBuffer(0, vb->handle); bgfx::setIndexBuffer(ib->handle, indexOffset, indexCount); bgfx::setState(BGFX_STATE_WRITE_RGB | BGFX_STATE_WRITE_A | BGFX_STATE_WRITE_Z | BGFX_STATE_DEPTH_TEST_LESS | BGFX_STATE_CULL_CW | BGFX_STATE_MSAA); diff --git a/src/services/impl/bgfx_gui_service.cpp b/src/services/impl/bgfx_gui_service.cpp index a299450..e2f1368 100644 --- a/src/services/impl/bgfx_gui_service.cpp +++ b/src/services/impl/bgfx_gui_service.cpp @@ -58,7 +58,7 @@ layout(location = 2) in vec2 inTexCoord; layout(location = 0) out vec4 fragColor; layout(location = 1) out vec2 fragTexCoord; -layout(std140) uniform GuiUniforms { +layout(std140, binding = 0) uniform GuiUniforms { mat4 u_modelViewProj; }; @@ -77,7 +77,7 @@ layout(location = 1) in vec2 fragTexCoord; layout(location = 0) out vec4 outColor; -uniform sampler2D s_tex; +layout(binding = 0) uniform sampler2D s_tex; void main() { outColor = fragColor * texture(s_tex, fragTexCoord); @@ -260,10 +260,23 @@ void BgfxGuiService::InitializeResources() { modelViewProjUniform_ = bgfx::createUniform("u_modelViewProj", bgfx::UniformType::Mat4); sampler_ = bgfx::createUniform("s_tex", bgfx::UniformType::Sampler); + + if (logger_) { + logger_->Trace("BgfxGuiService", "InitializeResources", + "Uniforms created: modelViewProj=" + std::to_string(bgfx::isValid(modelViewProjUniform_)) + + ", sampler=" + std::to_string(bgfx::isValid(sampler_))); + } + program_ = CreateProgram(kGuiVertexSource, kGuiFragmentSource); const uint32_t whitePixel = 0xffffffff; whiteTexture_ = CreateTexture(reinterpret_cast(&whitePixel), 1, 1, kGuiSamplerFlags); + + if (logger_) { + logger_->Trace("BgfxGuiService", "InitializeResources", + "Resources created: program=" + std::to_string(bgfx::isValid(program_)) + + ", whiteTexture=" + std::to_string(bgfx::isValid(whiteTexture_))); + } if (!bgfx::isValid(program_) && logger_) { logger_->Error("BgfxGuiService::InitializeResources: Failed to create GUI shader program"); @@ -357,6 +370,16 @@ void BgfxGuiService::ApplyGuiView(uint32_t width, uint32_t height) { if (logger_ && (previousWidth != width || previousHeight != height)) { logger_->Trace("BgfxGuiService", "ApplyGuiView", "viewport=" + std::to_string(width) + "x" + std::to_string(height)); + logger_->Trace("BgfxGuiService", "ApplyGuiView", + "projection[0-3]=[" + std::to_string(proj[0]) + "," + + std::to_string(proj[1]) + "," + + std::to_string(proj[2]) + "," + + std::to_string(proj[3]) + "]"); + logger_->Trace("BgfxGuiService", "ApplyGuiView", + "projection[12-15]=[" + std::to_string(proj[12]) + "," + + std::to_string(proj[13]) + "," + + std::to_string(proj[14]) + "," + + std::to_string(proj[15]) + "]"); } bgfx::setViewTransform(viewId_, view, proj); @@ -638,10 +661,30 @@ void BgfxGuiService::SubmitQuad(const GuiVertex& v0, float identity[16]; bx::mtxIdentity(identity); + if (logger_) { + logger_->Trace("BgfxGuiService", "SubmitQuad", + "vertex[0]: pos=[" + std::to_string(v0.x) + "," + std::to_string(v0.y) + "," + std::to_string(v0.z) + + "], color=[" + std::to_string(v0.r) + "," + std::to_string(v0.g) + "," + std::to_string(v0.b) + "," + std::to_string(v0.a) + + "], uv=[" + std::to_string(v0.u) + "," + std::to_string(v0.v) + "]"); + logger_->Trace("BgfxGuiService", "SubmitQuad", + "uniforms: mvp=" + std::to_string(bgfx::isValid(modelViewProjUniform_)) + + ", sampler=" + std::to_string(bgfx::isValid(sampler_)) + + ", program=" + std::to_string(bgfx::isValid(program_)) + + ", texture=" + std::to_string(bgfx::isValid(texture)) + + ", viewId=" + std::to_string(viewId_)); + logger_->Trace("BgfxGuiService", "SubmitQuad", + "viewProjection[0-3]=[" + std::to_string(viewProjection_[0]) + "," + + std::to_string(viewProjection_[1]) + "," + + std::to_string(viewProjection_[2]) + "," + + std::to_string(viewProjection_[3]) + "]"); + } + SetScissor(scissor); bgfx::setTransform(identity); if (bgfx::isValid(modelViewProjUniform_)) { bgfx::setUniform(modelViewProjUniform_, viewProjection_.data()); + } else if (logger_) { + logger_->Error("BgfxGuiService::SubmitQuad: modelViewProjUniform_ is invalid!"); } bgfx::setTexture(0, sampler_, texture); bgfx::setVertexBuffer(0, &tvb, 0, 4); @@ -835,12 +878,20 @@ bgfx::TextureHandle BgfxGuiService::CreateTexture(const uint8_t* rgba, bgfx::ProgramHandle BgfxGuiService::CreateProgram(const char* vertexSource, const char* fragmentSource) const { if (!vertexSource || !fragmentSource) { + if (logger_) { + logger_->Error("BgfxGuiService::CreateProgram: null shader source"); + } return BGFX_INVALID_HANDLE; } bgfx::ShaderHandle vs = CreateShader("gui_vertex", vertexSource, true); bgfx::ShaderHandle fs = CreateShader("gui_fragment", fragmentSource, false); if (!bgfx::isValid(vs) || !bgfx::isValid(fs)) { + if (logger_) { + logger_->Error("BgfxGuiService::CreateProgram: shader compilation failed (vs=" + + std::to_string(bgfx::isValid(vs)) + ", fs=" + + std::to_string(bgfx::isValid(fs)) + ")"); + } if (bgfx::isValid(vs)) { bgfx::destroy(vs); } @@ -849,7 +900,14 @@ bgfx::ProgramHandle BgfxGuiService::CreateProgram(const char* vertexSource, } return BGFX_INVALID_HANDLE; } - return bgfx::createProgram(vs, fs, true); + + bgfx::ProgramHandle program = bgfx::createProgram(vs, fs, true); + if (!bgfx::isValid(program) && logger_) { + logger_->Error("BgfxGuiService::CreateProgram: bgfx::createProgram failed to link shaders"); + } else if (logger_) { + logger_->Trace("BgfxGuiService", "CreateProgram", "GUI program created successfully"); + } + return program; } bgfx::ShaderHandle BgfxGuiService::CreateShader(const std::string& label, @@ -884,7 +942,10 @@ bgfx::ShaderHandle BgfxGuiService::CreateShader(const std::string& label, shaderc::CompileOptions options; options.SetTargetEnvironment(shaderc_target_env_vulkan, shaderc_env_version_vulkan_1_1); options.SetAutoBindUniforms(true); - options.SetAutoMapLocations(true); + // Do NOT use SetAutoMapLocations - it overrides explicit layout(location=N) declarations + // and assigns locations alphabetically by variable name, breaking the vertex layout. + // GUI shaders already specify explicit locations matching the VertexLayout. + // options.SetAutoMapLocations(true); shaderc_shader_kind kind = isVertex ? shaderc_vertex_shader : shaderc_fragment_shader; auto result = compiler.CompileGlslToSpv(source, kind, label.c_str(), options); diff --git a/src/services/impl/graphics_service.cpp b/src/services/impl/graphics_service.cpp index 8c31661..e09307a 100644 --- a/src/services/impl/graphics_service.cpp +++ b/src/services/impl/graphics_service.cpp @@ -156,13 +156,34 @@ void GraphicsService::RenderScene(const std::vector& commands, // Set the view-projection matrix for the frame backend_->SetViewState(viewState); - // Execute draw calls + // Execute draw calls with explicit fallback shader support for (const auto& command : commands) { auto it = pipelines_.find(command.shaderKey); if (it != pipelines_.end()) { backend_->Draw(device_, it->second, vertexBuffer_, indexBuffer_, command.indexOffset, command.indexCount, command.vertexOffset, command.modelMatrix); + } else { + // Try explicit fallback shaders in order + bool drawn = false; + + for (const auto& fallbackKey : command.fallbackShaderKeys) { + auto fallbackIt = pipelines_.find(fallbackKey); + if (fallbackIt != pipelines_.end()) { + logger_->Warn("GraphicsService::RenderScene: Pipeline not found for shaderKey='" + + command.shaderKey + "', using fallback='" + fallbackKey + "'"); + backend_->Draw(device_, fallbackIt->second, vertexBuffer_, indexBuffer_, + command.indexOffset, command.indexCount, command.vertexOffset, + command.modelMatrix); + drawn = true; + break; + } + } + + if (!drawn) { + logger_->Warn("GraphicsService::RenderScene: Pipeline not found for shaderKey='" + + command.shaderKey + "' and no fallback shaders available or found"); + } } } } diff --git a/src/services/impl/materialx_shader_generator.cpp b/src/services/impl/materialx_shader_generator.cpp index 76dd1b8..641175a 100644 --- a/src/services/impl/materialx_shader_generator.cpp +++ b/src/services/impl/materialx_shader_generator.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -482,8 +483,12 @@ std::string ConvertIndividualInputsToBlock(const std::string& source, // location=0: position (vec3) // location=1: normal (vec3) // location=2: texcoord (vec2) - // location=3: color/tangent (vec3/vec4) + // location=3: color (vec3) + // Note: tangent is NOT in our Vertex struct, so it gets assigned to location 4+ std::map nameToLocation; + std::set usedLocations; + int nextAvailableLocation = 4; // Start assigning non-standard attributes from location 4 + for (const auto& [loc, type, name] : inputs) { int newLoc = loc; // default: keep original @@ -493,8 +498,26 @@ std::string ConvertIndividualInputsToBlock(const std::string& source, newLoc = 1; } else if (name.find("texcoord") != std::string::npos) { newLoc = 2; - } else if (name.find("color") != std::string::npos || name.find("tangent") != std::string::npos) { + } else if (name.find("color") != std::string::npos) { newLoc = 3; + } else if (name.find("tangent") != std::string::npos) { + // Tangent is not in our Vertex struct, assign to unused location + newLoc = nextAvailableLocation++; + if (logger) { + logger->Trace("MaterialXShaderGenerator", "ConvertIndividualInputsToBlock", + "Tangent attribute found but not in Vertex struct, assigning location " + + std::to_string(newLoc) + " (original was " + std::to_string(loc) + ")"); + } + } + + // Check for location conflicts and assign a new location if needed + while (usedLocations.count(newLoc) > 0) { + newLoc = nextAvailableLocation++; + if (logger) { + logger->Trace("MaterialXShaderGenerator", "ConvertIndividualInputsToBlock", + "Location conflict for " + name + ", assigning location " + + std::to_string(newLoc)); + } } if (logger && newLoc != loc) { @@ -504,6 +527,7 @@ std::string ConvertIndividualInputsToBlock(const std::string& source, } nameToLocation[name] = newLoc; + usedLocations.insert(newLoc); } // Sort by remapped location