From 262c100e2f503a5beee638f889dff18b57982d30 Mon Sep 17 00:00:00 2001 From: johndoe6345789 Date: Thu, 8 Jan 2026 01:50:49 +0000 Subject: [PATCH] stuff --- CMakeLists.txt | 43 ++++ INITIALIZATION_ORDER_BUG.md | 15 +- src/services/impl/bgfx_graphics_backend.cpp | 16 +- src/services/impl/bgfx_graphics_backend.hpp | 6 + src/services/impl/graphics_service.cpp | 14 ++ tests/bgfx_backend_frame_guard_test.cpp | 68 ++++++ ...graphics_service_buffer_lifecycle_test.cpp | 203 ++++++++++++++++++ 7 files changed, 357 insertions(+), 8 deletions(-) create mode 100644 tests/bgfx_backend_frame_guard_test.cpp create mode 100644 tests/graphics_service_buffer_lifecycle_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 1c6af19..df2fafe 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -392,6 +392,7 @@ add_executable(script_engine_tests src/services/impl/scene_script_service.cpp src/services/impl/shader_script_service.cpp src/services/impl/materialx_shader_generator.cpp + src/services/impl/shader_pipeline_validator.cpp src/services/impl/sdl_window_service.cpp src/services/impl/ecs_service.cpp src/services/impl/scene_service.cpp @@ -432,6 +433,7 @@ add_executable(bgfx_gui_service_tests src/services/impl/bgfx_gui_service.cpp src/services/impl/bgfx_shader_compiler.cpp src/services/impl/materialx_shader_generator.cpp + src/services/impl/shader_pipeline_validator.cpp src/services/impl/pipeline_compiler_service.cpp ) target_include_directories(bgfx_gui_service_tests PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/src") @@ -464,6 +466,7 @@ add_executable(vulkan_shader_linking_test src/services/impl/bgfx_shader_compiler.cpp src/services/impl/json_config_service.cpp src/services/impl/materialx_shader_generator.cpp + src/services/impl/shader_pipeline_validator.cpp src/services/impl/platform_service.cpp src/services/impl/sdl_window_service.cpp src/events/event_bus.cpp @@ -564,6 +567,46 @@ target_link_libraries(render_coordinator_init_order_test PRIVATE GTest::gtest_main ) add_test(NAME render_coordinator_init_order_test COMMAND render_coordinator_init_order_test) + +# Test: Bgfx backend frame guard (ensures texture loads are blocked before first frame) +add_executable(bgfx_backend_frame_guard_test + tests/bgfx_backend_frame_guard_test.cpp + src/services/impl/bgfx_graphics_backend.cpp + src/services/impl/bgfx_shader_compiler.cpp + src/stb_image.cpp +) +target_include_directories(bgfx_backend_frame_guard_test PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/src") +target_include_directories(bgfx_backend_frame_guard_test PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/src/bgfx_tools/shaderc") +target_link_libraries(bgfx_backend_frame_guard_test PRIVATE + GTest::gtest + GTest::gtest_main + ${SDL3CPP_RENDER_STACK_LIBS} + ${SDL3CPP_STB_LIBS} + glm::glm + SDL3::SDL3 +) +if(TARGET shaderc_local) + target_link_libraries(bgfx_backend_frame_guard_test PRIVATE shaderc_local) +elseif(TARGET shaderc::shaderc) + target_link_libraries(bgfx_backend_frame_guard_test PRIVATE shaderc::shaderc) +elseif(TARGET shaderc::shaderc_combined) + target_link_libraries(bgfx_backend_frame_guard_test PRIVATE shaderc::shaderc_combined) +else() + message(WARNING "shaderc CMake target not found; skipping link for bgfx_backend_frame_guard_test.") +endif() +add_test(NAME bgfx_backend_frame_guard_test COMMAND bgfx_backend_frame_guard_test) + +# Test: Graphics service buffer lifecycle (TDD guard for VRAM leaks on reupload) +add_executable(graphics_service_buffer_lifecycle_test + tests/graphics_service_buffer_lifecycle_test.cpp + src/services/impl/graphics_service.cpp +) +target_include_directories(graphics_service_buffer_lifecycle_test PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/src") +target_link_libraries(graphics_service_buffer_lifecycle_test PRIVATE + GTest::gtest + GTest::gtest_main +) +add_test(NAME graphics_service_buffer_lifecycle_test COMMAND graphics_service_buffer_lifecycle_test) endif() if(ENABLE_VITA) diff --git a/INITIALIZATION_ORDER_BUG.md b/INITIALIZATION_ORDER_BUG.md index 05bcfc7..ca7981f 100644 --- a/INITIALIZATION_ORDER_BUG.md +++ b/INITIALIZATION_ORDER_BUG.md @@ -182,12 +182,11 @@ bgfx::TextureHandle BgfxGraphicsBackend::LoadTextureFromFile( } // VALIDATION: Ensure bgfx is ready for texture creation - const bgfx::Stats* stats = bgfx::getStats(); - if (stats && stats->numFrames == 0) { + // (bgfx::Stats does not expose a frame counter, so we track it ourselves.) + if (!HasProcessedFrame()) { if (logger_) { logger_->Error("BgfxGraphicsBackend::LoadTextureFromFile: " - "Attempted to load texture BEFORE first bgfx::frame()! " - "This will cause GPU driver crashes. " + "Attempted to load texture BEFORE first bgfx::frame(). " "Fix: Call BeginFrame()+EndFrame() before loading textures. " "path=" + path); } @@ -215,8 +214,10 @@ After implementing fix, check logs for proper order: [INFO] Application starting [TRACE] BgfxGraphicsBackend::Initialize [TRACE] RenderCoordinatorService::RenderFrame - shadersLoaded=false -[TRACE] BgfxGraphicsBackend::BeginFrame -[TRACE] BgfxGraphicsBackend::EndFrame frameNum=1 ← CRITICAL: First frame +[TRACE] RenderCoordinatorService::RenderFrame - Priming bgfx with a dummy frame before shader load +[TRACE] GraphicsService::BeginFrame +[TRACE] GraphicsService::EndFrame +[TRACE] BgfxGraphicsBackend::EndFrame frameNumber=0 ← CRITICAL: First frame [TRACE] GraphicsService::LoadShaders ← After first frame [TRACE] BgfxGraphicsBackend::CreatePipeline shaderKey=floor [TRACE] BgfxGraphicsBackend::LoadTextureFromFile path=wood_color.jpg @@ -692,4 +693,4 @@ Read bgfx_frame_requirement_test.cpp Now I'll update TEST 1 to use the manual frame tracking: -Now update TEST 2: \ No newline at end of file +Now update TEST 2: diff --git a/src/services/impl/bgfx_graphics_backend.cpp b/src/services/impl/bgfx_graphics_backend.cpp index 62a8a4e..09ea4b0 100644 --- a/src/services/impl/bgfx_graphics_backend.cpp +++ b/src/services/impl/bgfx_graphics_backend.cpp @@ -454,6 +454,7 @@ void BgfxGraphicsBackend::Initialize(void* window, const GraphicsConfig& config) if (initialized_) { return; } + frameCount_ = 0; (void)config; SDL_Window* sdlWindow = static_cast(window); @@ -609,6 +610,7 @@ void BgfxGraphicsBackend::Shutdown() { DestroyUniforms(); bgfx::shutdown(); initialized_ = false; + frameCount_ = 0; } void BgfxGraphicsBackend::RecreateSwapchain(uint32_t width, uint32_t height) { @@ -700,6 +702,13 @@ bgfx::TextureHandle BgfxGraphicsBackend::LoadTextureFromFile(const std::string& if (logger_) { logger_->Trace("BgfxGraphicsBackend", "LoadTextureFromFile", "path=" + path); } + if (!HasProcessedFrame()) { + if (logger_) { + logger_->Error("BgfxGraphicsBackend::LoadTextureFromFile: Attempted to load texture BEFORE first " + "bgfx::frame(). Call BeginFrame()+EndFrame() before loading textures. path=" + path); + } + return BGFX_INVALID_HANDLE; + } int width = 0; int height = 0; @@ -1063,7 +1072,12 @@ bool BgfxGraphicsBackend::EndFrame(GraphicsDeviceHandle device) { if (!initialized_) { return false; } - bgfx::frame(); + const uint32_t frameNumber = bgfx::frame(); + frameCount_ = frameNumber + 1; + if (logger_) { + logger_->Trace("BgfxGraphicsBackend", "EndFrame", + "frameNumber=" + std::to_string(frameNumber)); + } return true; } diff --git a/src/services/impl/bgfx_graphics_backend.hpp b/src/services/impl/bgfx_graphics_backend.hpp index 0302293..9c6ec4f 100644 --- a/src/services/impl/bgfx_graphics_backend.hpp +++ b/src/services/impl/bgfx_graphics_backend.hpp @@ -51,6 +51,10 @@ public: void* GetGraphicsQueue() const override; private: + friend bgfx::TextureHandle LoadTextureFromFileForTest(const BgfxGraphicsBackend& backend, + const std::string& path, + uint64_t samplerFlags); + // Texture memory budget tracker to prevent GPU memory exhaustion class TextureMemoryTracker { public: @@ -142,6 +146,7 @@ private: void ApplyMaterialXUniforms(const std::array& modelMatrix); void DestroyPipelines(); void DestroyBuffers(); + bool HasProcessedFrame() const { return frameCount_ > 0; } std::shared_ptr configService_; std::shared_ptr platformService_; @@ -156,6 +161,7 @@ private: uint32_t viewportWidth_ = 0; uint32_t viewportHeight_ = 0; bool initialized_ = false; + uint32_t frameCount_ = 0; bgfx::ViewId viewId_ = 0; PlatformHandleInfo platformHandleInfo_{}; bgfx::PlatformData platformData_{}; diff --git a/src/services/impl/graphics_service.cpp b/src/services/impl/graphics_service.cpp index 872318d..4d1fbcf 100644 --- a/src/services/impl/graphics_service.cpp +++ b/src/services/impl/graphics_service.cpp @@ -113,6 +113,13 @@ void GraphicsService::UploadVertexData(const std::vector& vertices throw std::runtime_error("Graphics service not initialized"); } + if (vertexBuffer_ && backend_) { + logger_->Trace("GraphicsService", "UploadVertexData", + "destroyingPreviousVertexBuffer=true"); + backend_->DestroyBuffer(device_, vertexBuffer_); + vertexBuffer_ = nullptr; + } + // Convert vertices to bytes std::vector data(sizeof(core::Vertex) * vertices.size()); std::memcpy(data.data(), vertices.data(), data.size()); @@ -127,6 +134,13 @@ void GraphicsService::UploadIndexData(const std::vector& indices) { throw std::runtime_error("Graphics service not initialized"); } + if (indexBuffer_ && backend_) { + logger_->Trace("GraphicsService", "UploadIndexData", + "destroyingPreviousIndexBuffer=true"); + backend_->DestroyBuffer(device_, indexBuffer_); + indexBuffer_ = nullptr; + } + // Convert indices to bytes std::vector data(sizeof(uint16_t) * indices.size()); std::memcpy(data.data(), indices.data(), data.size()); diff --git a/tests/bgfx_backend_frame_guard_test.cpp b/tests/bgfx_backend_frame_guard_test.cpp new file mode 100644 index 0000000..ef2a0c2 --- /dev/null +++ b/tests/bgfx_backend_frame_guard_test.cpp @@ -0,0 +1,68 @@ +#include + +#include "services/impl/bgfx_graphics_backend.hpp" +#include "services/interfaces/i_logger.hpp" + +#include +#include +#include +#include + +namespace sdl3cpp::services::impl { + +bgfx::TextureHandle LoadTextureFromFileForTest(const BgfxGraphicsBackend& backend, + const std::string& path, + uint64_t samplerFlags) { + return backend.LoadTextureFromFile(path, samplerFlags); +} + +class CapturingLogger final : public sdl3cpp::services::ILogger { +public: + void SetLevel(sdl3cpp::services::LogLevel level) override { level_ = level; } + sdl3cpp::services::LogLevel GetLevel() const override { return level_; } + void SetOutputFile(const std::string&) override {} + void SetMaxLinesPerFile(size_t) override {} + void EnableConsoleOutput(bool) override {} + void Log(sdl3cpp::services::LogLevel level, const std::string& message) override { + if (level == sdl3cpp::services::LogLevel::ERROR) { + errors_.push_back(message); + } + } + void Trace(const std::string&) override {} + void Trace(const std::string&, const std::string&, const std::string&, const std::string&) override {} + void Debug(const std::string&) override {} + void Info(const std::string&) override {} + void Warn(const std::string&) override {} + void Error(const std::string& message) override { errors_.push_back(message); } + void TraceFunction(const std::string&) override {} + void TraceVariable(const std::string&, const std::string&) override {} + void TraceVariable(const std::string&, int) override {} + void TraceVariable(const std::string&, size_t) override {} + void TraceVariable(const std::string&, bool) override {} + void TraceVariable(const std::string&, float) override {} + void TraceVariable(const std::string&, double) override {} + + bool HasErrorContaining(const std::string& needle) const { + return std::any_of(errors_.begin(), errors_.end(), + [&needle](const std::string& message) { + return message.find(needle) != std::string::npos; + }); + } + +private: + sdl3cpp::services::LogLevel level_ = sdl3cpp::services::LogLevel::TRACE; + std::vector errors_; +}; + +TEST(BgfxGraphicsBackendFrameGuardTest, RejectsTextureLoadBeforeFirstFrame) { + auto logger = std::make_shared(); + BgfxGraphicsBackend backend(nullptr, nullptr, logger, nullptr); + + const bgfx::TextureHandle handle = + LoadTextureFromFileForTest(backend, "missing_texture.png", BGFX_TEXTURE_NONE); + + EXPECT_FALSE(bgfx::isValid(handle)); + EXPECT_TRUE(logger->HasErrorContaining("Attempted to load texture BEFORE first bgfx::frame()")); +} + +} // namespace sdl3cpp::services::impl diff --git a/tests/graphics_service_buffer_lifecycle_test.cpp b/tests/graphics_service_buffer_lifecycle_test.cpp new file mode 100644 index 0000000..599aa00 --- /dev/null +++ b/tests/graphics_service_buffer_lifecycle_test.cpp @@ -0,0 +1,203 @@ +#include + +#include "services/impl/graphics_service.hpp" +#include "services/interfaces/i_graphics_backend.hpp" +#include "services/interfaces/i_logger.hpp" +#include "services/interfaces/i_window_service.hpp" + +#include +#include +#include +#include +#include + +namespace { + +class NullLogger final : public sdl3cpp::services::ILogger { +public: + void SetLevel(sdl3cpp::services::LogLevel) override {} + sdl3cpp::services::LogLevel GetLevel() const override { return sdl3cpp::services::LogLevel::OFF; } + void SetOutputFile(const std::string&) override {} + void SetMaxLinesPerFile(size_t) override {} + void EnableConsoleOutput(bool) override {} + void Log(sdl3cpp::services::LogLevel, const std::string&) override {} + void Trace(const std::string&) override {} + void Trace(const std::string&, const std::string&, const std::string&, const std::string&) override {} + void Debug(const std::string&) override {} + void Info(const std::string&) override {} + void Warn(const std::string&) override {} + void Error(const std::string&) override {} + void TraceFunction(const std::string&) override {} + void TraceVariable(const std::string&, const std::string&) override {} + void TraceVariable(const std::string&, int) override {} + void TraceVariable(const std::string&, size_t) override {} + void TraceVariable(const std::string&, bool) override {} + void TraceVariable(const std::string&, float) override {} + void TraceVariable(const std::string&, double) override {} +}; + +class DummyWindowService final : public sdl3cpp::services::IWindowService { +public: + void CreateWindow(const sdl3cpp::services::WindowConfig&) override {} + void DestroyWindow() override {} + SDL_Window* GetNativeHandle() const override { return nullptr; } + std::pair GetSize() const override { return {800, 600}; } + bool ShouldClose() const override { return false; } + void PollEvents() override {} + void SetTitle(const std::string&) override {} + bool IsMinimized() const override { return false; } + void SetMouseGrabbed(bool) override {} + bool IsMouseGrabbed() const override { return false; } + void SetRelativeMouseMode(bool) override {} + bool IsRelativeMouseMode() const override { return false; } + void SetCursorVisible(bool) override {} + bool IsCursorVisible() const override { return true; } +}; + +class TrackingBackend final : public sdl3cpp::services::IGraphicsBackend { +public: + ~TrackingBackend() override { + CleanupUndestroyed(createdVertexBuffers_); + CleanupUndestroyed(createdIndexBuffers_); + } + + void Initialize(void*, const sdl3cpp::services::GraphicsConfig&) override {} + void Shutdown() override {} + void RecreateSwapchain(uint32_t, uint32_t) override {} + void WaitIdle() override {} + + sdl3cpp::services::GraphicsDeviceHandle CreateDevice() override { + return reinterpret_cast(this); + } + void DestroyDevice(sdl3cpp::services::GraphicsDeviceHandle) override {} + + sdl3cpp::services::GraphicsPipelineHandle CreatePipeline( + sdl3cpp::services::GraphicsDeviceHandle, + const std::string&, + const sdl3cpp::services::ShaderPaths&) override { + return nullptr; + } + void DestroyPipeline(sdl3cpp::services::GraphicsDeviceHandle, + sdl3cpp::services::GraphicsPipelineHandle) override {} + + sdl3cpp::services::GraphicsBufferHandle CreateVertexBuffer( + sdl3cpp::services::GraphicsDeviceHandle, + const std::vector&) override { + auto handle = NewHandle(); + createdVertexBuffers_.push_back(handle); + return handle; + } + + sdl3cpp::services::GraphicsBufferHandle CreateIndexBuffer( + sdl3cpp::services::GraphicsDeviceHandle, + const std::vector&) override { + auto handle = NewHandle(); + createdIndexBuffers_.push_back(handle); + return handle; + } + + void DestroyBuffer(sdl3cpp::services::GraphicsDeviceHandle, + sdl3cpp::services::GraphicsBufferHandle buffer) override { + destroyedBuffers_.push_back(buffer); + destroyed_.insert(buffer); + delete reinterpret_cast(buffer); + } + + bool BeginFrame(sdl3cpp::services::GraphicsDeviceHandle) override { return true; } + bool EndFrame(sdl3cpp::services::GraphicsDeviceHandle) override { return true; } + void SetViewState(const sdl3cpp::services::ViewState&) override {} + void Draw(sdl3cpp::services::GraphicsDeviceHandle, + sdl3cpp::services::GraphicsPipelineHandle, + sdl3cpp::services::GraphicsBufferHandle, + sdl3cpp::services::GraphicsBufferHandle, + uint32_t, + uint32_t, + int32_t, + const std::array&) override {} + sdl3cpp::services::GraphicsDeviceHandle GetPhysicalDevice() const override { return nullptr; } + std::pair GetSwapchainExtent() const override { return {0, 0}; } + uint32_t GetSwapchainFormat() const override { return 0; } + void* GetCurrentCommandBuffer() const override { return nullptr; } + void* GetGraphicsQueue() const override { return nullptr; } + + const std::vector& DestroyedBuffers() const { + return destroyedBuffers_; + } + + const std::vector& CreatedVertexBuffers() const { + return createdVertexBuffers_; + } + + const std::vector& CreatedIndexBuffers() const { + return createdIndexBuffers_; + } + +private: + sdl3cpp::services::GraphicsBufferHandle NewHandle() { + return reinterpret_cast(new int(nextId_++)); + } + + void CleanupUndestroyed(const std::vector& handles) { + for (auto handle : handles) { + if (destroyed_.count(handle) == 0) { + delete reinterpret_cast(handle); + } + } + } + + int nextId_ = 1; + std::unordered_set destroyed_; + std::vector destroyedBuffers_; + std::vector createdVertexBuffers_; + std::vector createdIndexBuffers_; +}; + +TEST(GraphicsServiceBufferLifecycleTest, ReuploadingVerticesDestroysPreviousBuffer) { + auto backend = std::make_shared(); + auto windowService = std::make_shared(); + auto logger = std::make_shared(); + + sdl3cpp::services::impl::GraphicsService service(logger, backend, windowService); + service.Initialize(); + service.InitializeDevice(nullptr, sdl3cpp::services::GraphicsConfig{}); + + std::vector verticesA(1); + service.UploadVertexData(verticesA); + ASSERT_EQ(backend->CreatedVertexBuffers().size(), 1u); + auto firstBuffer = backend->CreatedVertexBuffers().front(); + + std::vector verticesB(2); + service.UploadVertexData(verticesB); + + EXPECT_EQ(backend->DestroyedBuffers().size(), 1u) + << "Uploading new vertex data should destroy the previous buffer to avoid VRAM leaks."; + if (!backend->DestroyedBuffers().empty()) { + EXPECT_EQ(backend->DestroyedBuffers().front(), firstBuffer); + } +} + +TEST(GraphicsServiceBufferLifecycleTest, ReuploadingIndicesDestroysPreviousBuffer) { + auto backend = std::make_shared(); + auto windowService = std::make_shared(); + auto logger = std::make_shared(); + + sdl3cpp::services::impl::GraphicsService service(logger, backend, windowService); + service.Initialize(); + service.InitializeDevice(nullptr, sdl3cpp::services::GraphicsConfig{}); + + std::vector indicesA = {0, 1, 2}; + service.UploadIndexData(indicesA); + ASSERT_EQ(backend->CreatedIndexBuffers().size(), 1u); + auto firstBuffer = backend->CreatedIndexBuffers().front(); + + std::vector indicesB = {0, 1, 2, 3, 4, 5}; + service.UploadIndexData(indicesB); + + EXPECT_EQ(backend->DestroyedBuffers().size(), 1u) + << "Uploading new index data should destroy the previous buffer to avoid VRAM leaks."; + if (!backend->DestroyedBuffers().empty()) { + EXPECT_EQ(backend->DestroyedBuffers().front(), firstBuffer); + } +} + +} // namespace