You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
bugfix(dx8): Render the scene into an A8R8G8B8 target when the fullscreen back buffer uses the display format - #3657
#3656 uses the display format (X8R8G8B8) for the fullscreen back buffer when a device reset does not last until a frame is presented. That back buffer has no destination alpha, so on affected systems the shoreline water effect is off for the rest of the session: renderShoreLinesSorted writes the water depth into the alpha channel of the back buffer, and W3DWater blends standing water with D3DBLEND_DESTALPHA. Without it, shallow water at the shore is opaque and the edge of the water mesh shows above the surf.
Change
When the fullscreen back buffer uses the display format, DX8Wrapper renders the scene into an A8R8G8B8 render target texture and draws it onto the back buffer with a full-screen quad before Present(). A quad is needed because CopyRects does not convert A8R8G8B8 to X8R8G8B8.
With multisampling, the scene target is multisampled instead of the back buffer. The scene is rendered into a multisampled A8R8G8B8 surface with its own depth buffer, which CopyRects resolves into the texture before the quad is drawn. The back buffer only receives the finished frame, so it is not multisampled. This is decided before Reset() in Prepare_Scene_Alpha_Target, because it changes the presentation parameters.
The target stays the device's render target for the whole frame, so code that restores the previous render target (water reflection, texture projectors, screen filters) returns to it. It is released before Reset() and created again after it.
getBackBufferFormat() reports A8R8G8B8 while the target exists, so the existing destination alpha checks are unchanged.
Heat haze (W3DSmudge) and the profiler frame capture copy the scene in the middle of the frame, so they read the new _Get_DX8_Scene_Surface() instead of the back buffer.
All device states are captured in a state block before the copy and restored after it.
Nothing changes in windowed mode, or in fullscreen when the fallback from #3656 does not trigger.
Cost
Only when the fallback is active, at 2560x1440:
No MSAA
4x MSAA
Extra video memory
15 MB
44 MB
Extra GPU time per frame on an RTX 5070
0.018 ms
0.074 ms
The memory is 4 bytes per pixel without MSAA, and 12 bytes per pixel with MSAA at any sample count, because the back buffer and its depth buffer are no longer multisampled. The GPU times were measured with a standalone D3D8 test using random image data, and are probably best case because the GPU cache holds the whole frame.
Screenshots
Shoreline crop of the same frame of the default shell map in three windowed runs at 2560x1440. The X8R8G8B8 runs use a temporary local switch that is not part of this change.
Testing
On the system that reproduces #3049 (Windows 11 26200, RTX 5070 driver 32.0.16.1742, two 2560x1440 @ 144 Hz monitors), Zero Hour:
Windowed, default shell map at 2560x1440, with a temporary local switch that forces an X8R8G8B8 back buffer and the scene target: the back buffer equals the scene target in 10 of 10 captures, including after a forced reset. Compared with the original A8R8G8B8 back buffer in the matching frame, 467 pixels differ, all inside the on-screen clock. With the X8R8G8B8 back buffer and no scene target (bugfix(dx8): Use the display format for the fullscreen back buffer when a device reset does not last #3656 alone), the standing water at the shore is opaque.
Fullscreen with 4x MSAA: the back buffer is not multisampled, the scene target is 4x multisampled with its own depth buffer, and the captures match. The picture, including the shoreline and anti-aliased edges, looks the same as the original.
An earlier revision of this change kept the back buffer multisampled. Device loss and alt-tab with 4x MSAA were tested with that revision: both reset and frames resumed.
Not tested:
Generals (it builds and uses the same code).
Device loss and alt-tab with MSAA on this revision.
Screen filters, which are only reachable from demo commands.
Heat haze with visible smudges.
Systems where the fallback does not trigger (this code is not reached).
AI Disclosure
Claude Opus 5.5 wrote this PR entirely: the code, the investigation and test tooling, and this description. I directed the work.
…en a device reset does not last
In fullscreen the back buffer is promoted to D3DFMT_A8R8G8B8 while the display mode is D3DFMT_X8R8G8B8.
Because the formats differ, every IDirect3DDevice8::Reset() sets the display mode again. On systems where
a display mode change loses the device (seen on Windows 11 with multiple monitors and GPUs), the device is
lost again by its own Reset() before a single frame is presented, so the game resets forever while the
screen keeps switching modes.
When the previous fullscreen Reset() did not last until a frame was presented, Reset_Device now switches the
back buffer to the display format, which lets Reset() restore the device without a display mode change.
Destination alpha effects such as the soft water edge are then unavailable for the rest of the session.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
DX8Wrapper adds fullscreen back-buffer format handling and an optional alpha scene target. It copies the rendered scene to the back buffer before presentation. Frame capture and smudge rendering now obtain the scene surface through the new accessor.
Changes
Fullscreen scene rendering
Layer / File(s)
Summary
Fullscreen reset and target setup Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.h, Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp
DX8Wrapper tracks fullscreen resets without a successful presentation. It selects a display-format back buffer in the fallback case and prepares the alpha scene target when device capabilities permit.
Scene target lifecycle and presentation Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp
DX8Wrapper creates and releases the scene target, copies its contents to the back buffer before presentation, and clears the reset counter after a successful presentation or primary-buffer flip.
Scene surface access and consumers Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.h, Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp, Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DProfilerFrameCapture.cpp, Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DSmudge.cpp
The scene-surface accessor returns the alpha target when available, or the back buffer otherwise. Frame capture and smudge rendering use the accessor.
sequenceDiagram
participant DX8Wrapper
participant Direct3DDevice
participant SceneAlphaTarget
participant BackBuffer
DX8Wrapper->>Direct3DDevice: Reset with selected fullscreen format
DX8Wrapper->>SceneAlphaTarget: Create target when supported
DX8Wrapper->>BackBuffer: Copy scene before presentation
DX8Wrapper->>Direct3DDevice: Present
Loading
Suggested labels: Rendering, Bug, Fix, Gen, ZH
Suggested reviewers: xezon, stephanmeesters
Merge Risk: Moderate · up to f1df1
On multi-adapter fullscreen setups, the display-format fallback may be wrongly rejected, leaving the device in a reset cycle. On some devices, MSAA may also be dropped unnecessarily. Fix the adapter used for the depth-format check before merging.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title identifies the main change: rendering to an A8R8G8B8 scene target when the fullscreen back buffer uses the display format.
Description check
The description covers the scene target, fullscreen fallback, multisampling, and related changes.
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.
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.
Test heat haze on the scene surface with MSAA fullscreen fallback.
W3DSmudgeManager::render now copies the heat haze background from DX8Wrapper::_Get_DX8_Scene_Surface() (W3DSmudge.cpp:445). With MSAA, the scene render target is multisampled. The presentation resolve copies it into a separate texture, so the smudge copy reads a different surface than before.
No automated test covers this copy. The PR also states that visible heat haze was not tested. Run the game in fullscreen fallback with MSAA enabled. Confirm that heat haze renders correctly.
Review scope
This request covers only the heat haze background copy. It does not claim that the copy fails.
1728-1731: Minor · Potential issue · Poor tradeoff
Unverified finding
We couldn't verify this from the code. It is shown for review, not as a verified issue.
The new render-target texture uses the exact display dimensions without accounting for texture dimension restrictions. Format support alone may not permit ordinary non-power-of-two fullscreen sizes on supported DX8 hardware.
CreateTexture fails and rendering continues directly into the non-alpha back buffer, leaving shoreline rendering incorrect despite A8R8G8B8 render-target format support.
This can occur when: Fallback activates at a resolution such as 1024x768 on a device whose render-target textures require power-of-two dimensions.
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: a8edac6c-ebd9-453e-8a6b-5ed3402f6a7e
Commits
Reviewing files that changed from the base of the PR and between 116fbcf and e470382.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
…cannot be created
- Release only unbinds the multisampled depth buffer of the scene target when it is the one that is set.
- Report the multisampling the device has when the scene target cannot be created.
- Create the state block once with the scene target instead of every frame.
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
Minor · Resolve the MSAA scene surface before copying the heat-haze background. · W3DSmudge.cpp:327-328
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DSmudge.cpp:327-328 Minor · Functional Correctness · Quick win
Resolve the MSAA scene surface before copying the heat-haze background.
With MSAA enabled, this call supplies a multisampled source to CopyRects before the presentation resolve. A supported runtime that rejects multisampled CopyRects leaves the background texture unchanged, so visible heat haze can sample empty or stale data. Add an in-frame resolve to a non-multisampled surface before background->Copy(...); the later presentation resolve is too late for this path.
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 67a36c3f-57e8-4b3c-bbe8-8d155b1bfc0a
Commits
Reviewing files that changed from the base of the PR and between e470382 and f492321.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
@coderabbitai Replies to the findings that could not be posted inline:
Heat haze with MSAA (W3DSmudge.cpp:327): without this change, the back buffer is multisampled when MSAA is on, and W3DSmudgeManager::render already copies from it with CopyRects. With this change it copies from the multisampled scene surface instead, which is the same kind of copy. Copy_Scene_Alpha_Target_To_Back_Buffer resolves that surface with CopyRects too, and that is tested with 4x MSAA. Heat haze with visible smudges is still untested, as the description says.
Power-of-two render target textures (dx8wrapper.cpp:1728): if CreateTexture fails, Create_Scene_Alpha_Target releases the target, the scene renders into the back buffer, and the MSAA mode the device has is reported. The game then behaves as with bugfix(dx8): Use the display format for the fullscreen back buffer when a device reset does not last #3656 alone. The fallback only triggers on systems with the reset loop on current Windows, and GPUs with drivers for it support D3DPTEXTURECAPS_NONPOW2CONDITIONAL.
f1df119 also brings in the fix from #3656: if no depth buffer format matches the display format, the back buffer format is kept.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
sed -n '450,610p' Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp | nl -ba -v450
Repository: TheSuperHackers/GeneralsGameCode
Length of output: 4981
Check depth-format support on the selected adapter.
If CurRenderDevice is not the primary adapter, Find_Z_Mode checks D3DADAPTER_DEFAULT. The new guard can reject the display-format fallback even when the selected adapter supports it. The device can then remain in the reset-without-present cycle.
Use CurRenderDevice for the fallback depth-format checks.
For Direct3D 8 IDirect3DDevice8::CopyRects, what restrictions apply when copying a multisampled render target to a non-multisampled surface, especially source and destination rectangles and size? For D3DPTEXTURECAPS_NONPOW2CONDITIONAL, can non-power-of-two textures be used as render targets, and what limitations apply? Use Microsoft Direct3D documentation where possible.
Length of output: 30816
Script executed:
#!/bin/bashset -u
smudge="Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DSmudge.cpp"
wrapper="Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.cpp"echo'--- Smudge hardware test and render copy ---'
sed -n '79,105p;204,310p;313,385p'"$smudge"echo'--- Surface-copy implementations and callers ---'
rg -n -C 4 'Copy_Surface|CopyRects|CopyRect'"$smudge" Core/Libraries/Source/WWVegas/WW3D2
echo'--- Adapter selection ---'
rg -n -C 3 '\bCurRenderDevice\b|Find_Z_Mode\('"$wrapper"| head -220
echo'--- Review comments, excluding other users ---'
gh api --paginate 'repos/TheSuperHackers/GeneralsGameCode/pulls/3657/comments?per_page=100' --jq '.[] | select(.user.login | test("coderabbit"; "i")) | {path, line, original_line, body}'||trueecho'--- Review bodies ---'
gh api --paginate 'repos/TheSuperHackers/GeneralsGameCode/pulls/3657/reviews?per_page=100' --jq '.[] | select((.user.login | test("coderabbit"; "i")) and (.body != null and .body != "")) | {state, body, commit_id}'||true
#!/bin/bash
file="Core/Libraries/Source/WWVegas/WW3D2/surfaceclass.cpp"
ast-grep outline "$file" --items all --match 'Copy' --view expanded ||true
sed -n '436,490p'"$file"
Length of output: 2709
@coconutbird Thanks for the details.
Heat haze: I withdraw the claim that a separate in-frame resolve is required. The previous code also copied from a multisampled back buffer. The tested presentation copy is useful evidence, but it does not test visible heat haze; the smudge copy uses an explicit rectangle. Please test visible heat haze in fullscreen fallback with 4x MSAA on this revision.
Texture dimensions: I withdraw the unverified finding as a confirmed defect. If CreateTexture fails, the code falls back to the back buffer and reports its actual MSAA mode. That fallback still loses shoreline transparency and, if MSAA was requested, disables MSAA after reset. The affected hardware conditions remain unverified.
Depth-format fallback: I confirmed that f1df119 keeps the existing back-buffer format when Find_Z_Mode fails. The separate blocking adapter finding remains: Find_Z_Mode checks D3DADAPTER_DEFAULT, not CurRenderDevice. Please use the selected adapter for this check before merging, and verify the fallback on a non-default adapter.
You are interacting with an AI system.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Problem
#3656 uses the display format (
X8R8G8B8) for the fullscreen back buffer when a device reset does not last until a frame is presented. That back buffer has no destination alpha, so on affected systems the shoreline water effect is off for the rest of the session:renderShoreLinesSortedwrites the water depth into the alpha channel of the back buffer, andW3DWaterblends standing water withD3DBLEND_DESTALPHA. Without it, shallow water at the shore is opaque and the edge of the water mesh shows above the surf.Change
DX8Wrapperrenders the scene into anA8R8G8B8render target texture and draws it onto the back buffer with a full-screen quad beforePresent(). A quad is needed becauseCopyRectsdoes not convertA8R8G8B8toX8R8G8B8.A8R8G8B8surface with its own depth buffer, whichCopyRectsresolves into the texture before the quad is drawn. The back buffer only receives the finished frame, so it is not multisampled. This is decided beforeReset()inPrepare_Scene_Alpha_Target, because it changes the presentation parameters.Reset()and created again after it.getBackBufferFormat()reportsA8R8G8B8while the target exists, so the existing destination alpha checks are unchanged.W3DSmudge) and the profiler frame capture copy the scene in the middle of the frame, so they read the new_Get_DX8_Scene_Surface()instead of the back buffer.Nothing changes in windowed mode, or in fullscreen when the fallback from #3656 does not trigger.
Cost
Only when the fallback is active, at 2560x1440:
The memory is 4 bytes per pixel without MSAA, and 12 bytes per pixel with MSAA at any sample count, because the back buffer and its depth buffer are no longer multisampled. The GPU times were measured with a standalone D3D8 test using random image data, and are probably best case because the GPU cache holds the whole frame.
Screenshots
Shoreline crop of the same frame of the default shell map in three windowed runs at 2560x1440. The
X8R8G8B8runs use a temporary local switch that is not part of this change.Testing
On the system that reproduces #3049 (Windows 11 26200, RTX 5070 driver 32.0.16.1742, two 2560x1440 @ 144 Hz monitors), Zero Hour:
X8R8G8B8back buffer and the scene target: the back buffer equals the scene target in 10 of 10 captures, including after a forced reset. Compared with the originalA8R8G8B8back buffer in the matching frame, 467 pixels differ, all inside the on-screen clock. With theX8R8G8B8back buffer and no scene target (bugfix(dx8): Use the display format for the fullscreen back buffer when a device reset does not last #3656 alone), the standing water at the shore is opaque.An earlier revision of this change kept the back buffer multisampled. Device loss and alt-tab with 4x MSAA were tested with that revision: both reset and frames resumed.
Not tested:
AI Disclosure
Claude Opus 5.5 wrote this PR entirely: the code, the investigation and test tooling, and this description. I directed the work.