From 9e8ab8b6a186551ea3b4b78b7ddbb2bcd51a85e8 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Wed, 6 May 2026 14:12:03 -0700 Subject: [PATCH 1/8] Throw from Device::UpdateDevice when called while rendering is enabled Before this change, UpdateDevice's effect on a Device whose bgfx is already initialized was silently buffered: the new device pointer was stored in Bgfx.InitState.platformData, but bgfx::init does not run again until the caller does a DisableRendering / EnableRendering cycle, and EnableRendering early-outs while already initialized. The result was a swap that appeared to succeed but in which the device never actually changed -- a footgun that's hard to diagnose. This commit: - Throws std::runtime_error from DeviceImpl::UpdateDevice when called with Bgfx.Initialized == true. The throw message names the required call sequence (DisableRendering first). - Documents the contract on the public Device::UpdateDevice declaration in Device.h, including the canonical D3D11/D3D12 device-removed recovery sequence. - Adds TEST(Device, UpdateDeviceThrowsWhenRenderingEnabled) that exercises the contract: pre-init UpdateDevice is permitted, post-init throws, and the swap completes successfully after DisableRendering. UpdateWindow has the same surface-level contract but is left unchanged in this PR -- the Android Playground's surfaceChanged JNI callback calls UpdateWindow followed by UpdateSize while bgfx is initialized, and on the OpenGL backend bgfx::reset (triggered by UpdateSize) re-creates the EGL surface from the new ANativeWindow*. So the "init-only vs. reset-applicable" classification is more nuanced for UpdateWindow than for UpdateDevice, and narrowing the change to UpdateDevice avoids breaking the Android flow. UpdateBackBuffer, UpdateSize, UpdateMSAA, and UpdateAlphaPremultiplied are left unchanged because they are reset-applicable: bgfx::reset (called from UpdateSize) reads g_platformData.backBuffer and init.resolution.reset, so those updates take effect mid-frame as expected. The existing TEST(Device, BackBuffer) exercises that flow and continues to pass. Local verification: full UnitTests suite on Win32 D3D11 PASSED including the new test; Win32 D3D12 builds and runs (UniformPadding sample passes). [Created by Copilot on behalf of @bghgary] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Apps/UnitTests/Source/Tests.Device.D3D11.cpp | 46 +++++++++++++++++++ .../Include/Shared/Babylon/Graphics/Device.h | 16 +++++++ Core/Graphics/Source/DeviceImpl.cpp | 4 ++ 3 files changed, 66 insertions(+) diff --git a/Apps/UnitTests/Source/Tests.Device.D3D11.cpp b/Apps/UnitTests/Source/Tests.Device.D3D11.cpp index 4168cbdaa..e17cd592e 100644 --- a/Apps/UnitTests/Source/Tests.Device.D3D11.cpp +++ b/Apps/UnitTests/Source/Tests.Device.D3D11.cpp @@ -101,3 +101,49 @@ TEST(Device, BackBuffer) device.FinishRenderingCurrentFrame(); } } + +// Verifies that UpdateDevice throws when called while bgfx is initialized. +// +// Without the throw, UpdateDevice would silently store the new device pointer in the bgfx init +// state without applying it (bgfx::init only runs on the next EnableRendering, and EnableRendering +// early-outs while already initialized). This is a footgun: the caller's swap appears to succeed +// but the device never actually changes. +TEST(Device, UpdateDeviceThrowsWhenRenderingEnabled) +{ + winrt::com_ptr deviceA = CreateDevice(); + winrt::com_ptr deviceB = CreateDevice(); + + constexpr SIZE dimensions{1280, 720}; + RenderTargetTexture rttA{CreateTestRenderTargetTexture(deviceA.get(), dimensions.cx, dimensions.cy)}; + + Babylon::Graphics::Configuration config{}; + config.Device = deviceA.get(); + config.BackBufferColor = rttA.View.get(); + config.Width = dimensions.cx; + config.Height = dimensions.cy; + + Babylon::Graphics::Device device{config}; + + // Before EnableRendering: UpdateDevice should be permitted (init state has not been consumed + // yet). This pattern is used by callers who deferred their device choice. + EXPECT_NO_THROW(device.UpdateDevice(deviceA.get())); + + // After EnableRendering (driven by StartRenderingCurrentFrame): UpdateDevice must throw. + device.StartRenderingCurrentFrame(); + EXPECT_THROW(device.UpdateDevice(deviceB.get()), std::runtime_error); + device.FinishRenderingCurrentFrame(); + + // Still initialized after the frame -- still throws. + EXPECT_THROW(device.UpdateDevice(deviceB.get()), std::runtime_error); + + // After DisableRendering, UpdateDevice is permitted again. The next frame picks up the new device. + device.DisableRendering(); + EXPECT_NO_THROW(device.UpdateDevice(deviceB.get())); + + RenderTargetTexture rttB{CreateTestRenderTargetTexture(deviceB.get(), dimensions.cx, dimensions.cy)}; + device.UpdateBackBuffer(rttB.View.get()); + device.StartRenderingCurrentFrame(); + device.FinishRenderingCurrentFrame(); + + EXPECT_EQ(device.GetPlatformInfo().Device, deviceB.get()); +} diff --git a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h index 8a529cdee..e1144ebaa 100644 --- a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h +++ b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h @@ -112,7 +112,23 @@ namespace Babylon::Graphics // method and structure might change. void UpdateWindow(WindowT window); + + // Sets the underlying graphics device that bgfx is initialized with on the next + // EnableRendering call. + // + // This API is only valid when rendering is disabled. Calling it while rendering is + // enabled throws std::runtime_error: bgfx is already initialized with the previous device + // and the swap would otherwise be silently buffered until the caller happens to do a + // DisableRendering / EnableRendering cycle, leading to bugs that are hard to diagnose. + // + // The typical call sequence to swap to a new device (e.g. on D3D11 / D3D12 device-removed + // recovery) is: + // device.DisableRendering(); + // device.UpdateDevice(newDevice); + // // Optional: device.UpdateBackBuffer(newBackBuffer); on D3D11 if a caller-owned RTV is in use + // device.StartRenderingCurrentFrame(); // implicitly calls EnableRendering with the new device void UpdateDevice(DeviceT device); + void UpdateSize(size_t width, size_t height); void UpdateMSAA(uint8_t value); void UpdateAlphaPremultiplied(bool enabled); diff --git a/Core/Graphics/Source/DeviceImpl.cpp b/Core/Graphics/Source/DeviceImpl.cpp index 93a3fa75e..f199f2193 100644 --- a/Core/Graphics/Source/DeviceImpl.cpp +++ b/Core/Graphics/Source/DeviceImpl.cpp @@ -108,6 +108,10 @@ namespace Babylon::Graphics void DeviceImpl::UpdateDevice(DeviceT device) { std::scoped_lock lock{m_state.Mutex}; + if (m_state.Bgfx.Initialized) + { + throw std::runtime_error{"UpdateDevice called while rendering is enabled. Call DisableRendering first; the new device will take effect on the next EnableRendering / StartRenderingCurrentFrame."}; + } m_state.Bgfx.InitState.platformData.context = device; m_state.Bgfx.Dirty = true; } From c257157c3b51283330c4487c600e16b5a514db93 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Wed, 6 May 2026 15:00:00 -0700 Subject: [PATCH 2/8] Trim UpdateDevice exception message to a simple statement Exception messages should describe what went wrong; how to recover belongs in the API doc-comment (which Device.h already has). Aligns with the simple- statement style of the other DeviceImpl exception messages. [Created by Copilot on behalf of @bghgary] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Core/Graphics/Source/DeviceImpl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/Graphics/Source/DeviceImpl.cpp b/Core/Graphics/Source/DeviceImpl.cpp index f199f2193..30b9b01b1 100644 --- a/Core/Graphics/Source/DeviceImpl.cpp +++ b/Core/Graphics/Source/DeviceImpl.cpp @@ -110,7 +110,7 @@ namespace Babylon::Graphics std::scoped_lock lock{m_state.Mutex}; if (m_state.Bgfx.Initialized) { - throw std::runtime_error{"UpdateDevice called while rendering is enabled. Call DisableRendering first; the new device will take effect on the next EnableRendering / StartRenderingCurrentFrame."}; + throw std::runtime_error{"UpdateDevice called while rendering is enabled."}; } m_state.Bgfx.InitState.platformData.context = device; m_state.Bgfx.Dirty = true; From 5e25e2d52c5a6b45b4496d8752a4e13384242dd2 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Wed, 6 May 2026 15:05:43 -0700 Subject: [PATCH 3/8] Drop bgfx mention from UpdateDevice doc-comment bgfx is an implementation detail not part of the public Device API surface; the doc-comment should describe behavior in terms of the public API (EnableRendering / DisableRendering / etc). [Created by Copilot on behalf of @bghgary] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Include/Shared/Babylon/Graphics/Device.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h index e1144ebaa..2ae3371ef 100644 --- a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h +++ b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h @@ -113,13 +113,13 @@ namespace Babylon::Graphics void UpdateWindow(WindowT window); - // Sets the underlying graphics device that bgfx is initialized with on the next - // EnableRendering call. + // Sets the underlying graphics device used for rendering. The new device takes effect on + // the next EnableRendering call. // - // This API is only valid when rendering is disabled. Calling it while rendering is - // enabled throws std::runtime_error: bgfx is already initialized with the previous device - // and the swap would otherwise be silently buffered until the caller happens to do a - // DisableRendering / EnableRendering cycle, leading to bugs that are hard to diagnose. + // This API is only valid when rendering is disabled. Calling it while rendering is enabled + // throws std::runtime_error: the swap would otherwise be silently buffered until the caller + // happens to do a DisableRendering / EnableRendering cycle, leading to bugs that are hard + // to diagnose. // // The typical call sequence to swap to a new device (e.g. on D3D11 / D3D12 device-removed // recovery) is: From 26e4f18c3ce51281711a24e108ba5c045bc422d0 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Wed, 6 May 2026 15:16:33 -0700 Subject: [PATCH 4/8] Trim UpdateDevice throw-test to the throw contract The post-Disable frame + GetPlatformInfo assertion was demonstrating the canonical UpdateDevice usage sequence, which is what TEST(Device, UpdateDevice) in #1687 does. The throw-test should focus on the throw contract only: - permitted pre-init - throws after EnableRendering - permitted again after DisableRendering [Created by Copilot on behalf of @bghgary] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Apps/UnitTests/Source/Tests.Device.D3D11.cpp | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/Apps/UnitTests/Source/Tests.Device.D3D11.cpp b/Apps/UnitTests/Source/Tests.Device.D3D11.cpp index e17cd592e..358277913 100644 --- a/Apps/UnitTests/Source/Tests.Device.D3D11.cpp +++ b/Apps/UnitTests/Source/Tests.Device.D3D11.cpp @@ -124,26 +124,19 @@ TEST(Device, UpdateDeviceThrowsWhenRenderingEnabled) Babylon::Graphics::Device device{config}; - // Before EnableRendering: UpdateDevice should be permitted (init state has not been consumed - // yet). This pattern is used by callers who deferred their device choice. + // Pre-EnableRendering: UpdateDevice is permitted (init state has not been consumed yet). + // This pattern is used by callers who deferred their device choice. EXPECT_NO_THROW(device.UpdateDevice(deviceA.get())); - // After EnableRendering (driven by StartRenderingCurrentFrame): UpdateDevice must throw. + // After EnableRendering (driven by StartRenderingCurrentFrame): UpdateDevice throws. device.StartRenderingCurrentFrame(); EXPECT_THROW(device.UpdateDevice(deviceB.get()), std::runtime_error); device.FinishRenderingCurrentFrame(); - // Still initialized after the frame -- still throws. + // Still initialized between frames -- still throws. EXPECT_THROW(device.UpdateDevice(deviceB.get()), std::runtime_error); - // After DisableRendering, UpdateDevice is permitted again. The next frame picks up the new device. + // After DisableRendering: UpdateDevice is permitted again. device.DisableRendering(); EXPECT_NO_THROW(device.UpdateDevice(deviceB.get())); - - RenderTargetTexture rttB{CreateTestRenderTargetTexture(deviceB.get(), dimensions.cx, dimensions.cy)}; - device.UpdateBackBuffer(rttB.View.get()); - device.StartRenderingCurrentFrame(); - device.FinishRenderingCurrentFrame(); - - EXPECT_EQ(device.GetPlatformInfo().Device, deviceB.get()); } From 5c0f22ac953e8004ebfc4f9b8e7d74137bd8b2d7 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Wed, 6 May 2026 15:20:01 -0700 Subject: [PATCH 5/8] Pare UpdateDevice throw test down to the contract Drops: - second ID3D11Device (only the throw is being tested; same pointer suffices) - caller-owned BackBuffer setup (use g_deviceConfig HWND, let bgfx own swap chain) - between-frames EXPECT_THROW (redundant with the post-Start one) - over-explanatory comments Net ~30 lines removed. [Created by Copilot on behalf of @bghgary] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Apps/UnitTests/Source/Tests.Device.D3D11.cpp | 38 +++++++------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/Apps/UnitTests/Source/Tests.Device.D3D11.cpp b/Apps/UnitTests/Source/Tests.Device.D3D11.cpp index 358277913..ae573e6b4 100644 --- a/Apps/UnitTests/Source/Tests.Device.D3D11.cpp +++ b/Apps/UnitTests/Source/Tests.Device.D3D11.cpp @@ -6,6 +6,8 @@ #include +extern Babylon::Graphics::Configuration g_deviceConfig; + namespace { winrt::com_ptr CreateDevice() @@ -102,41 +104,25 @@ TEST(Device, BackBuffer) } } -// Verifies that UpdateDevice throws when called while bgfx is initialized. -// -// Without the throw, UpdateDevice would silently store the new device pointer in the bgfx init -// state without applying it (bgfx::init only runs on the next EnableRendering, and EnableRendering -// early-outs while already initialized). This is a footgun: the caller's swap appears to succeed -// but the device never actually changes. +// Verifies that UpdateDevice throws when called while rendering is enabled. TEST(Device, UpdateDeviceThrowsWhenRenderingEnabled) { - winrt::com_ptr deviceA = CreateDevice(); - winrt::com_ptr deviceB = CreateDevice(); - - constexpr SIZE dimensions{1280, 720}; - RenderTargetTexture rttA{CreateTestRenderTargetTexture(deviceA.get(), dimensions.cx, dimensions.cy)}; + winrt::com_ptr d3dDevice = CreateDevice(); - Babylon::Graphics::Configuration config{}; - config.Device = deviceA.get(); - config.BackBufferColor = rttA.View.get(); - config.Width = dimensions.cx; - config.Height = dimensions.cy; + Babylon::Graphics::Configuration config = g_deviceConfig; + config.Device = d3dDevice.get(); Babylon::Graphics::Device device{config}; - // Pre-EnableRendering: UpdateDevice is permitted (init state has not been consumed yet). - // This pattern is used by callers who deferred their device choice. - EXPECT_NO_THROW(device.UpdateDevice(deviceA.get())); + // Permitted before EnableRendering. + EXPECT_NO_THROW(device.UpdateDevice(d3dDevice.get())); - // After EnableRendering (driven by StartRenderingCurrentFrame): UpdateDevice throws. + // StartRenderingCurrentFrame triggers EnableRendering -> throws. device.StartRenderingCurrentFrame(); - EXPECT_THROW(device.UpdateDevice(deviceB.get()), std::runtime_error); + EXPECT_THROW(device.UpdateDevice(d3dDevice.get()), std::runtime_error); device.FinishRenderingCurrentFrame(); - // Still initialized between frames -- still throws. - EXPECT_THROW(device.UpdateDevice(deviceB.get()), std::runtime_error); - - // After DisableRendering: UpdateDevice is permitted again. + // Permitted again after DisableRendering. device.DisableRendering(); - EXPECT_NO_THROW(device.UpdateDevice(deviceB.get())); + EXPECT_NO_THROW(device.UpdateDevice(d3dDevice.get())); } From 2079f3d69ca2c86caea9a0766a6c0a132a03b3ee Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Wed, 6 May 2026 15:21:48 -0700 Subject: [PATCH 6/8] Trim implementation rationale from UpdateDevice doc-comment The doc is for the API consumer; rationale for why the throw exists belongs in the commit message / PR body, not the public header. [Created by Copilot on behalf of @bghgary] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Core/Graphics/Include/Shared/Babylon/Graphics/Device.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h index 2ae3371ef..15d0b30e7 100644 --- a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h +++ b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h @@ -117,9 +117,7 @@ namespace Babylon::Graphics // the next EnableRendering call. // // This API is only valid when rendering is disabled. Calling it while rendering is enabled - // throws std::runtime_error: the swap would otherwise be silently buffered until the caller - // happens to do a DisableRendering / EnableRendering cycle, leading to bugs that are hard - // to diagnose. + // throws std::runtime_error. // // The typical call sequence to swap to a new device (e.g. on D3D11 / D3D12 device-removed // recovery) is: From 6c9354209a0652b7b1a671fd193a70916f9a1cdc Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Wed, 6 May 2026 15:36:25 -0700 Subject: [PATCH 7/8] Drop typical-call-sequence example from UpdateDevice doc-comment Doc shouldn't presume the user app's call pattern. The throw contract is sufficient. [Created by Copilot on behalf of @bghgary] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Core/Graphics/Include/Shared/Babylon/Graphics/Device.h | 7 ------- 1 file changed, 7 deletions(-) diff --git a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h index 15d0b30e7..a3847af33 100644 --- a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h +++ b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h @@ -118,13 +118,6 @@ namespace Babylon::Graphics // // This API is only valid when rendering is disabled. Calling it while rendering is enabled // throws std::runtime_error. - // - // The typical call sequence to swap to a new device (e.g. on D3D11 / D3D12 device-removed - // recovery) is: - // device.DisableRendering(); - // device.UpdateDevice(newDevice); - // // Optional: device.UpdateBackBuffer(newBackBuffer); on D3D11 if a caller-owned RTV is in use - // device.StartRenderingCurrentFrame(); // implicitly calls EnableRendering with the new device void UpdateDevice(DeviceT device); void UpdateSize(size_t width, size_t height); From 577660dc1164ff8be84088bede9a84c34dfa8d6b Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Fri, 8 May 2026 08:54:41 -0700 Subject: [PATCH 8/8] Clarify UpdateDevice doc-comment lifecycle states Reviewer flagged "only valid when rendering is disabled" as ambiguous (rendering can be "not in a frame" while still initialized). Make the contract explicit by naming the lifecycle states: before first EnableRendering / StartRenderingCurrentFrame, or after DisableRendering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Core/Graphics/Include/Shared/Babylon/Graphics/Device.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h index a3847af33..6374161d3 100644 --- a/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h +++ b/Core/Graphics/Include/Shared/Babylon/Graphics/Device.h @@ -116,8 +116,9 @@ namespace Babylon::Graphics // Sets the underlying graphics device used for rendering. The new device takes effect on // the next EnableRendering call. // - // This API is only valid when rendering is disabled. Calling it while rendering is enabled - // throws std::runtime_error. + // Only valid when rendering is disabled -- i.e. before the first EnableRendering / + // StartRenderingCurrentFrame call, or after a DisableRendering call. Throws + // std::runtime_error otherwise. void UpdateDevice(DeviceT device); void UpdateSize(size_t width, size_t height);