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

[xezon]

Spawning new thread for every image is quite an expensive approach. It would be better to have a screenshot processing thread sleeping and wake up when new work is ready. Unfortunately with c++98 we do not have good sync primitives for that so maybe the thread spawn is ok for now.

[xezon]

It still offers quality 1. What is the use case for this low quality level?

[stephanmeesters]

This appeared to work and is a bit simpler:

diff --git a/Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp b/Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
index 7d3e84791..d4dbe7c07 100644
--- a/Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
+++ b/Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
@@ -171,7 +171,6 @@ void W3D_TakeCompressedScreenshot(ScreenshotFormat format, Int jpegQuality)
 		return;
 	}
 
-	unsigned int y;
 	unsigned int width = surfaceDesc.Width;
 	unsigned int height = surfaceDesc.Height;
 
@@ -179,11 +178,7 @@ void W3D_TakeCompressedScreenshot(ScreenshotFormat format, Int jpegQuality)
 	// operations are done on the screenshot thread to keep the main thread cheap.
 	const unsigned int bytesPerPixel = is32Bit ? 4 : 2;
 	unsigned char* pixels = new unsigned char[bytesPerPixel * width * height];
-
-	for (y = 0; y < height; y++)
-	{
-		memcpy(pixels + y * width * bytesPerPixel, (const unsigned char*)lrect.pBits + y * lrect.Pitch, width * bytesPerPixel);
-	}
+	memcpy(pixels, lrect.pBits, bytesPerPixel * width * height);
 
 	surfaceCopy->Unlock();
 	surfaceCopy->Release_Ref();

[xezon]

Will this work correctly in regards to lrect.Pitch ?

[stephanmeesters]

I think so because lrect.Pitch * height equals bytesPerPixel * width * height, we're copying all the channels

[greptile-apps]

Missing package include path

This wrapper target is now always created, but the found-package path still does not reliably populate its include directory. Stb_INCLUDE_DIR is only assigned in the FetchContent branch above, so when find_package(Stb CONFIG QUIET) succeeds with a package that exposes an imported target or different include variable, this line can give the local stb target no include path. Downstream targets still include <stb_image_write.h>, so that build can configure successfully and then fail at compile time with the header missing. Please populate the wrapper from the found package's exported include directories, or link the package target directly when it exists.

Prompt To Fix With AI

This is a comment left during a code review.
Path: cmake/stb.cmake
Line: 19

Comment:
**Missing package include path**

This wrapper target is now always created, but the found-package path still does not reliably populate its include directory. `Stb_INCLUDE_DIR` is only assigned in the FetchContent branch above, so when `find_package(Stb CONFIG QUIET)` succeeds with a package that exposes an imported target or different include variable, this line can give the local `stb` target no include path. Downstream targets still include `<stb_image_write.h>`, so that build can configure successfully and then fail at compile time with the header missing. Please populate the wrapper from the found package's exported include directories, or link the package target directly when it exists.

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

[greptile-apps]

Found package loses includes
When find_package(Stb CONFIG QUIET) succeeds, Stb_INCLUDE_DIR is never assigned because it is only set in the FetchContent branch. The local stb target is still created, but line 19 gives it no usable include path, so downstream files that include <stb_image_write.h> can fail to compile with a vcpkg-provided package. Please populate the wrapper target from the found package target or include directories on that path.

Prompt To Fix With AI

This is a comment left during a code review.
Path: cmake/stb.cmake
Line: 19

Comment:
**Found package loses includes**
When `find_package(Stb CONFIG QUIET)` succeeds, `Stb_INCLUDE_DIR` is never assigned because it is only set in the FetchContent branch. The local `stb` target is still created, but line 19 gives it no usable include path, so downstream files that include `<stb_image_write.h>` can fail to compile with a vcpkg-provided package. Please populate the wrapper target from the found package target or include directories on that path.

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