bugfix(dx8): Unbind device resources before Reset

[tintinhamans]

Reset() was called while the device could still hold the last draw's buffers, textures and render target, because Set_Vertex_Buffer and Set_Index_Buffer are deferred. Reset_Device now unbinds the streams and indices on the device through a new Unbind_Device_Buffers(), which Release_Device uses too, and invalidates the cached render states and resets the render target before Reset(). A failed Reset() no longer asserts in debug.

To reproduce, leave a render target bound and force a device loss. On main Reset() then fails on every frame and never recovers, on both NVIDIA and Intel. With this change it resets on the first try.

For context, Generals Online's crash reports show this as an access violation in DestroyResource, which is not a game function. The game frames end at DX8Wrapper::Reset_Device. Below it, Reset() in d3d8.dll has the display driver destroy the device's resources, including the ones this change now unbinds first. Intel's igd9trinity32.dll is on the crashing stack in 279 of 281 reports of the last 90 days, and in 567 of 570 for a release that only released the render target before Reset(). Their Sentry has about 8,800 of these since December 2025. The same change is in GeneralsOnlineDevelopmentTeam#634.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 74f57d2b-0b99-4c1f-b708-dfeed009532f

📥 Commits

Reviewing files that changed from the base of the PR and between 3489fb3 and c6bab1f.

📒 Files selected for processing: 2

• Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp
• Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

Walkthrough

DX8Wrapper now tracks reset failures and uses that state to control device-resource operations. Reset handling checks device availability, updates the failure state from the reset result, and restores assets and render state after success.

Changes

DX8 device reset handling

Layer / File(s)	Summary
Reset failure state Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.h, Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp	Adds and initializes DeviceResetFailed, clears it before device creation, and skips SetTexture during cached-state invalidation when the device is absent or reset has failed.
Reset and resource handling Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.h, Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp	Reset_Device checks wrapper and device availability and handles reset results. Unbind_Device_Buffers unbinds every vertex stream and the index buffer. Release_Device skips texture and buffer unbinding after a failed reset.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mauller

Merge Risk: ⚪ Minimal · up to c6bab

The inspected reset-retry path does not show the previously suspected device-call failure. The change is ready for normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: unbinding DX8 device resources before Reset.
Description check		The description directly explains the reset failure, the implemented fix, reproduction details, and reported crash context. It is relevant to the changeset.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new blocking issues were found.

Summary

The PR releases device bindings before Reset and tracks failed reset attempts.

• DX8 resets release held draw resources before resetting the device.

No new actionable issues were found. The earlier unnumbered finding is addressed by releasing resources on every retry.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Reset_Device] --> B[Release resources]
  B --> C{Device still lost?}
  C -->|Yes| D[Return false]
  C -->|No| E[Unbind buffers unless prior reset failed]
  E --> F[Clear cached states and saved targets]
  F --> G[Reset]
  G -->|Failure| H[Record failure and return false]
  G -->|Success| I[Restore assets and default states]

Loading

_{Reviews (11) · Last reviewed commit: "bugfix(dx8): Unbind device resources bef..." · Reviewed by Greptile}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: ba7740ea-2f70-4b9c-91b2-34285ac708e6

📥 Commits

Reviewing files that changed from the base of the PR and between 2119cc9 and 316e960.

📒 Files selected for processing (2)

• Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp
• Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

[bobtista]

ok sweet so device-loss recovery can succeed, and retries after a failed Reset stop touching the device. Comments and variable names seem fine. LGTM

[xezon]

Very confusing.

[xezon]

Is this a hack? The EA comment is confusing too.

[tintinhamans]

Not meant as a hack. After a failed Reset() the device should only get TestCooperativeLevel, Reset or Release, so I skip the SetTexture there. Added a note on DeviceResetFailed.

[Mauller]

Basically the device is not in a state to make use of certain functions and only a small subset of the API can be called on it at this point.

[tintinhamans]

the flag is gone now, see the second commit

[tintinhamans]

It’s back. I tested a Reset() that keeps failing: after a failed Reset() the next attempt crashes in SetStreamSource even though TestCooperativeLevel says the device can be reset. So the device isn’t touched until a Reset() succeeds, everything else is still released every attempt.

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: b3984891-d68e-408c-9564-ff7054eb7204

📥 Commits

Reviewing files that changed from the base of the PR and between 338fa02 and c2678ec.

📒 Files selected for processing (2)

• Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp
• Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

[xezon]

I do not understand this comment.

[tintinhamans]

Reworded for clarity - device and wrapper still hold references on textures and buffers of the last draw and Reset() fails while those are there

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9af5d55f-cbe8-44ab-9755-3007f0c0ebe3

📥 Commits

Reviewing files that changed from the base of the PR and between c2678ec and 3670989.

📒 Files selected for processing: 1

• Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

[Mauller]

LGTM

[xezon]

I gave this to Claude to Review. Here are some findings:

Review

Unbinding the device's textures, streams and index buffer before Reset() addresses the real cause. Set_Vertex_Buffer(nullptr) and Set_Index_Buffer(nullptr) only update the wrapper. The device keeps its own reference to the last-drawn (default-pool) buffers, so Reset() fails. The pre-reset Invalidate_Cached_Render_States() and Set_Render_Target(nullptr) also correctly drop the wrapper's own references to textures and render targets.

My concerns are about the new DeviceResetFailed flag. Findings 1–4 all come from it. I think dropping the flag and always unbinding is both simpler and more robust. Findings 6 and 7 predate this PR and don't block it.

---

Findings in this change

1. A retry after a failed Reset no longer unbinds anything

Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp line 670 (Unbind_Device_Resources), together with line 435 (Invalidate_Cached_Render_States).

After one failed Reset(), DeviceResetFailed stays true until a Reset succeeds. On the next Reset_Device() call, Unbind_Device_Resources() returns early and Invalidate_Cached_Render_States() skips SetTexture(a, nullptr). The retry therefore depends on nothing having been bound on the device since the failure.

That isn't guaranteed. GUIEdit's EditWindow::draw ignores the result of WW3D::Begin_Render and still runs winRepaint(). That binds a default-pool dynamic VB/IB and textures through Apply_Render_State_Changes(), and a lost device accepts these calls. Then:

1. End_Render → End_Scene → Present returns D3DERR_DEVICELOST.
2. TestCooperativeLevel returns D3DERR_DEVICENOTRESET, so Reset_Device() runs again.
3. DynamicVBAccessClass::_Deinit() drops the wrapper's reference, but the unbind is skipped, so the device still holds the dynamic VB.
4. Reset() fails again, the flag stays set, and every later retry repeats this. The device never recovers.

WorldBuilder's ObjectPreview also renders without checking Begin_Render. In the game itself, the draw paths are mostly guarded by TestCooperativeLevel, so this is less likely there, but the fix shouldn't depend on that.

Suggestion: always unbind before Reset(). That call is exactly what makes Reset() succeed, so it shouldn't depend on an earlier failure.

2. While the flag is set, the texture cache stops matching the device

dx8wrapper.cpp line 435.

Invalidate_Cached_Render_States() is public, and game code calls it mid-frame (W3DBridgeBuffer.cpp:1195, W3DVolumetricShadow.cpp:3659 and :3677). While DeviceResetFailed is set, it now sets Textures[a] = nullptr without calling SetTexture(a, nullptr) on the device. The next Set_DX8_Texture(a, nullptr) is then filtered out as redundant (dx8wrapper.h:913), so the old texture stays bound. That is the exact problem MW's comment above this line warns about.

This only matters if the device can render while the flag is set. One way: Reset() is rejected (for example for invalid present parameters) but TestCooperativeLevel still returns D3D_OK, and the caller ignores the failure (Set_Swap_Interval does). I haven't confirmed that the D3D8 runtime behaves this way. Still, the flag is sticky, and only a successful Reset or Create_Device() clears it, so any such state lasts the rest of the session.

3. The flag's documented rule is only checked in two places

dx8wrapper.h line 638.

The comment says that after a failed Reset, "the device only accepts TestCooperativeLevel, Reset and Release". But only Unbind_Device_Resources() and one call in Invalidate_Cached_Render_States() check the flag. The same failed-state paths keep calling other device methods:

• Set_Render_Target(nullptr) in the same pre-reset block (dx8wrapper.cpp:640) calls SetRenderTarget whenever DefaultRenderTarget is held.
• Shutdown() calls Set_Render_Target(nullptr) before Release_Device().
• m_pCleanupHook->ReleaseResources() → W3DShaderManager::shutdown() calls DeletePixelShader before the flag is ever checked.
• Tool render paths (see 1) keep issuing draw calls.

