Skip to content
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

Switch to LibPNG #1391

Closed
faissaloo opened this Issue Sep 20, 2018 · 11 comments

Comments

Projects
None yet
2 participants
@faissaloo
Copy link
Contributor

faissaloo commented Sep 20, 2018

Is there any particular reason why we're using LodePNG? It's abandoned, slow and is often not on package managers. I'm currently trying to get LibEGM and Emake compiling on MacOS and this is one of the things making it difficult.

@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Sep 20, 2018

Ok, I'll do it, from testing, libPNG is faster too and fundies has been asking me to do this so we don't have to check in lodepng in the repo (even though we do have it in shared). I've just been lazy about doing it, sorry. I will get to it!

@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Sep 22, 2018

Alright, #1442 switches emake and libEGM to libpng. That just leaves LodePNG being used in the engine. Try that out for me and let me know if it makes the setup easier. I mean we already have our reasons for switching which you reiterated, but there's no rush until everybody is sure.

I don't know how performance will be different yet, I will do a benchmark in a second. I am actually doing the reading differently now by decoding a single row of the image at a time, so it may be faster or use a different amount of RAM.

@faissaloo

This comment has been minimized.

Copy link
Contributor Author

faissaloo commented Sep 22, 2018

libEGM compiles successfully now.

@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Sep 22, 2018

Nice, huehue, I'll include the engine change in that pr too.

@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Sep 24, 2018

Ok so I made some tweaks to emake in order to do a benchmark so we can prove to the whole wide hello world that we're doing this scientifically. My table below will compare the average time master and the pull request take, in milliseconds, to load several projects five successive times.

    } else if (ext == "gm81" || ext == "gmk" || ext == "gm6" || ext == "gmd") {
      buffers::Project* project;
      size_t runs = 5;
      double avgTime = 0.0;
      for (size_t i = 0; i < runs;) {
        auto start = std::chrono::high_resolution_clock::now();
        if (!(project = gmk::LoadGMK(input_file))) return 1;
        auto elapsed = std::chrono::high_resolution_clock::now() - start;
        long long milliseconds = std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count();
        std::cout << "time " << i << " " << milliseconds << " ms" << std::endl;
        double delta = milliseconds - avgTime;
        avgTime += delta/++i;
      }
      std::cout << "benchmark completed avgTime " << avgTime << " ms" << std::endl;

      return 0;

Having reviewed the results of my own benchmark, I am now going to proceed to switching the engine to use libpng in my open pull request. I think we've shown well here the practicality of libpng in a real world use case for loading projects in a game engine and that it scales to the task much better.

Master/LodePNG (ms) Pull Request/libpng (ms)
fps6.gmk 0199.8 0093.0 ✔️
Wild Racing.gmk 2271.6 0734.0 ✔️
life.gm6 0031.0 0031.0
platform_engine.gmk 0165.0 0055.6 ✔️
destructible_blocks.gmk 0034.0 0030.4 ✔️
@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Sep 24, 2018

Alright, #1442 now switches the engine to libpng too. Only remnants of LodePNG at this point is in the makefiles, which I can do next. We can still use the shared sources to abstract libpng helpers so it's not as many additions. Overall, this looks like a pretty great change.

@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Feb 13, 2019

Now that #1533 has been merged and we've finished the transition to Google Protocol Buffers, I am coming back and finally going to get this into master. Won't take me much effort to redo it, and this is the best time now that we've eliminated a lot of the redundant places PNGs are handled.

@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Feb 13, 2019

Going to redo the benchmark first so we can see how it shifted as a result of moving the protos directly inside the compiler. I am also going to perform the benchmark differently this time, to include the compile time of the game since that's where the png stuff moved to.

Master/LodePNG (ms) Pull Request/libpng (ms)
fps6.gmk 07381.8 07220.2 ✔️
Wild Racing.gmk 16700.8 14153.4 ✔️
life.gm6 06704.4 07537.4
platform_engine.gmk 07266.6 07186.0 ✔️
destructible_blocks.gmk 06436.6 06359.6 ✔️
@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Feb 14, 2019

Ok, once again, I decided to redo the pr a totally different way, and this time with great results. #1548 is not only faster, it's also cleaner and more complete with LodePNG totally removed. I am now going to redo the benchmark one more time with the build times included.

Master/LodePNG (ms) Pull Request/libpng (ms)
fps6.gmk 07381.8 06820.0 ✔️
Wild Racing.gmk 16700.8 13587.4 ✔️
life.gm6 06704.4 06173.4 ✔️
platform_engine.gmk 07266.6 06651.4 ✔️
destructible_blocks.gmk 06436.6 05855.4 ✔️
@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Feb 16, 2019

Ok so after Josh pointed something out, I made another optimization in 9076bb3 that avoids additional allocations. This had another positive impact on the performance, so I am going to redo the benchmark again.

Master/LodePNG (ms) Pull Request/libpng (ms)
fps6.gmk 07381.8 05979.4 ✔️
Wild Racing.gmk 16700.8 12989.6 ✔️
life.gm6 06704.4 05442.8 ✔️
platform_engine.gmk 07266.6 05813.0 ✔️
destructible_blocks.gmk 06436.6 04890.8 ✔️

Then I later realized b63338e myself and believed it should speed up loading GMKs from GM8 and GM8.1 since those are the two versions that have transparency baked into the image. The only game that was in my original benchmark that fits this description is the Wild Racing GMK, so I redid the benchmark for it alone.

Master/LodePNG (ms) Pull Request/libpng (ms)
Wild Racing.gmk 16700.8 12796.6 ✔️
@RobertBColton

This comment has been minimized.

Copy link
Member

RobertBColton commented Feb 16, 2019

Closing as resolved by #1548. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.