feat(screenshot): Add threaded JPEG/PNG screenshots without game stalls

[bobtista]

Summary

Replaces the old BMP screenshot with compressed JPEG screenshots that don't stall the game, and adds PNG support.

Closes #1555
Closes #106 ... sort of

Adds a new screenshot function using the stb_image_write library with background threading:

• F12 - Compressed JPEG screenshot (no stalling, ~600KB files)
• Ctrl+F12 - Lossless PNG screenshot

Notes

• stb_image-write is public domain licensed, single-header, zero compilation needed
• Captures frame buffer on main thread (~1-2ms)
• Spawns detached thread for JPEG/PNG compression and disk I/O
• JPEG quality configurable via Options.ini (default 80)
• Works for both Generals and Zero Hour

Testing

• Screenshots save correctly with F12 (jpg) and Ctrl+F12 (png)
• No game stalling during screenshot
• Files appear in correct directory with sequential numbering
  

[Stubbjax]

Some initial thoughts:

• Keep variants of the same function to a single hotkey; instead toggle image format via a new setting in Options.ini
• Apply image compression / quality via a new setting in Options.ini
• Can we consolidate this logic in core instead of repeating the implementation twice?

[xezon]

Agree with Stubbjax.

JPG 90 is big file. Better make it default 80.

Replace BMP screenshot with PNG screenshot. PNG is lossless compressed and always better than BMP.

Make F12 take JPG 80 screenshot. Make CTRL+F12 take PNG screenshot. Make JPG Quality adjustable.

Remove the old BMP code(s) and only use the new code for screenshot.

[xezon]

Regarding Github formatting:

When you write

Addresses #1555

Then it will not close this report when this is merged.

Please read up on it here:
https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#closing-multiple-issues

[bobtista]

Some initial thoughts:

• Keep variants of the same function to a single hotkey; instead toggle image format via a new setting in Options.ini
• Apply image compression / quality via a new setting in Options.ini
• Can we consolidate this logic in core instead of repeating the implementation twice?

RE moving logic to core, I moved what I could to core - but there are a lot more files that need to be moved to core before this can be moved there eg WWVegas/WW3D2/*

[xezon]

Needs rebase.

[bobtista]

Needs rebase.

Done

[Skyaero42]

Looks good to me

[Skyaero42]

This needs rebasing before review.

[greptile-apps]

Greptile Summary

This PR replaces the old BMP screenshot system with a threaded JPEG/PNG implementation using stb_image_write, adding Ctrl+F12 for lossless PNG alongside the existing F12 JPEG shortcut, with JPEG quality configurable via Options.ini.

• New W3DScreenshot.cpp extracts screenshot logic out of W3DDisplay.cpp into a standalone translation unit; the STB implementation file lives in Core and is included by both game targets via the corei_gameenginedevice_private INTERFACE.
• Frame-buffer capture still happens on the main thread (~1–2 ms for surface copy + pixel swizzle), while compression and disk I/O are offloaded to a detached Windows thread; all previously flagged issues (missing DESTROY_MESSAGE, uninitialized m_jpegQuality, thread-failure memory leak, premature success message) are resolved in the current revision.

Confidence Score: 5/5

The change is safe to merge; the new threading model correctly segregates main-thread surface capture from background compression/IO, and every previously identified defect has been fixed in this revision.

All the concrete bugs identified in earlier review rounds — missing DESTROY_MESSAGE for the JPEG case, uninitialized m_jpegQuality, user-preference quality never applied, memory leak on thread creation failure, and the premature success message — are fully addressed in the current code. The build wiring (Core INTERFACE library carrying stb_image_write_impl.cpp into both game targets) is architecturally sound. No new functional defects were found.

No files require special attention. W3DScreenshot.cpp in both Generals/ and GeneralsMD/ is the highest-churn new code and has been well reviewed through multiple iterations.

Important Files Changed

Filename	Overview
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp	New file implementing threaded JPEG/PNG capture; all previously flagged issues are resolved (DESTROY_MESSAGE, memory cleanup on thread failure, success message gated on thread creation, quality fallback to m_jpegQuality).
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp	Mirror of Generals W3DScreenshot.cpp for Zero Hour; identical logic, only header comment differs. All issues resolved in both copies.
cmake/stb.cmake	Adds STB via FetchContent when vcpkg doesn't provide it; add_library(stb INTERFACE) is unconditional so the target is always defined, and Stb_INCLUDE_DIR is populated in both paths.
Core/GameEngineDevice/Source/W3DDevice/GameClient/stb_image_write_impl.cpp	Single-purpose translation unit that defines STB_IMAGE_WRITE_IMPLEMENTATION; compiled into both game targets via the corei_gameenginedevice_private INTERFACE.
Core/GameEngine/Source/Common/OptionPreferences.cpp	Adds getJPEGQuality() reading JPEGQuality key from Options.ini; clamps result to [1, 100] with default 80 when key is absent.
Generals/Code/GameEngine/Source/Common/GlobalData.cpp	Initializes m_jpegQuality to 80 in constructor and loads it from OptionPreferences in parseGameDataDefinition; mirrors correctly in GeneralsMD.
Generals/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp	Registers F12 for JPEG screenshot and Ctrl+F12 for PNG; both bindings use the guarded if(map->m_key == MK_NONE) pattern.
Generals/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp	Both MSG_META_TAKE_SCREENSHOT and MSG_META_TAKE_SCREENSHOT_PNG cases correctly set disp = DESTROY_MESSAGE before break.
Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DScreenshot.h	Minimal shared header declaring W3D_TakeCompressedScreenshot with default quality=0, ensuring the m_jpegQuality fallback triggers when called from W3DDisplay::takeScreenShot.

Sequence Diagram

sequenceDiagram
    participant User
    participant MetaMap
    participant CommandXlat
    participant W3DDisplay
    participant W3DScreenshot
    participant BGThread

    User->>MetaMap: Press F12 (or Ctrl+F12)
    MetaMap->>CommandXlat: MSG_META_TAKE_SCREENSHOT(_PNG)
    CommandXlat->>W3DDisplay: "takeScreenShot(JPEG|PNG)"
    W3DDisplay->>W3DScreenshot: W3D_TakeCompressedScreenshot(format)
    W3DScreenshot->>W3DScreenshot: Get back buffer surface copy
    W3DScreenshot->>W3DScreenshot: Lock surface, swizzle BGR to RGB pixels
    W3DScreenshot->>W3DScreenshot: Unlock and release surface
    W3DScreenshot->>BGThread: CreateThread(screenshotThreadFunc, data)
    W3DScreenshot->>User: "TheInGameUI->message(GUI:ScreenCapture)"
    BGThread->>BGThread: stbi_write_jpg / stbi_write_png
    BGThread->>BGThread: delete imageData, delete threadData

Loading

_{Reviews (14): Last reviewed commit: "fix(stb): Create stb target on both vcpk..." | Re-trigger Greptile}

[greptile-apps]

_{34 files reviewed, 7 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{34 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{34 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{34 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{34 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp
Dead CreateBMPFile function left behind

The old takeScreenShot method (which was the only caller of CreateBMPFile) has been removed, leaving this ~75-line static function as dead code. It will produce a compiler warning for unused static function. Consider removing it since BMP screenshot functionality is fully replaced by the new JPEG/PNG implementation. The same applies to the GeneralsMD mirror file.

Prompt To Fix With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp
Line: 2807:2880

Comment:
**Dead `CreateBMPFile` function left behind**

The old `takeScreenShot` method (which was the only caller of `CreateBMPFile`) has been removed, leaving this ~75-line static function as dead code. It will produce a compiler warning for unused static function. Consider removing it since BMP screenshot functionality is fully replaced by the new JPEG/PNG implementation. The same applies to the GeneralsMD mirror file.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

_{33 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{33 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[bobtista]

Any more changes requested? Or is this good to merge?

[githubawn]

LGTM, frametime analysis shows zero stutter.

Nitpick 1: The screenshot naming logic seems to be incrementing per file extension (or filling the lowest available index) rather than using a global counter. This could break 'Sort by Name' in the destination folder because a newer PNG might get a lower index than an older JPG.

Nitpick 2: The shortcut for uncompressed images has been changed from F12 to Ctrl+F12. This took me a whole few seconds to get used to. But for users this might be preferred.

Neither of these are blockers for me personally.

[Skyaero42]

Nitpick 1: The screenshot naming logic seems to be incrementing per file extension (or filling the lowest available index) rather than using a global counter. This could break 'Sort by Name' in the destination folder because a newer PNG might get a lower index than an older JPG.

Maybe the filename should contain the date and time (incl milliseconds) instead as a sequencer

[bobtista]

JPEG

I'm happy to add this now or as a follow on PR, would be good to get this merged though. Edit: Done, went with Skyaero's suggestion.