So either calls on a failed or lost device are harmless (lost-device behaviour: they succeed and do nothing), and the flag isn't needed, or they aren't harmless, and the flag would have to cover far more than two sites. I'd go with the first and remove the flag, which also fixes 1, 2 and 4.

Question: was the flag added because the debug build asserted in Unbind_Device_Resources() (DX8CALL → Log_DX8_ErrorCode → WWASSERT(0)) after a failed Reset? If so, I'd rather fix it at the reporting level (see 6) than skip the unbind.

4. Unbind_Device_Resources silently does nothing after a failed Reset

dx8wrapper.cpp line 668.

The function name and its comment promise to unbind textures, streams and indices, but it returns early depending on global state. At both call sites, Reset_Device() and Release_Device() look like they always unbind, which hides the problem in 1. If some form of the flag stays, please check it in the caller that needs it, Release_Device(), so the helper does what its name says.

5. Texture stages are unbound twice before Reset

dx8wrapper.cpp lines 638–639.

Before Reset(), Unbind_Device_Resources() calls SetTexture(stage, nullptr) for all 8 stages, then Invalidate_Cached_Render_States() does the same for every stage again. That's 8 redundant calls per Reset, and two texture-unbind loops that have to be kept in sync.

Suggestion: have Unbind_Device_Resources() handle only streams and indices, and use Invalidate_Cached_Render_States() for textures, since it also clears the wrapper's Textures[] cache:

// Unbinds streams and indices on the device. Set_Vertex_Buffer and Set_Index_Buffer only update the wrapper until the next draw.
void DX8Wrapper::Unbind_Device_Resources()
{
	for (unsigned stream = 0; stream < MAX_VERTEX_STREAMS; ++stream) {
		DX8CALL(SetStreamSource(stream, nullptr, 0));
	}
	DX8CALL(SetIndices(nullptr, 0));
}

		HRESULT hr=_Get_D3D_Device8()->TestCooperativeLevel();
		if (hr != D3DERR_DEVICELOST )
		{
			// TheSuperHackers @bugfix arcticdolphin 09/10/2026 Release everything the device and the wrapper still reference.
			// Reset() fails while default pool resources are alive.
			Unbind_Device_Resources();
			Invalidate_Cached_Render_States();
			Set_Render_Target((IDirect3DSurface8*)nullptr);
			DX8CALL_HRES(Reset(&_PresentParameters),hr)
			if (hr != D3D_OK)
				return false;	//reset failed.
		}

Release_Device() would then need to unbind textures itself, either by keeping its own SetTexture loop or by calling Invalidate_Cached_Render_States(), which also fixes 7.

---

Pre-existing issues (not caused by this PR, fine as follow-ups)

6. A failed Reset asserts in debug builds

dx8wrapper.cpp line 641.

DX8CALL_HRES(Reset(...), hr) passes a failing result to DX8_ErrorCode(), and Log_DX8_ErrorCode() hits WWASSERT(0) in WWDEBUG builds. WW3D::Begin_Render retries Reset_Device() on every frame that sees D3DERR_DEVICENOTRESET, so a persistent failure asserts every frame, and the recovery path can't really be tested in a debug build. This was already the case before the PR, but the PR now treats a failed Reset as an expected, recoverable state. A raw call with Non_Fatal_Log_DX8_ErrorCode() or WWDEBUG_SAY would fit better.

7. The Textures[] cache is never released at shutdown

dx8wrapper.cpp line 686 (Release_Device), together with line 344 (Shutdown).

Shutdown() releases Textures[i] only inside if (CurrentCaps). But it calls Release_Device() first, which reaches Do_Onetime_Device_Dependent_Shutdowns(), which deletes CurrentCaps and sets it to nullptr. So the loop never runs. The textures Set_DX8_Texture() AddRef'd into Textures[] keep their references after D3DDevice->Release(), and the D3D debug runtime will report them as leaked. Unbind_Device_Resources() clears only the device side. This predates the PR, and Release_Device() is the natural place for the wrapper-side release (see the suggestion in 5).

[tintinhamans]

@xezon

I removed the flag and added a second commit that releases everything once per device loss and stops drawing while the device is lost, which covers 1 and 3. A failed Reset no longer asserts, and the texture leak is in a separate PR: #3543. Kept the texture unbind in Unbind_Device_Resources so the reset doesn’t rely on a side effect of Invalidate_Cached_Render_States.

[xezon]

Claude says:

Review (round 2)

Thanks for the update. Most of the first round is resolved:

• ✅ DeviceResetFailed is gone, and Unbind_Device_Resources() now runs before every Reset(), so a retry no longer depends on nothing having been bound since the last failure.
• ✅ Invalidate_Cached_Render_States() is back to an unconditional SetTexture(a, nullptr), so the texture cache can't drift from the device any more.
• ✅ The helper no longer hides a skip condition.
• ✅ A failed Reset() now logs through Non_Fatal_Log_DX8_ErrorCode() and no longer asserts in debug builds.

The new release-once logic introduces a regression (1). Most of the new DeviceState machinery seems unnecessary now that the unbind always runs (2–4).

---

Findings in this change

1. Default-pool resources are released only once per reset cycle

Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp line 615.

Because of ResourcesReleasedForReset, Release_Reset_Resources() runs only on the first attempt. That function covers DX8TextureManagerClass::Release_Textures(), m_pCleanupHook->ReleaseResources() and WW3D::_Invalidate_Textures(). Any D3DPOOL_DEFAULT resource created between attempts is therefore never released, and every later Reset() fails.

One way this happens:

1. Reset_Device() releases everything and sets the flag. Then either Reset() fails (for example D3DERR_OUTOFVIDEOMEMORY, or a default-pool resource this fix still misses), or TestCooperativeLevel() returns D3DERR_DEVICELOST at line 621. The latter can happen when Set_Device_Resolution, Set_Swap_Interval or Set_Render_Device calls Reset_Device() directly, or when the user alt-tabs again between the check in Begin_Render and the one in Reset_Device.
2. Game logic keeps running. A map load runs W3DShroud::init() → ReAcquireResources() → new TextureClass(..., POOL_DEFAULT), and DX8TextureManagerClass tracks every such texture. Water and projected-shadow render targets and the cleanup hook's dynamic buffers are recreated in similar ways.
3. On the next attempt the flag skips Release_Textures() and the cleanup hook. The new D3D texture stays alive and Reset() fails again. The flag stays true, so the device never recovers.

Before this change, both releases ran on every attempt, and they are safe to repeat:

• TextureTrackerClass::Release() just sets the D3D texture to nullptr.
• The shader shutdown() functions zero their handles.
• The hooks use REF_PTR_RELEASE.

Suggestion: release everything on every attempt and drop ResourcesReleasedForReset. Then Release_Dynamic_Buffers() doesn't need to be a separate step either:

	if ((IsInitted) && (D3DDevice != nullptr)) {
		// Release all non-MANAGED stuff
		Release_Reset_Resources();	// includes the dynamic buffers

		HRESULT hr=_Get_D3D_Device8()->TestCooperativeLevel();
		if (hr == D3DERR_DEVICELOST) {
			return false;	//device is lost and can't be reset.
		}

		// TheSuperHackers @bugfix arcticdolphin 09/10/2026 Reset() fails while resources are still bound
		Unbind_Device_Resources();
		Invalidate_Cached_Render_States();
		Set_Render_Target((IDirect3DSurface8*)nullptr);

		hr = _Get_D3D_Device8()->Reset(&_PresentParameters);
		Increment_DX8_CallCount();
		if (hr != D3D_OK)
		{
			Non_Fatal_Log_DX8_ErrorCode(hr,__FILE__,__LINE__);
			return false;	//reset failed.
		}

		Restore_Reset_Resources(reload_assets);
		WWDEBUG_SAY(("Device reset completed"));
		return true;
	}

If the concern was the per-frame cost of WW3D::_Invalidate_Textures() while retrying, it would be fine to run only that part once. It only affects managed textures, and those don't block Reset().

2. The draw check covers only one of many draw paths

dx8wrapper.cpp line 2076.

// Skip draws on a lost device holds only for DX8Wrapper::Draw(). Many draw paths bypass it:

• Draw_Sorting_IB_VB() (dx8wrapper.cpp:2052), which draws all sorted (translucent) geometry.
• Direct DrawIndexedPrimitive / DrawPrimitive calls in W3DWater, W3DVolumetricShadow, W3DProjectedShadow, W3DShaderManager, W3DSmudge, W3DSnow and W3DScene.

As far as I know, D3D already treats draws on a lost device as no-ops. Any bindings those draws leave behind are now cleared by the unconditional unbind before Reset(). So this check doesn't make Reset() work. It adds a special case to the hottest call in the wrapper and promises something the code doesn't deliver.

