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

PNG compression #2098

Closed

Conversation

eclecticanimal
Copy link

Hello, I tried to do the PNG compression implementation.
I was not able to test if it works, I just get there reading documentation, but not really sure if it would work.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Aug 17, 2023

I'm sorry, but this PR is not good at all. There are multiple major issues with this.

Compilation issues:

  1. First of all, we must clarify that it requires adding zlib library as a dependency. Our engine nor editor do not link it, but without it your code won't work.
  2. New files png.h and png.cpp are not actually added to the project's compilation. They have to be added to MSVS project Common to be compiled on Windows, and they also have to be added to Common/CMakeLists.txt to be compiled along the CMake builds on other platforms.
  3. ZLib library will have to be added to dependencies: in MSVS project, CMake script (I will need to double check where), and Engine/Makefile-defs.linux (maybe Makefile-defs.osx too, I will need to double check if we still use that).

You could start with making this work on Windows at minimum, for example.

Code issues:

  1. png.cpp contains lots of useless code that you copied from lzw.cpp for some reason. I'm speaking about insert and delete functions, as well as global variables and macros. These are used in lzw algorithm, but not png algorithm, so they should not be there.
  2. There are also 2 functions - save_png and load_png - added, also cloned from lzw variants, which are not needed. These lzw variants were used to save room backgrounds with certain extra metadata prepended to them. Even if we are going to save room bgs as pngs too, there's no need to duplicate these 2 functions fully, instead it's better to merge them with lzw ones and pass compression type as an option. But this is something I won't be discussing before the main purpose (the sprite compression) is resolved. So I think it's best to not add such functions at all.

Overall, the rule of thumb is to start with adding only minimal code necessary for a purpose of a feature.

Logical issues:

  1. The new code is not put into use, so it will never get called. To be a complete feature it must be added as an option for saving sprites and/or room backgrounds.
  2. PNG compression option has to be added to the enum SpriteCompression, and this option should be checked in SpriteFileWriter::WriteBitmap and SpriteFile::LoadSprite functions, with pngcompress and pngdecompress called respectively.
  3. PNG compression option has to be added in the Editor too, so that users could select it for their game. It's done by adding it to the SpriteCompression enum.

After this is done, then will then be possible to actually test it on real game projects, see if it adds benefit for compression sizes, and so on.

@ivan-mogilko ivan-mogilko marked this pull request as draft August 17, 2023 02:30
@eclecticanimal
Copy link
Author

Thank you. I'll follow the guidelines

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Aug 18, 2023

Okay, I was not clear about the zlib library.

We do not push whole library sources into the repository, unless there's no other choice. This is redundant, and also this prevents platforms like Linux use the libs installed on their systems.

Instead we add corresponding library to the list of linked libs. I will probably have to explain how.
Perhaps I should write an instruction in our repository's wiki for the future contributors to use.

@ivan-mogilko
Copy link
Contributor

I tried to explain how to add a lib dependency in MSVS here:
https://github.com/adventuregamestudio/ags/wiki/Adding-third‐party-libraries-to-the-project

I think it's best to begin with Windows, since it's required to test this feature with the Editor.

If you have questions, please ask!

@eclecticanimal eclecticanimal marked this pull request as ready for review August 24, 2023 23:09
@eclecticanimal
Copy link
Author

Hello I did some more changes following your guidelines, but not clear about the dependencies.

@ivan-mogilko ivan-mogilko marked this pull request as draft August 25, 2023 00:23
@ivan-mogilko
Copy link
Contributor

I will try making a properly linked test in a separate PR, based on your code, and test if that works in principle.

@ivan-mogilko
Copy link
Contributor

Made #2111 based on your suggestion. I will close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants