Adds ScreenshotWriter to minimise code duplication
Needs ReviewPublic
Actions

Authored by vladislavbelov on Oct 5 2019, 6:28 PM.

Details

Reviewers

None

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Summary

Subj.

Test Plan

Apply the patch and compile the game
Run the game and make sure that screenshots work as before

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 9759
Build 16405: Vulcan Build	Jenkins
Build 16404: Vulcan Build (Windows)	Jenkins

Event Timeline

vladislavbelov created this revision.Oct 5 2019, 6:28 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/927/display/redirect

Harbormaster failed remote builds in B9752: Diff 10089!Oct 5 2019, 6:28 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/412/display/redirect

Stan added a subscriber: Stan.Oct 5 2019, 6:47 PM

Stan added inline comments.

source/ps/ScreenshotWriter.cpp
89	static_cast ?
191	static_casts ? space between operators.
source/ps/ScreenshotWriter.h
51	Anything more specific i32, i16 ?

vladislavbelov added inline comments.Oct 5 2019, 6:50 PM

source/ps/ScreenshotWriter.h
51	`size_t` makes sense. As it's used in `Tex::wrap`.

Tested and seems to work on first sight, no compile warnings with clang 8.0.1.

Perhaps two classes are good, but do as you wish if you need this as a preparation for.... for what even? I don't see the reference in the summary (so as an unaware reader I have to assume it's refactoring to minimize as much duplication as possible independent of any other plans).

Don't see anything objectionable, if this patch doesnt give anyone a reason to complain, then noone would have a reason to complain, then it would be unreasonable to complain (otherwise I might complain).

source/ps/ScreenshotWriter.cpp
63	could be done in the header
134	`if(` in lib/, `if (` elsewhere or so

vladislavbelov added a parent revision: D2356: Adds BMPBlockWriter to prevent huge memory allocation.Oct 5 2019, 8:30 PM

vladislavbelov mentioned this in D2356: Adds BMPBlockWriter to prevent huge memory allocation.Oct 5 2019, 9:02 PM

Adds if ( space and moves destructor to header.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/932/display/redirect

Harbormaster failed remote builds in B9758: Diff 10095!Oct 6 2019, 5:17 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/417/display/redirect

Fixes @Stan `s notes.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/933/display/redirect

Harbormaster failed remote builds in B9759: Diff 10096!Oct 6 2019, 5:20 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/418/display/redirect

Stan added inline comments.Oct 6 2019, 7:49 PM

source/ps/ScreenshotWriter.cpp
103	Could it be size_t ?
117	cast to definite size.
124	Space between operators :) I'm not a fan of combining line declarations, but it's up to you.
131	Do you need that to be a GLvoid, or can you use char* ?
131	Does it needs to be freed ?