From 51faec5c5080d75f94ec7303bbef96cce2963d04 Mon Sep 17 00:00:00 2001 From: csunday95 Date: Tue, 29 Dec 2020 04:12:55 -0800 Subject: [PATCH] Close #13637: Refactor sprite compiler for filesystem efficiency - sprite building would save a file with just the sprite file header and then immediately open it again at the beginning of compilation - sprite file generation is now entirely in memory until the final output file is saved on success - added validation of no file activity in the failed build test case; failed builds will not generate a file or edit an existing one --- src/openrct2/CmdlineSprite.cpp | 30 +++--- test/tests/CLITests.cpp | 98 +++++++++++++++++++ test/tests/testdata/sprites/badManifest.json | 8 ++ test/tests/testdata/sprites/example.dat | Bin 0 -> 8587 bytes test/tests/testdata/sprites/manifest.json | 5 + test/tests/testdata/sprites/result.dat | Bin 0 -> 8 bytes test/tests/tests.vcxproj | 6 ++ 7 files changed, 129 insertions(+), 18 deletions(-) create mode 100644 test/tests/CLITests.cpp create mode 100644 test/tests/testdata/sprites/badManifest.json create mode 100644 test/tests/testdata/sprites/example.dat create mode 100644 test/tests/testdata/sprites/manifest.json create mode 100644 test/tests/testdata/sprites/result.dat diff --git a/src/openrct2/CmdlineSprite.cpp b/src/openrct2/CmdlineSprite.cpp index bb97194498..9c4a7daa9d 100644 --- a/src/openrct2/CmdlineSprite.cpp +++ b/src/openrct2/CmdlineSprite.cpp @@ -589,10 +589,11 @@ int32_t cmdline_for_sprite(const char** argv, int32_t argc) bool silent = (argc >= 4 && strcmp(argv[3], "silent") == 0); - SpriteFile baseFile; - baseFile.Header.num_entries = 0; - baseFile.Header.total_size = 0; - baseFile.Save(spriteFilePath); + // keep sprite file entirely in memory until ready to write out a complete, + // correct file + SpriteFile spriteFile; + spriteFile.Header.num_entries = 0; + spriteFile.Header.total_size = 0; fprintf(stdout, "Building: %s\n", spriteFilePath); @@ -632,25 +633,18 @@ int32_t cmdline_for_sprite(const char** argv, int32_t argc) return -1; } - auto spriteFile = SpriteFile::Open(spriteFilePath); - if (!spriteFile.has_value()) - { - fprintf(stderr, "Unable to open sprite file: %s\nCanceling\n", spriteFilePath); - return -1; - } - - spriteFile->AddImage(importResult.value()); - - if (!spriteFile->Save(spriteFilePath)) - { - fprintf(stderr, "Could not save sprite file: %s\nCanceling\n", imagePath.c_str()); - return -1; - } + spriteFile.AddImage(importResult.value()); if (!silent) fprintf(stdout, "Added: %s\n", imagePath.c_str()); } + if (!spriteFile.Save(spriteFilePath)) + { + log_error("Could not save sprite file, cancelling."); + return -1; + } + free(directoryPath); fprintf(stdout, "Finished\n"); diff --git a/test/tests/CLITests.cpp b/test/tests/CLITests.cpp new file mode 100644 index 0000000000..92be6408f1 --- /dev/null +++ b/test/tests/CLITests.cpp @@ -0,0 +1,98 @@ + +#include "TestData.h" + +#include +#include +#include +#include +#include +#include + +class CommandLineTests : public testing::Test +{ +public: + static std::string SpriteTestDataPath() + { + return Path::Combine(TestData::GetBasePath(), "sprites"); + } + + static std::string ManifestFilePath() + { + return Path::Combine(SpriteTestDataPath(), "manifest.json"); + } + + static std::string BadManifestFilePath() + { + return Path::Combine(SpriteTestDataPath(), "badManifest.json"); + } + + static std::string ExampleSpriteFilePath() + { + return Path::Combine(SpriteTestDataPath(), "example.dat"); + } + + static std::string BuildOutputfilePath() + { + return Path::Combine(SpriteTestDataPath(), "result.dat"); + } + + static bool CompareSpriteFiles(std::string original, std::string generated) + { + std::ifstream originalFile(original, std::ios::binary | std::ifstream::in); + std::ifstream generatedFile(generated, std::ios::binary | std::ifstream::in); + if (!(originalFile.is_open() && generatedFile.is_open())) + { + return false; + } + if (originalFile.tellg() != generatedFile.tellg()) + { + return false; + } + return std::equal( + std::istreambuf_iterator(originalFile.rdbuf()), std::istreambuf_iterator(), + std::istreambuf_iterator(generatedFile.rdbuf())); + } +}; + +TEST_F(CommandLineTests, cmdline_cmdline_for_sprite_details) +{ + std::string exampleFilePath = ExampleSpriteFilePath(); + const char* detailsCmd[3] = { "details", exampleFilePath.c_str() }; + + int32_t result = cmdline_for_sprite(detailsCmd, 2); + // need to come up with some way to extract stdout/stderr stream if we want to + // fully test this module + ASSERT_EQ(result, 1); +} + +TEST_F(CommandLineTests, cmdline_cmdline_for_sprite_build) +{ + std::string manifestFilePath = ManifestFilePath(); + std::string outputfilePath = BuildOutputfilePath(); + const char* detailsCmd[3] = { "build", outputfilePath.c_str(), manifestFilePath.c_str() }; + + int32_t result = cmdline_for_sprite(detailsCmd, 3); + ASSERT_EQ(result, 1); + // compare the resulting output file and assert its identical to expected + ASSERT_TRUE(CompareSpriteFiles(ExampleSpriteFilePath(), outputfilePath)); +} + +TEST_F(CommandLineTests, cmdline_cmdline_for_sprite_failed_build) +{ + // run on correct manifest file + std::string manifestFilePath = ManifestFilePath(); + std::string outputfilePath = BuildOutputfilePath(); + const char* detailsCmd[3] = { "build", outputfilePath.c_str(), manifestFilePath.c_str() }; + int32_t result = cmdline_for_sprite(detailsCmd, 3); + ASSERT_EQ(result, 1); + ASSERT_TRUE(CompareSpriteFiles(ExampleSpriteFilePath(), outputfilePath)); + + // now use bad manifest and make sure output file is not edited + std::string badManifestFilePath = BadManifestFilePath(); + detailsCmd[2] = badManifestFilePath.c_str(); + result = cmdline_for_sprite(detailsCmd, 3); + // check the command failed + ASSERT_EQ(result, -1); + // validate the target file was unchanged + ASSERT_TRUE(CompareSpriteFiles(ExampleSpriteFilePath(), outputfilePath)); +} diff --git a/test/tests/testdata/sprites/badManifest.json b/test/tests/testdata/sprites/badManifest.json new file mode 100644 index 0000000000..b3e46d3469 --- /dev/null +++ b/test/tests/testdata/sprites/badManifest.json @@ -0,0 +1,8 @@ +[ + { + "path": "../images/logo.png" + }, + { + "path": "../images/FAKEFILE.png" + } +] diff --git a/test/tests/testdata/sprites/example.dat b/test/tests/testdata/sprites/example.dat new file mode 100644 index 0000000000000000000000000000000000000000..c81bf87a4234626e337566b66771847ab8a5710e GIT binary patch literal 8587 zcmaJ`Yiu0V8O6?fX4Y%(+KKJ89WR7DoH!8g)4_I3;vhl)P}(ZIe$Wb4ZD>^ik-)E% zwlwi0icm#SKq*9k(p`JKc0%GL#-<3N5TPWh5Q;7z^ET>910t;>3rU0znVeDCs)mv36JZ^g`tR^y0KX#U(hZ-%W`EyMGy=L^rn zMHh?OyyM8g3F=3g}xtM6U?%<4B* zUs(Ok>gw9?hM) z+tXs1=K8U*=-UUIE%g@fV~7`oRtFy0f!hWR4zyB3FOIe^>j~PSa-F%yGxJ6I7DI-o;=om!XP^D-ViyH zN=0Mw`fEE&{Wc<{TLT+8Q0I2BH5}o{)6tlWJ$79U?vk;oI?nJY)Xss1aHK9es0NS6 zC-&?-cD;VDU~`z2-auqK*H;JDg*QYZ>r<)J7_ejUc)YD`=k@E4$ap~?SufKR`URi^ zJ~9gYGf~cn#m9HF?L5}_8E2?`j<4ekJD-l^iO>dcFryS^P(dsm0<=AbKF7i~VCX)@2R~L!WkyK0x zM6#oU=`>cG8gOZQY z*4Eb1G4-C$(R2!r2t1Ja8sWXwTDxlBo{nwXPKH!8BnJit^dJ04{ay2r57lB4$ijq- z_iJnq_S*(>8QgVzO9LZxLV<)s;ZPtD3ZFdL+Pc1`#>eS)2#R^OTD)*!2+SP<5+cmt z$uOewfd@?ECnpC-?|yRA#uy>XD^zBL17IcyXF;5T8=Cz`1yhqq zr-$P+jzZ-aD#%lo`S)@Rnc>>W!Gmwe4}g~?zoF{jzN3g$w=td!<@e{1}F6 zY2-}#<4Ww{EbEA=VHZEZ1jATQa97pOz@E zZX8yd2S~_)^UPpsaH@FM^nCGL(GAL{j<_#Ff{sL6W`x*E{wdc&Gy6~rB_*fQB16#md5n~SSCn}>v| zMqgrIn6YNag`ltn0#}rpECgMNI^Rys=!0ro=gQ%hQ;n4-K+wx~#OUESpnIrH`~TFAgZi`DOHz zdm4fo^T_D0T*h}K2b@+BjUWVTmQvwF&*anfAAn57`SnuARUrf=sk6hrz2J2yqal=# zrkN%w0%~0b(_CL}bHY`G!}`q<=^R;ijD!tX#gC+^(ce+@RN^n^^3oZ1R@+b6nPP!L69TTjW-3*Ee6Wt+|tz-x6Dt zW^Zw^&(E&1!?v8<{%p-`U|X~1{%@h1Tk6XS=T}+zeL?Sc4O`%MV#;e75~inEr$E=i z>?_S7S2fo3O8NH%Y-+1rx>|R39yzT5J$_nrYlluy`>(E8`WRl_=GJ!ms+c4*C7-$*SAtF-IibNjOOzRX?2PwFNwA2GefxE z8BWJk|IJ*@HauIi;#IpDd+jE?>@oR@Ba!4uovdM7uV#&o32oa%tjaJBLiN0_B(KXv zt5(Z>n`#wjw!ij*md?h7giZQ+D92=*6u0wnd$oC{9a3AVyl752J1NdQca^zZOT8VL zc9t9Y)KEn0j76a{J>2$mW4uaq8pe!6=~R_MWgQt`t&odzs>};e*}kHjJW0^kQKeDy z;VT3o93E+Gtbwhx*3g6*iB1pk>H-%t`Fy%}_{B`K>K^i8xw3s`o%V^QRUzveW27RX zrgO`qIs?~D;4;CM7fKlx=S)KF%G4&yGlg40XO*0?y9NamWk$Cqb7=z??wxQoNGo%= z91lAT2nWGI;uyo6xH5`>)QpGq*OhfCgwJwTs+fI z6E-_#prc7GKo)GnLF2|*Er>KW6W6<88n{}|dG7kFqSbU8=j?Ef9?=9c`dEl%c>oRTVOz`k$6$F>r&hmpsdnMUY(&e0N6zh}hFu zAIuQA6ae0s_5oja7xPCy*@3gU${%&$*^T9P)j`9v5E*z^_DKJVpZDly2h(OBp}s2I zHFm&V{>M&bs;t3XnbR74TR`w4EJ5a7q@iJZ;)M2nK+<;S9gA}9!Im831zmZ5sfqGMp9&?&DKZ z>G4~g`KrcJ85+U)+0*&g%R6^qw;1Pa(2w1w^3NDxXTRWNb+`H1#(*%NUZa?$BQj}| zH=mZD-~e4S%+cG`Ojj%E%H>a$Pv)h6maLU|=}lT`CfS7;OUvdbsq*+bMK|Z_wwu{g zTBe#~WWCJ6w|#SH_W#F&8B)mf2AT26?d6xv8(vJgX$icNRjN+LS766jvwPCCFlE7# zsiiZp(HZ?#BGJ_J;iT#DmiTjI%UC0^NydKC(9n`h@R4Y4+HKxr&N(-*Wo0lFv8rC7 z;u{SeErCFw^2))Trj0+1dQF;@o7F%)Dv1-vT8uAm+d8<{_qcgV2a&ae}KDj76& zrDFem<&&(UapX?!P-YYVmBiSF`^zc>peiIQqkN0NG;?#oMnB4br$y@O>T0V0cpnf< z<8XuY!)|qQIi(gl)@VS^K(B<9 literal 0 HcmV?d00001 diff --git a/test/tests/tests.vcxproj b/test/tests/tests.vcxproj index 574ac7a22b..d192c57318 100644 --- a/test/tests/tests.vcxproj +++ b/test/tests/tests.vcxproj @@ -57,6 +57,7 @@ + @@ -78,5 +79,10 @@ + + + + + \ No newline at end of file