Skip to content

Commit 9e8ab8b

Browse files
bghgaryCopilot
andcommitted
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>
1 parent 58f8f14 commit 9e8ab8b

3 files changed

Lines changed: 66 additions & 0 deletions

File tree

Apps/UnitTests/Source/Tests.Device.D3D11.cpp

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,49 @@ TEST(Device, BackBuffer)
101101
device.FinishRenderingCurrentFrame();
102102
}
103103
}
104+
105+
// Verifies that UpdateDevice throws when called while bgfx is initialized.
106+
//
107+
// Without the throw, UpdateDevice would silently store the new device pointer in the bgfx init
108+
// state without applying it (bgfx::init only runs on the next EnableRendering, and EnableRendering
109+
// early-outs while already initialized). This is a footgun: the caller's swap appears to succeed
110+
// but the device never actually changes.
111+
TEST(Device, UpdateDeviceThrowsWhenRenderingEnabled)
112+
{
113+
winrt::com_ptr<ID3D11Device> deviceA = CreateDevice();
114+
winrt::com_ptr<ID3D11Device> deviceB = CreateDevice();
115+
116+
constexpr SIZE dimensions{1280, 720};
117+
RenderTargetTexture rttA{CreateTestRenderTargetTexture(deviceA.get(), dimensions.cx, dimensions.cy)};
118+
119+
Babylon::Graphics::Configuration config{};
120+
config.Device = deviceA.get();
121+
config.BackBufferColor = rttA.View.get();
122+
config.Width = dimensions.cx;
123+
config.Height = dimensions.cy;
124+
125+
Babylon::Graphics::Device device{config};
126+
127+
// Before EnableRendering: UpdateDevice should be permitted (init state has not been consumed
128+
// yet). This pattern is used by callers who deferred their device choice.
129+
EXPECT_NO_THROW(device.UpdateDevice(deviceA.get()));
130+
131+
// After EnableRendering (driven by StartRenderingCurrentFrame): UpdateDevice must throw.
132+
device.StartRenderingCurrentFrame();
133+
EXPECT_THROW(device.UpdateDevice(deviceB.get()), std::runtime_error);
134+
device.FinishRenderingCurrentFrame();
135+
136+
// Still initialized after the frame -- still throws.
137+
EXPECT_THROW(device.UpdateDevice(deviceB.get()), std::runtime_error);
138+
139+
// After DisableRendering, UpdateDevice is permitted again. The next frame picks up the new device.
140+
device.DisableRendering();
141+
EXPECT_NO_THROW(device.UpdateDevice(deviceB.get()));
142+
143+
RenderTargetTexture rttB{CreateTestRenderTargetTexture(deviceB.get(), dimensions.cx, dimensions.cy)};
144+
device.UpdateBackBuffer(rttB.View.get());
145+
device.StartRenderingCurrentFrame();
146+
device.FinishRenderingCurrentFrame();
147+
148+
EXPECT_EQ(device.GetPlatformInfo().Device, deviceB.get());
149+
}

Core/Graphics/Include/Shared/Babylon/Graphics/Device.h

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,23 @@ namespace Babylon::Graphics
112112
// method and structure might change.
113113

114114
void UpdateWindow(WindowT window);
115+
116+
// Sets the underlying graphics device that bgfx is initialized with on the next
117+
// EnableRendering call.
118+
//
119+
// This API is only valid when rendering is disabled. Calling it while rendering is
120+
// enabled throws std::runtime_error: bgfx is already initialized with the previous device
121+
// and the swap would otherwise be silently buffered until the caller happens to do a
122+
// DisableRendering / EnableRendering cycle, leading to bugs that are hard to diagnose.
123+
//
124+
// The typical call sequence to swap to a new device (e.g. on D3D11 / D3D12 device-removed
125+
// recovery) is:
126+
// device.DisableRendering();
127+
// device.UpdateDevice(newDevice);
128+
// // Optional: device.UpdateBackBuffer(newBackBuffer); on D3D11 if a caller-owned RTV is in use
129+
// device.StartRenderingCurrentFrame(); // implicitly calls EnableRendering with the new device
115130
void UpdateDevice(DeviceT device);
131+
116132
void UpdateSize(size_t width, size_t height);
117133
void UpdateMSAA(uint8_t value);
118134
void UpdateAlphaPremultiplied(bool enabled);

Core/Graphics/Source/DeviceImpl.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,10 @@ namespace Babylon::Graphics
108108
void DeviceImpl::UpdateDevice(DeviceT device)
109109
{
110110
std::scoped_lock lock{m_state.Mutex};
111+
if (m_state.Bgfx.Initialized)
112+
{
113+
throw std::runtime_error{"UpdateDevice called while rendering is enabled. Call DisableRendering first; the new device will take effect on the next EnableRendering / StartRenderingCurrentFrame."};
114+
}
111115
m_state.Bgfx.InitState.platformData.context = device;
112116
m_state.Bgfx.Dirty = true;
113117
}

0 commit comments

Comments
 (0)