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

Losslessly compress all tiles #21685

Merged
merged 1 commit into from Aug 23, 2017

Conversation

Projects
None yet
5 participants
@Leland
Contributor

Leland commented Aug 20, 2017

Reduces file sizes by >30% on average, reducing total file size by 6.5mb.

Pngcrush, Zopfli, PNGOUT and Advpng were used to compress.

Losslessly compress images
Uses Pngcrush, Zopfli, PNGOUT and Advpng
@kevingranade

This comment has been minimized.

Show comment
Hide comment
@kevingranade

kevingranade Aug 21, 2017

Member
Member

kevingranade commented Aug 21, 2017

@ZhilkinSerg

This comment has been minimized.

Show comment
Hide comment
Contributor

ZhilkinSerg commented Aug 21, 2017

@Leland

This comment has been minimized.

Show comment
Hide comment
@Leland

Leland Aug 21, 2017

Contributor

Decompression speeds should be similar, - perhaps marginally increased (we're talking nanoseconds most likely) and the net result should be faster load speeds as file size is much more important in image loading than compression scheme.

This is for mobile apps, and thus different hardware, but the principle is the same:

Decoding speed appears to be correlated to image file size more than anything else (most likely savings on byteswapping are negligible compared to additional disk I/O and extra data to decompress.)

https://imageoptim.com/tweetbot.html

And another one: https://www.cocoanetics.com/2011/10/avoiding-image-decompression-sickness/

Contributor

Leland commented Aug 21, 2017

Decompression speeds should be similar, - perhaps marginally increased (we're talking nanoseconds most likely) and the net result should be faster load speeds as file size is much more important in image loading than compression scheme.

This is for mobile apps, and thus different hardware, but the principle is the same:

Decoding speed appears to be correlated to image file size more than anything else (most likely savings on byteswapping are negligible compared to additional disk I/O and extra data to decompress.)

https://imageoptim.com/tweetbot.html

And another one: https://www.cocoanetics.com/2011/10/avoiding-image-decompression-sickness/

@kevingranade

This comment has been minimized.

Show comment
Hide comment
@kevingranade

kevingranade Aug 21, 2017

Member
Member

kevingranade commented Aug 21, 2017

@Leland

This comment has been minimized.

Show comment
Hide comment
@Leland

Leland Aug 21, 2017

Contributor

Hah – thought those articles were fairly conclusive. Will work on finding more assertive ones.

Contributor

Leland commented Aug 21, 2017

Hah – thought those articles were fairly conclusive. Will work on finding more assertive ones.

@kevingranade

This comment has been minimized.

Show comment
Hide comment
@kevingranade

kevingranade Aug 21, 2017

Member

I'm convinced that it's faster in theory, so more articles won't help. I just want a simple test that demonstrates that it doesn't actually regress. I need to double-check when tile loading happens, but it might be sufficient to simply do something like:
configure cataclysm to use one of the tilesets
time ./cataclysm
hit the requisite keys to enter a game
hit the requisite keys to exit the game
check the output of time
repeat for each tileset, and for before/after.

Member

kevingranade commented Aug 21, 2017

I'm convinced that it's faster in theory, so more articles won't help. I just want a simple test that demonstrates that it doesn't actually regress. I need to double-check when tile loading happens, but it might be sufficient to simply do something like:
configure cataclysm to use one of the tilesets
time ./cataclysm
hit the requisite keys to enter a game
hit the requisite keys to exit the game
check the output of time
repeat for each tileset, and for before/after.

@codemime

This comment has been minimized.

Show comment
Hide comment
@codemime

codemime Aug 21, 2017

Member

I like the change, but it will tend to regress since the tilesets are occasionally updated.

Member

codemime commented Aug 21, 2017

I like the change, but it will tend to regress since the tilesets are occasionally updated.

@Leland

This comment has been minimized.

Show comment
Hide comment
@Leland

Leland Aug 21, 2017

Contributor

@codemime correct – was almost tempted to not compress the Chesthole tilesets. The other tilesets are very rarely updated.

However, my thinking is that a future regression from an update will have minimal impact and I'm more than happy to compress again at certain points in the future.

Also @kevingranade I've been looking into this a lot more and my thinking was erroneous – there will be no additional "decompression" time. To the computer, these files are rendered exactly the same as the original files, they just happen to be smaller.

PNGs themselves are compressed files, and the performance of decompressing them into raw pixels is only relative to the filesize. Which is to say, the speed of rendering them to the screen will only increase as filesize goes down. The algorithm used to get to that smaller filesize is irrelevant.

Contributor

Leland commented Aug 21, 2017

@codemime correct – was almost tempted to not compress the Chesthole tilesets. The other tilesets are very rarely updated.

However, my thinking is that a future regression from an update will have minimal impact and I'm more than happy to compress again at certain points in the future.

Also @kevingranade I've been looking into this a lot more and my thinking was erroneous – there will be no additional "decompression" time. To the computer, these files are rendered exactly the same as the original files, they just happen to be smaller.

PNGs themselves are compressed files, and the performance of decompressing them into raw pixels is only relative to the filesize. Which is to say, the speed of rendering them to the screen will only increase as filesize goes down. The algorithm used to get to that smaller filesize is irrelevant.

@Coolthulhu

This comment has been minimized.

Show comment
Hide comment
@Coolthulhu

Coolthulhu Aug 21, 2017

Contributor

It should probably be automated.
Best if it was an optional (defaulting to yes if necessary tools are detected) step in make bindist. Then we could have the binary distro always use the smaller files without manual re-compressing.

Contributor

Coolthulhu commented Aug 21, 2017

It should probably be automated.
Best if it was an optional (defaulting to yes if necessary tools are detected) step in make bindist. Then we could have the binary distro always use the smaller files without manual re-compressing.

@Leland

This comment has been minimized.

Show comment
Hide comment
@Leland

Leland Aug 21, 2017

Contributor

@Coolthulhu I will say that my initial goal in this was to free up room in the repo for adding in the music created in the original CDDA contest. As I understand, it was removed due to space constraints on the CI server.

Contributor

Leland commented Aug 21, 2017

@Coolthulhu I will say that my initial goal in this was to free up room in the repo for adding in the music created in the original CDDA contest. As I understand, it was removed due to space constraints on the CI server.

@kevingranade

This comment has been minimized.

Show comment
Hide comment
@kevingranade

kevingranade Aug 21, 2017

Member

@Leland I think a better way to handle the space constraints is to make the data files available outside the existing build path. Also the music files are just gigantic all on their own :/

Member

kevingranade commented Aug 21, 2017

@Leland I think a better way to handle the space constraints is to make the data files available outside the existing build path. Also the music files are just gigantic all on their own :/

@Leland

This comment has been minimized.

Show comment
Hide comment
@Leland

Leland Aug 21, 2017

Contributor

Made #21690 for tracking tile compression automation; this PR should be a net positive for the time being, and ready to merge – assuming @kevingranade's good with my breakdown of compression logic above.

Contributor

Leland commented Aug 21, 2017

Made #21690 for tracking tile compression automation; this PR should be a net positive for the time being, and ready to merge – assuming @kevingranade's good with my breakdown of compression logic above.

@kevingranade

This comment has been minimized.

Show comment
Hide comment
@kevingranade

kevingranade Aug 23, 2017

Member

hahahahaha!!!!.... whew
Are you ready for this? (times in ms)
tl;dr ~60% faster!!!!!!!!

Tileset before after difference
gfx//BlockheadTileset/blockheadtiles.png 68738 25622 43116
gfx//ChestHole16Tileset/fallback.png 83480 2966 80514
gfx//ChestHole16Tileset/tiles24.png 83871 919 82952
gfx//ChestHole16Tileset/tiles.png 145250 23332 121918
gfx//ChestHole32Tileset/fallback.png 158360 9051 149309
gfx//ChestHole32Tileset_iso/fallback.png 169289 74067 95222
gfx//ChestHole32Tileset_iso/tiles48.png 10854 5364 5490
gfx//ChestHole32Tileset_iso/tiles.png 201099 122479 78620
gfx//ChestHole32Tileset/tiles48.png 106521 3333 103188
gfx//ChestHole32Tileset/tiles.png 250300 83373 166927
gfx//ChestHole32Tileset/tree.png 80772 7751 73021
gfx//ChestHoleTileset/fallback.png 26897 5567 21330
gfx//ChestHoleTileset/tiles32.png 3115 2729 386
gfx//ChestHoleTileset/tiles.png 70712 44648 26064
gfx//ChestHoleTileset/tree.png 4482 3817 665
gfx//DeonTileset/deontiles.png 25422 14569 10853
gfx//HitButton_iso/fallback.png 11470 4560 6910
gfx//HitButton_iso/HitButton_iso.png 7737 5012 2725
gfx//HoderTileset/hodertiles.png 6878 4109 2769
gfx//MShock24TilesetModded/fallback.png 80641 23303 57338
gfx//MShock24TilesetModded/tiles.png 289673 32318 257355
gfx//MShock32TilesetModded/fallback.png 63024 33339 29685
gfx//MShock32TilesetModded/tiles.png 73969 71667 2302
gfx//RetroASCIITileset/retroasciitiles.png 18200 3569 14631
gfx//RetroDaysTileset10/retrodaysfallback10.png 103381 1619 101762
gfx//RetroDaysTileset10/retrodaystiles10.png 53724 1653 52071
gfx//RetroDaysTileset20/../RetroDaysTileset10/retrodaysfallback10.png 4152 1592 2560
gfx//RetroDaysTileset20/../RetroDaysTileset10/retrodaystiles10.png 3883 1933 1950
gfx//ThuztorTileset@/thuztortiles@.png 24294 4940 19354
gfx//TsuTileset/tsutiles.png 17631 3760 13871
Member

kevingranade commented Aug 23, 2017

hahahahaha!!!!.... whew
Are you ready for this? (times in ms)
tl;dr ~60% faster!!!!!!!!

Tileset before after difference
gfx//BlockheadTileset/blockheadtiles.png 68738 25622 43116
gfx//ChestHole16Tileset/fallback.png 83480 2966 80514
gfx//ChestHole16Tileset/tiles24.png 83871 919 82952
gfx//ChestHole16Tileset/tiles.png 145250 23332 121918
gfx//ChestHole32Tileset/fallback.png 158360 9051 149309
gfx//ChestHole32Tileset_iso/fallback.png 169289 74067 95222
gfx//ChestHole32Tileset_iso/tiles48.png 10854 5364 5490
gfx//ChestHole32Tileset_iso/tiles.png 201099 122479 78620
gfx//ChestHole32Tileset/tiles48.png 106521 3333 103188
gfx//ChestHole32Tileset/tiles.png 250300 83373 166927
gfx//ChestHole32Tileset/tree.png 80772 7751 73021
gfx//ChestHoleTileset/fallback.png 26897 5567 21330
gfx//ChestHoleTileset/tiles32.png 3115 2729 386
gfx//ChestHoleTileset/tiles.png 70712 44648 26064
gfx//ChestHoleTileset/tree.png 4482 3817 665
gfx//DeonTileset/deontiles.png 25422 14569 10853
gfx//HitButton_iso/fallback.png 11470 4560 6910
gfx//HitButton_iso/HitButton_iso.png 7737 5012 2725
gfx//HoderTileset/hodertiles.png 6878 4109 2769
gfx//MShock24TilesetModded/fallback.png 80641 23303 57338
gfx//MShock24TilesetModded/tiles.png 289673 32318 257355
gfx//MShock32TilesetModded/fallback.png 63024 33339 29685
gfx//MShock32TilesetModded/tiles.png 73969 71667 2302
gfx//RetroASCIITileset/retroasciitiles.png 18200 3569 14631
gfx//RetroDaysTileset10/retrodaysfallback10.png 103381 1619 101762
gfx//RetroDaysTileset10/retrodaystiles10.png 53724 1653 52071
gfx//RetroDaysTileset20/../RetroDaysTileset10/retrodaysfallback10.png 4152 1592 2560
gfx//RetroDaysTileset20/../RetroDaysTileset10/retrodaystiles10.png 3883 1933 1950
gfx//ThuztorTileset@/thuztortiles@.png 24294 4940 19354
gfx//TsuTileset/tsutiles.png 17631 3760 13871
@kevingranade

This comment has been minimized.

Show comment
Hide comment
@kevingranade

kevingranade Aug 23, 2017

Member

BTW, here's my benchmark code:

diff --git a/src/cata_tiles.cpp b/src/cata_tiles.cpp
index ec9ce84170..443cef15fb 100644
--- a/src/cata_tiles.cpp
+++ b/src/cata_tiles.cpp
@@ -316,12 +316,17 @@ static void apply_color_filter(SDL_Surface_Ptr &surf, void (&pixel_converter)(pi
         }
     }
 }
+#include <chrono>
 
 int cata_tiles::load_tileset(std::string img_path, int R, int G, int B, int sprite_width, int sprite_height)
 {
+    auto start = std::chrono::high_resolution_clock::now();
     /** reinit tile_atlas */
     SDL_Surface_Ptr tile_atlas( IMG_Load( img_path.c_str() ) );
-
+    auto end = std::chrono::high_resolution_clock::now();
+    long diff = std::chrono::duration_cast<std::chrono::microseconds>( end - start ).count();
+    printf( "Loaded %s in %ld microseconds\n", img_path.c_str(), diff );
+    
     if(!tile_atlas) {
         throw std::runtime_error( std::string("Could not load tileset image at ") + img_path + ", error: " +
                                   IMG_GetError() );
Member

kevingranade commented Aug 23, 2017

BTW, here's my benchmark code:

diff --git a/src/cata_tiles.cpp b/src/cata_tiles.cpp
index ec9ce84170..443cef15fb 100644
--- a/src/cata_tiles.cpp
+++ b/src/cata_tiles.cpp
@@ -316,12 +316,17 @@ static void apply_color_filter(SDL_Surface_Ptr &surf, void (&pixel_converter)(pi
         }
     }
 }
+#include <chrono>
 
 int cata_tiles::load_tileset(std::string img_path, int R, int G, int B, int sprite_width, int sprite_height)
 {
+    auto start = std::chrono::high_resolution_clock::now();
     /** reinit tile_atlas */
     SDL_Surface_Ptr tile_atlas( IMG_Load( img_path.c_str() ) );
-
+    auto end = std::chrono::high_resolution_clock::now();
+    long diff = std::chrono::duration_cast<std::chrono::microseconds>( end - start ).count();
+    printf( "Loaded %s in %ld microseconds\n", img_path.c_str(), diff );
+    
     if(!tile_atlas) {
         throw std::runtime_error( std::string("Could not load tileset image at ") + img_path + ", error: " +
                                   IMG_GetError() );
@kevingranade

This comment has been minimized.

Show comment
Hide comment
@kevingranade

kevingranade Aug 23, 2017

Member

Android players are gonna LOVE you.

Member

kevingranade commented Aug 23, 2017

Android players are gonna LOVE you.

@kevingranade kevingranade merged commit ff0fc1b into CleverRaven:master Aug 23, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.5%) to 22.409%
Details
gorgon-ghprb Build finished.
Details
@kevingranade

This comment has been minimized.

Show comment
Hide comment
@kevingranade

kevingranade Aug 23, 2017

Member

Side note, I don't trust these numbers to be at all accurate, they're jittery as hell, but it's a massive improvement so measuring more carefully is pointless.

Member

kevingranade commented Aug 23, 2017

Side note, I don't trust these numbers to be at all accurate, they're jittery as hell, but it's a massive improvement so measuring more carefully is pointless.

@ZhilkinSerg

This comment has been minimized.

Show comment
Hide comment
@ZhilkinSerg

ZhilkinSerg Aug 23, 2017

Contributor

Now we gonna need to revive #19165 and/or #19412 and the game will be quicker than roadrunner.

Contributor

ZhilkinSerg commented Aug 23, 2017

Now we gonna need to revive #19165 and/or #19412 and the game will be quicker than roadrunner.

@Leland Leland deleted the Leland:feature/optimize-images branch Aug 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment