From f7ef65e4f9daae322214a7989a32a1ff75ed5077 Mon Sep 17 00:00:00 2001 From: Ole Bueker Date: Tue, 7 Jul 2026 19:35:31 +0200 Subject: [PATCH] fix(mcp): settle render-throttle transient before perf-lever writes return (#519) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A renderer-setting write (olo_renderer_settings_set) or scene load (olo_scene_open) landing right after a main-thread stall could return before the editor's render-budget throttle cleared, so the change hadn't actually rendered yet: RendererProfiler::BeginFrame/EndFrame only run inside Renderer3D::BeginScene, which the throttle skips, so GetLastCompletedFrameData() (what olo_perf_snapshot/olo_perf_pass_timings read) stayed frozen on stale pre-change data until an unrelated later frame rendered — the "first write doesn't take effect, repeatable A/B afterwards" symptom from #519's last open item. Both handlers now wait out the throttle/resize transient via AwaitRenderedFrames before returning, mirroring the settle discipline the camera-pose screenshot tools already use for the identical class of problem. Live-verified against a running editor: an immediate olo_renderer_settings_set(depthprepass) + olo_perf_pass_timings pair now shows the change with fresh (non-stale) GPU data on the first read. Co-Authored-By: Claude Sonnet 5 --- OloEditor/src/MCP/McpTools.cpp | 49 +++++++ OloEngine/tests/CMakeLists.txt | 5 + .../PropertyTests/McpPerfSettleTest.cpp | 138 ++++++++++++++++++ 3 files changed, 192 insertions(+) create mode 100644 OloEngine/tests/Rendering/PropertyTests/McpPerfSettleTest.cpp diff --git a/OloEditor/src/MCP/McpTools.cpp b/OloEditor/src/MCP/McpTools.cpp index 9aa4a7420..d13099fe4 100644 --- a/OloEditor/src/MCP/McpTools.cpp +++ b/OloEditor/src/MCP/McpTools.cpp @@ -3689,6 +3689,32 @@ namespace OloEngine::MCP if (result.is_object() && result.contains("__error")) return ToolResult::Error(result["__error"].get()); + + // Settle before returning (#519 "first perf-lever write right after + // scene load doesn't take effect on the GPU"). A lever flip right + // after a heavy scene load lands while the editor's render-budget + // throttle is still skipping frames — the just-blocked main thread + // reports a huge timestep for the next OnUpdate, which trips + // skipRender for a beat. While throttled, Renderer3D::BeginScene + // never runs, so RendererProfiler::BeginFrame/EndFrame don't either: + // GetLastCompletedFrameData() (what olo_perf_snapshot reads) stays + // frozen on whatever rendered before the write, making the change + // invisible until an unrelated later frame finally renders. Waiting + // out the same throttle/resize transient the screenshot tools + // already respect (AwaitRenderedFrames/IsCaptureUnready) guarantees + // at least a couple of real frames have executed with the new + // setting by the time this call returns, so an immediately + // following olo_perf_snapshot reads live data instead of a stale + // pre-change frame. + constexpr int kSettingsSettleFrames = 2; + if (server.Context().GetFrameIndex) + { + const u64 baseFrame = server.MarshalRead([&server]() -> Json + { return Json{ { "frame", server.Context().GetFrameIndex() } }; }) + .value("frame", static_cast(0)); + AwaitRenderedFrames(server, baseFrame, kSettingsSettleFrames); + } + return ToolResult::Text(result.dump(2)); } @@ -3751,6 +3777,29 @@ namespace OloEngine::MCP if (result.is_object() && result.contains("__error")) return ToolResult::Error(result["__error"].get()); + + // Settle before returning (#519 "first perf-lever write right after + // scene load doesn't take effect on the GPU"). The load above ran + // synchronously inside the MarshalRead job, blocking the main-thread + // frame pump for however long it took; the very next OnUpdate sees + // that stall as an inflated timestep and trips the render-budget + // throttle for a beat, during which Renderer3D::BeginScene (and so + // RendererProfiler::BeginFrame/EndFrame) never runs. A caller that + // immediately writes a renderer setting and reads back + // olo_perf_snapshot in that window sees stale pre-load data, not the + // new state — Handle_RendererSettingsSet settles on its own end too, + // but waiting here as well means a plain scene-load caller (no + // follow-up settings write) also gets a scene that has actually + // rendered before the tool returns. + constexpr int kPostLoadSettleFrames = 2; + if (server.Context().GetFrameIndex) + { + const u64 baseFrame = server.MarshalRead([&server]() -> Json + { return Json{ { "frame", server.Context().GetFrameIndex() } }; }) + .value("frame", static_cast(0)); + AwaitRenderedFrames(server, baseFrame, kPostLoadSettleFrames); + } + return ToolResult::Text(result.dump(2)); } diff --git a/OloEngine/tests/CMakeLists.txt b/OloEngine/tests/CMakeLists.txt index 7486eb3da..1c914ce64 100644 --- a/OloEngine/tests/CMakeLists.txt +++ b/OloEngine/tests/CMakeLists.txt @@ -208,6 +208,11 @@ add_executable(OloEngine-Tests # loop. Header-only host helper (McpHeadlessHost.h); pulls in the real MCP # tools (McpTools.cpp + McpScriptApi.cpp) added to the source list below. Rendering/PropertyTests/McpHeadlessAttachTest.cpp + # Pins the #519 "first perf-lever write after scene load doesn't take + # effect" settle-wait fix: olo_renderer_settings_set now waits out the + # render-budget throttle transient before returning. Same McpHeadlessHost + # infra as McpHeadlessAttachTest.cpp above. + Rendering/PropertyTests/McpPerfSettleTest.cpp Rendering/PropertyTests/VideoOverlayVisualEvidenceTest.cpp Rendering/PropertyTests/AutoExposureEvidenceTest.cpp Rendering/PropertyTests/TestFailureCapture.cpp diff --git a/OloEngine/tests/Rendering/PropertyTests/McpPerfSettleTest.cpp b/OloEngine/tests/Rendering/PropertyTests/McpPerfSettleTest.cpp new file mode 100644 index 000000000..8c95d07f8 --- /dev/null +++ b/OloEngine/tests/Rendering/PropertyTests/McpPerfSettleTest.cpp @@ -0,0 +1,138 @@ +// ============================================================================= +// McpPerfSettleTest.cpp +// +// Pins the #519 "Smaller findings" fix: the first perf-lever write right after +// a heavy scene load (or any render-throttled beat) didn't take effect on the +// GPU — a caller that flipped a renderer setting then immediately read +// olo_perf_snapshot saw stale pre-change data, because RendererProfiler's +// BeginFrame/EndFrame (and so GetLastCompletedFrameData(), what the snapshot +// reads) only run inside Renderer3D::BeginScene, which the editor's render- +// budget throttle skips entirely for a beat after a stall. Handle_ +// RendererSettingsSet (McpTools.cpp) now waits out that transient — the same +// AwaitRenderedFrames/IsCaptureUnready settle discipline the camera-pose +// screenshot tools already use — before returning, so a caller never +// round-trips on the "changed but not yet rendered" window. +// +// This test proves the settle-wait actually executes (not just that the tool +// still returns successfully): olo_renderer_settings_set must pump at least +// `kSettingsSettleFrames` (2) more rendered frames before its response comes +// back, which is only true if AwaitRenderedFrames blocks the response as +// intended. Without the fix the call returns near-instantly (0-1 pumps). +// +// Classification: integration (real Renderer3D bring-up + in-process MCP +// dispatch via McpHeadlessHost). Sibling of McpHeadlessAttachTest. +// ============================================================================= + +// OLO_TEST_LAYER: integration + +#include "OloEnginePCH.h" + +#include "McpHeadlessHost.h" +#include "RenderPropertyTest.h" +#include "RendererAttachedTest.h" + +#include "MCP/McpServer.h" + +#include "OloEngine/Renderer/Camera/EditorCamera.h" +#include "OloEngine/Renderer/Framebuffer.h" +#include "OloEngine/Renderer/Mesh.h" +#include "OloEngine/Renderer/MeshPrimitives.h" +#include "OloEngine/Renderer/Renderer3D.h" +#include "OloEngine/Renderer/ResourceHandle.h" +#include "OloEngine/Scene/Components.h" +#include "OloEngine/Scene/Entity.h" + +#include + +namespace OloEngine::Tests +{ + namespace + { + using Json = OloEngine::MCP::Json; + + constexpr u32 kWidth = 320; + constexpr u32 kHeight = 180; + } // namespace + + // A minimal lit scene, same shape as McpHeadlessAttachTest's fixture — only + // needs SOMETHING rendering so Renderer3D is fully initialized and + // olo_renderer_settings_set has a live graph to act on. + class McpPerfSettleTest : public RendererAttachedTest + { + protected: + void BuildScene() override + { + Scene& scene = GetScene(); + EnableRendering(kWidth, kHeight); + + Entity light = scene.CreateEntity("Sun"); + auto& dl = light.AddComponent(); + dl.m_Direction = glm::normalize(glm::vec3(-0.4f, -0.8f, -0.45f)); + dl.m_Intensity = 3.0f; + + Entity cube = scene.CreateEntity("Cube"); + auto& mc = cube.AddComponent(); + mc.m_Primitive = MeshPrimitive::Cube; + if (Ref mesh = MeshPrimitives::CreateCube()) + mc.m_MeshSource = mesh->GetMeshSource(); + cube.AddComponent(); + } + + [[nodiscard]] EditorCamera MakeCamera() const + { + EditorCamera camera(45.0f, static_cast(kWidth) / static_cast(kHeight), 0.1f, 1000.0f); + camera.SetViewportSize(static_cast(kWidth), static_cast(kHeight)); + camera.SetPose(glm::vec3(0.0f, 1.2f, 4.5f), /*yaw=*/0.0f, /*pitch=*/0.18f); + return camera; + } + }; + + TEST_F(McpPerfSettleTest, RendererSettingsSetSettlesBeforeReturning) + { + OLO_ENSURE_GPU_OR_SKIP(); + + EditorCamera camera = MakeCamera(); + RunEditorFrames(camera, 2); // warm the graph up before hosting + + McpHeadlessHost host(McpHeadlessHost::Hooks{ + .GetScene = [this]() -> Ref + { return GetSceneRef(); }, + .GetCompositeFramebuffer = []() -> Ref + { return Renderer3D::ResolveFrameGraphFramebuffer(ResourceNames::UIComposite); }, + .Camera = &camera, + .RenderWidth = kWidth, + .RenderHeight = kHeight, + .RenderOneFrame = [this, &camera]() + { RunEditorFrames(camera, 1); }, + }); + + const u16 port = host.Start(); + ASSERT_NE(port, 0) << "McpHeadlessHost failed to bind a port for the MCP server"; + + // olo_renderer_settings_set is a consented WRITE tool. + host.Server().SetAllowWrites(true); + + // Get past the host's own frame==0 "unready" special-case before timing + // the settle window, so the measurement below isolates the NEW settle + // logic in Handle_RendererSettingsSet rather than that startup case. + host.PumpOnce(); + host.PumpOnce(); + + const u64 before = host.FrameIndex(); + const Json resp = host.CallTool("olo_renderer_settings_set", Json{ { "setting", "depthprepass" }, { "value", "on" } }); + const u64 after = host.FrameIndex(); + + ASSERT_TRUE(resp.contains("result")) << "olo_renderer_settings_set returned an error: " << resp.dump(2); + EXPECT_FALSE(resp["result"].value("isError", false)) << resp.dump(2); + + // The core regression guard: the call must not return until at least a + // couple more frames have actually rendered with the new setting + // applied — proving AwaitRenderedFrames genuinely blocked the response + // rather than the tool returning as soon as the write landed. + EXPECT_GE(after - before, 2u) + << "olo_renderer_settings_set returned without settling rendered frames (#519 regression: " + "a caller reading olo_perf_snapshot immediately afterward could see stale pre-change data)"; + + host.Stop(); + } +} // namespace OloEngine::Tests