Remove transparent backgrounds from screenshots

[JonBooth78]

This fixes #4671

Also, somewhat more optimal and safer, given the old code appears to be copying most bytes 3/4 times and then copying 2/3 bytes off the end of the image, if I read it correctly.

As a side effect screenshot images are a bit smaller now too.

[JonBooth78]

Not quite sure what to do about the commented out alternative path, or if the optimal path is really required, go for simplicity, leave as it or something else?

[sturnclaw]

I'm not a great fan of the built-in screenshot functionality, but I can see the argument for having it, especially with the existence and quality of screenshot functionality varying wildly between OS brand and distribution. I'd recommend paring down the write_png function to only what's absolutely necessary for taking screenshots / dumping the backbuffer; other uses for the function should either go through a "real" PNG library for e.g. editor asset management or be implemented as an in-process tool for e.g. inspecting intermediate render targets.

If possible, I'd highly recommend all or almost all code for "taking a screenshot" be moved into the PngWriter file and decoupled from the rest of the code; as of the last time I worked on it, we have to link the savegame dump command line utility against SDL2_Image because PngWriter is a transitive dependency somewhere and somehow.

[JonBooth78]

Hi @Web-eWorks , I'm not quite sure what you're suggesting.

I can see 3 places that use this "PngWriter.h" in our codebase

• Pi.cpp where it is used to take in-game screenshots, using write_screenshot
• ModelViewer.cpp where it is used to take screenshots, I assume in that tool, using write_screenshot
• GasGiant.cpp where it is used to save a png of a gas giant if DUMP_TO_TEXTURE is #defined. This directly uses write_png.

Are you suggesting moving the fragments of code in the first two cases into write_screenshot, consolidating the two functions in that file and removing the third?

And/or are you suggesting we remove the dependency on SDL to save png files? This could be achieved, for example, by adding a dependency on libpng and does seem a better solution. This is something I could do, it's been years since I looked at libpng but I'd wager it hasn't changed much. However, I think that's out of the scope of this PR.

It seems like the PngWriter.cpp file is compiled as part of the pioneer-lib target. I'm not sure why this is getting pulled in as a transient dependency of the savegame dump utility unless somehow it's use of something else inside Pi.cpp is causing Pi::HandleKeyDown to pull it in and the linker hasn't managed to prune it all. Although why the command line utility would get caught up in all that, I don't know !?! I do see that if I remove the additional dependency on SDL2_Image from the savegamedump executable when building with visual studio it links just fine.

[sturnclaw]

I can see 3 places that use this "PngWriter.h" in our codebase

This is fine - if there's duplicated functionality between the three paths then coalesce them; otherwise it's not a huge exposure. I'd probably suggest removing the GasGiant.cpp codepath entirely, as the need to dump the texture is completely obviated by the in-process texture cache explorer via the Ctrl+I menu.

And/or are you suggesting we remove the dependency on SDL to save png files?

No, I don't think that's necessary - I apologize for the wild goose chase I've inadvertently led you down. I have a distinct memory from 3 or 4 years gone of setting up one of the CLI executable targets and it requiring a linkage against SDL_Image for very very non-obvious reasons connected to the screenshot functionality, but it's very possible it's been "fixed at a distance" in the intervening time and I've simply not noticed the knock-on effect.

This could be achieved, for example, by adding a dependency on libpng and does seem a better solution.

I'd rather not pull any additional libraries in for the purpose of writing out screenshots on the command line. If we introduce a resource loading system of some sort I'd be happy to revisit the question but I think SDL_Image is a fine (and small) enough dependency for the purpose.

I do see that if I remove the additional dependency on SDL2_Image from the savegamedump executable when building with visual studio it links just fine.

Excellent news - if that also builds on the CI machines then I'll consider the whole mess resolved. 😄

[sturnclaw]

...this would be because I in fact was the one to fix everything in the world linking against PngWriter - it used to be linked into utils.cpp back when that was still a thing 80% of the codebase depended on.

If interested, this is the commit responsible for fixing the dependency on PngWriter 6a19458 - it seems I simply forgot to remove the linkage against SDL_Image in the savegamedump target.

[JonBooth78]

Nice :-)

No worries, I only spent about 15 mins looking at this; no wild geese involved.

I'll remove the DUMP_TO_TEXTURE path from the gas giant code, see if I can remove the dependency in CMakelists.txt and tidy the rest over the next week or so.

[sturnclaw]

Bump @JonBooth78 - I'd like to merge this ahead of Pioneer Day if your schedule permits making the needed changes in that timeframe 😄

[JonBooth78]

Hi @Web-eWorks , I've done all the tidy up required and then fixed another bug I had (when taking screenshots my F3 view with the ship stopped showing the ship, instead a picture in picture of itself).

I also stripped out a dead function in the renderer that would need to be considered if we ever put video recording back into the game. I added a comment by the ifdefed out code that might want to call it. I believe that function was buggy anyway as per my comments in the commit removing it.

[sturnclaw]

Sigh... clang-format constantly tries to squirrel its way into aligning arguments or parameters in an incorrect way. Nothing you need to address here, but I'll have to review the clang-format file again, for the Nth time.