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

Lossless PNG Compression #2803

Closed
wants to merge 3 commits into from

Conversation

ExeVirus
Copy link
Contributor

@ExeVirus ExeVirus commented Jan 6, 2021

These two commits applied png compression to all textures in MTG using the following two utiities, in two separate commits:

https://github.com/fhanau/Efficient-Compression-Tool/releases
https://sourceforge.net/projects/optipng/

Both were set to maximum compression modes and to strip needless metadata. The resulting PNG files are lossless copies of the originals, but total texture bytes have shrunk from 247,571 Bytes to 140,719 Bytes. Or 56.84% of the original file sizes.


As a second discussion, I also tested the results of using webp to store the same information losslessly, and the result was 120066 Bytes, or 48.50% of the original file sizes. We may want to look at including webp decoding support in minetest in the future. Their license is compatible, I believe: https://www.webmproject.org/license/software/

@paramat
Copy link
Contributor

paramat commented Jan 7, 2021

Both were set to maximum compression modes and to strip needless metadata.

This will probably break leaf and bush textures (anything for 'allfaces_optional' nodes) which need to remain RGBA to function properly in the various modes.
👎 until this is checked and corrected if necessary.

Please read through #1831 for important points.
Probably, the only thing that should be done is removing metadata, textures should not be changed from RGB(A) to indexed colour.
Also, texture compression should wait until just before a release (is 5.4.0 release soon? i am not sure, you should check this).
We get regular texture compression PRs (because at first they seem so easy and obvious) but most are unsuitable for various reasons, or are submitted at the wrong time.
@TumeniNodes

As a second discussion, I also tested the results of using webp to store the same information losslessly, and the result was 120066 Bytes, or 48.50% of the original file sizes. We may want to look at including webp decoding support in minetest in the future.

👎
There is no point being this obsessed with compression when textures are 16x16 and are only 247kB in total. It is not important enough to justify the time, effort and attention, lets keep it simple.

@ExeVirus
Copy link
Contributor Author

ExeVirus commented Jan 7, 2021

Interestingly this is what most other said you would say. I'm relatively new to the community in terms of PR's.
I will go back and put back the original versions of the leaf and bush textures (anything with allfaces_optional), that's easy and I understand the reasons.
I will still compress them and check that they work as expected, but leave them in RGBA mode rather than indexed. (and confirm they look correct. I will go through every node in MTG before updating the PR next)

(is 5.4.0 release soon? i am not sure, you should check this)

Roughly before the end of january

There is no point being this obsessed with compression when textures are 16x16 and are only 247kB in total.

This cost me roughly 45 minutes to go from scratch to writing a script to compressing every texture in MTG. It can absolutely be automated, super easily.

As for file sizes, all of MTG amounts to roughly 300 KB in total media. So, removing out ~100 KB of that is a significant reduction. This 100 KB is done for every single server that every client connects to. That reduces first time load times and actually does have a significant effect on player adoption for servers.

Finally, minetest is a worldwide game, many don't have access to the same bandwidth speeds you or I do. While mapblocks will dwarf 100 KB over the course of playing several hours, 100 KB is comparable to loading up spawn for most servers. Long story short, even if you don't care, others most certainly do. As evidenced by the three thumbs up this has already received.

P.s. Be on the lookout for compressed .obj files next. We also send those over the wire without compression and we can shrink them by roughly 50% as well.

Edit 1:

I searched every lua file and these are the list of files used in allfaces_optional nodes:

  • default_leaves.png
  • default_jungleleaves.png
  • default_pine_needles.png
  • default_acacia_leaves.png
  • default_aspen_leaves.png
  • default_leaves_simple.png
  • default_blueberry_bush_leaves.png
  • default_blueberry_overlay.png

So when you are looking at the updated PR, check that those are either unchanged or at least still in RGBA color mode. I will also be checking them visually, as well as checking with gimp. (though gimp is most of the reason these files are bloated in the first place, but that's a different story, I love gimp, and they deserve better compression options....)

@ExeVirus
Copy link
Contributor Author

ExeVirus commented Jan 8, 2021

Okay, further update. I have reviewed every node in minetest, especially anything having to do with allfaces. I have seen no difference at all. here's the 7 nodes using the above mentioned textures. Do you see anything I'm missing?

image

In light of this, I would push back against your assumption that allfaces has a real issue with indexed pngs. perhaps someone compressed them incorrectly? Or maybe I don't have my setup the same to recreate the issue?

Unless shown otherwise, I don't see why any changes are needed to this PR or any reason not to merge.

Anyways, I read through #1831, and can shed some more light on pertinent issues:

  1. Metadata is a major part of the problem, but also the some textures just aren't compressed well.
  2. I will show you the exact programs and settings used to compress every texture in MTG. This was done with a batch script with the above mentioned two programs I linked, on a windows machine.

For commit one, I used:

ect.exe --strict -strip --allfilters-b "file.png"

Options explained:
Strict requires that the results be 100% lossless
Strip removes metadata
Allfilters-b says to try all filters, brute force approach with a genetic algorithm (ai speak for try smartly)


For Commit two, I used:

opti.exe -o7 -clobber"file.png"

Options explained:
-o7 is check every compression method possible (again)
Clobber means to overwrite the file, only if it got any smaller.


And that's everything. The two programs result in the smallest file sizes possible out of ~6 programs I tried. YET, they have had no visible effects on the nodes from what I can find. (tried for 95 minutes to find differences...)

@ExeVirus
Copy link
Contributor Author

ExeVirus commented Jan 8, 2021

Further Update

I went and searched for the original issue and found this: #1397

I don't have the needed hardware to test if the issue has been fixed, and will now be reverted the needed textures. Coincidentally, all textures affected by that issue haven't been changed since they were fixed in 2016.

I have been able to revert all 30 of the files that were displaying problems, and then removed only the metadata.

The result is nearly the same size as my compression (because those weren't the bad culprits) and now we can be certain there wont be issues.

…uanti-org#1397

Apply optipng -strip all to remove only metadata and no further compression
@ExeVirus ExeVirus force-pushed the CompressPNG-ECT-Opti branch from 56755a8 to 3a7d0e8 Compare January 8, 2021 04:29
@ExeVirus
Copy link
Contributor Author

ExeVirus commented Jan 8, 2021

Okay, updated and ready to be merged 👍

@rubenwardy
Copy link
Contributor

Usually, a compression program will convert RGBA textures to 'indexed colour' mode, this breaks leaves textures ('allfaces_optional' drawtype) as the transparent pixels have important colour information that is used when rendered as opaque.

I can't reproduce this with this PR. Does it only happen on certain platforms?

@ExeVirus
Copy link
Contributor Author

ExeVirus commented Jan 22, 2021

Thanks for checking out the PR Ruben, all textures that use allfaces_optional have NOT been converted into indexed mode. I have opted to use the original versions, and stripped metadata only. In fact, the original PR from 2016 that reverted those textures is what I reverted to for those textures, and it seems there were others than just allfaces_optional that caused the issue. 30 files in total were done this way. The reverting PR is: 47efa2f

Edit:
Further clarification, you can only reproduce the error with certain graphical settings (mipmapping I believe) AND you must have a non-NVIDIA GPU. Very, very specific. Check the parent issue the PR I linked addresses.

@ExeVirus
Copy link
Contributor Author

Closing this pull Request after much discussion with community and recent core dev meeting. Will be submitting a new PR with much less controversy, but similar effect.

@ExeVirus ExeVirus closed this Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants