-
Notifications
You must be signed in to change notification settings - Fork 118
feat(screenshot): Add threaded JPEG screenshots on F11 without game stalls #1785
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(screenshot): Add threaded JPEG screenshots on F11 without game stalls #1785
Conversation
3eeb16c to
20a3df1
Compare
20a3df1 to
37bd840
Compare
|
Some initial thoughts:
|
|
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. |
|
Regarding Github formatting: When you write
Then it will not close this report when this is merged. Please read up on it here: |
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/* |
15c379c to
3535e1e
Compare
3535e1e to
efc773f
Compare
Generals/Code/GameEngine/Source/GameClient/MessageStream/MetaEvent.cpp
Outdated
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DDisplay.h
Outdated
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
Outdated
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
Outdated
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DScreenshot.cpp
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| { | ||
| MetaMapRec *map = TheMetaMap->getMetaMapRec(GameMessage::MSG_META_TAKE_SCREENSHOT_PNG); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this cause a key conflict as F12 is already in use.
Or is it the intention to take both JPG and PNG screenshots?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is to use ctrl + f12 for png and f12 for jpg, though to your point, maybe the regular f12 should be png as it's lossless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regular F12 for PNG would be more in line with lossless BMP, HOWEVER, most players will want to take JPG screenshots first because these will compress much smaller and are far better for archiving and sending around the internet.
PNG screenshots are more for artists that want to create artwork and similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I went with F12: JPG and CTRL+F12: PNG
ead7d4e to
977a6dc
Compare
977a6dc to
f8162f3
Compare
cmake/stb.cmake
Outdated
| FetchContent_Declare( | ||
| stb | ||
| GIT_REPOSITORY https://github.com/nothings/stb.git | ||
| GIT_TAG master # Could pin to specific commit for stability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pinning to a specific tag or commit is the way forward, otherwise you risk changes to upstream breaking the build, supply chain attacks and such.
Some efforts have been made to move dependencies to vcpkg which might be a better approach. At the very least I'd suggest adding a find command to try and find the module first (from vcpkg or alternatively package managers), then fall back on the fetch content method if not found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
d7e8a8d to
d197bdd
Compare
Summary
Implements compressed JPEG screenshots (F11) that don't stall the game, addressing issues with the current F12 BMP screenshot functionality.
Closes #1555
Adds a new screenshot function using the stb_image_write library with background threading:
Notes
std::shared_ptrfor safe memory management across threadsTesting