Suggestion: remove the check. If you hit a concrete problem without it (an assert or a crash), please describe it, so we can fix it where every draw passes through.

3. Release_Device() stops unbinding on a lost device

dx8wrapper.cpp line 730.

The // Not safe on a lost device claim isn't backed up, and it contradicts the rest of the PR: Reset_Device() unbinds while the device is in D3DERR_DEVICENOTRESET. Retail always unbound here, and D3DDevice->Release() follows right after, so skipping the unbind gains nothing. It does:

• change retail behaviour without a TheSuperHackers comment, and
• add a TestCooperativeLevel() call that also updates DeviceState to a teardown path.

Suggestion: restore the unconditional Unbind_Device_Resources(). If you saw a specific failure here, please document it in the comment.

4. The DeviceState machinery only feeds the draw check

dx8wrapper.h line 642.

The following exist only for the check in 2, since Is_Device_Lost() has no callers:

• the DeviceStateType enum and DeviceState
• Test_Cooperative_Level() and Update_Device_State()
• the rewritten call sites in End_Scene, Flip_To_Primary and Release_Device
• the ww3d.cpp change

It is also incomplete: W3DDisplay::draw (W3DDisplay.cpp:1978) and W3DShroud::render (W3DShroud.cpp:533) still call TestCooperativeLevel() directly, so there are two sources of truth for the device state.

Suggestion: if 2 goes, revert all of this to the original IsDeviceLost handling, or drop it entirely, since nothing reads it.

5. The new comments in Reset_Device() describe the wrong mechanism

dx8wrapper.cpp lines 613 and 630.

• // Draws can recreate these between attempts: with the draw check, draws are skipped while the device is not OK. The dynamic VB and IB are recreated when frame code constructs DynamicVBAccessClass and DynamicIBAccessClass, not by Draw(). The comment also suggests that dynamic buffers are the only thing recreated between attempts, which is how 1 slipped through.
• // Retried next frame: that's true only for the Begin_Render, End_Scene and Flip_To_Primary paths. Set_Device_Resolution, Set_Swap_Interval and Set_Render_Device return the failure to their caller, and W3DDisplay::setDisplayMode() immediately retries with the old mode.

6. Texture stages are still unbound twice before Reset()

dx8wrapper.cpp lines 626–627.

This carries over from the last round. Unbind_Device_Resources() calls SetTexture(stage, nullptr) for all 8 stages, then Invalidate_Cached_Render_States() does it again. While a reset keeps failing, that now happens once per frame.

Suggestion: have Unbind_Device_Resources() handle only streams and indices, and use Invalidate_Cached_Render_States() for textures. In Release_Device(), that would also fix 7.

---

Pre-existing issue (not caused by this PR, fine as a follow-up)

7. The Textures[] cache is never released at shutdown

dx8wrapper.cpp line 725 (Release_Device), together with line 344 (Shutdown).

Shutdown() releases Textures[i] only inside if (CurrentCaps). But Release_Device() → Do_Onetime_Device_Dependent_Shutdowns() has already deleted CurrentCaps and set it to nullptr, so the loop never runs. The textures Set_DX8_Texture() AddRef'd into Textures[] outlive D3DDevice->Release(), and the D3D debug runtime reports them as leaked.

[xezon]

While essentially rewriting the whole function, might as well make this an early return here.

[tintinhamans]

Done.

[xezon]

Why does this call a Invalidate function on Restore? Maybe it should be outside of this function and documented why it is called if it is not obvious?

[tintinhamans]

The release and restore helpers are gone, so the invalidate is back where it was, right after Reset().

[tintinhamans]

This is back to almost the first version. Everything is released on every attempt again like retail, and the release-once logic, device state and draw check are gone. I did keep the flag: I tested a Reset() that keeps failing, and the next attempt after a failed Reset() crashes in SetStreamSource even though TestCooperativeLevel reports the device as resettable, the same crash as CLIENT-55X in GO.

With the flag the device isn't touched until a Reset() succeeds, and it recovered after 9 failed attempts with new default pool textures created in between, in the game and in GUIEdit.

A failed Reset no longer asserts, and textures are only unbound once since Unbind_Device_Buffers only handles streams and indices.

Skipping rendering in the tools when Begin_Render fails is in #3548, and the texture cache leak was fixed in #3543.