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

Convert images to webp format #6611

Merged
merged 7 commits into from Apr 30, 2022
Merged

Conversation

Pentarctagon
Copy link
Member

The commits in order:

  • Fix a couple issues with the optiwebp script I noticed.
  • Convert the contents of all images to webp, but leaving their extensions untouched.
  • Rename all image files to match their webp contents.

This was done in two separate commits like this so that git would be able to recognize the image files as being the same before and after, and so still be able to track their history through the conversion.

The list of file size changes for all images changed, sorted by change in size, is here. This conversion does result in 34 files that have their size increased by some amount, however given it's only 34 out of 15,532 images, for the sake of consistency I went ahead and had those converted as well. Additionally:

  • My own remaining work (replacing all references to .png and .jpg images with .webp) it's simpler since I can just blindly replace all filename extensions.
  • Likewise, for UMC authors, they can rename all of their usage of mainline images to webp without needing to double check whether it's actually one of the few that's not a webp image.

The end result of this conversion is a decrease in the total size of the images Wesnoth uses from 234,037,555 bytes to 160,955,590. This is a decrease of 73,081,965 bytes, or 31.23% (almost 1/3).


Remaining work:

  • Actually change all of mainline to reference the webp images.
  • See if there's a way for wmllint to automatically do this work. The main thing I'm wondering about being whether it would be able to tell apart images an add-on is using from mainline vs from its own images directory.
  • Re-add some test png and jpg images somewhere so the scons conf tests still work.

@Pentarctagon
Copy link
Member Author

@doofus-01 and @Lordbob FYI as well.

@newfrenchy83
Copy link
Contributor

Wait, you convert all images? What you convert story, portraits and other large file to wepb, ok, but small like units sprite, no.

You must convert what heavy files for who reduce by change of format is useful, and never convert images in wesnoth\images folder.

@Pentarctagon
Copy link
Member Author

Why not?

@stevecotton
Copy link
Contributor

Maybe we should change the image-loading support so that UMC doesn't need to include the extension, or for 1.18 support automatically remapping .png and .jpg to .webp.

@Pentarctagon
Copy link
Member Author

Is there a central place that handles image loading, or is it more dispersed?

@stevecotton
Copy link
Contributor

I don't know.

@Wedge009 Wedge009 added the Graphics Issues that involve the graphics engine or assets. label Apr 6, 2022
@Elvish-Hunter
Copy link
Contributor

See if there's a way for wmllint to automatically do this work. The main thing I'm wondering about being whether it would be able to tell apart images an add-on is using from mainline vs from its own images directory.

I'm not even sure whether it could be a good idea to have wmllint handle this.
Explanation: whenever we modify a file path, the usual procedure consists in adding a pair with the old and the new paths to the linechanges tuple. This tuple currently has 561 pairs, and I'm quite sure that adding 5'000 more path substitutions to it will slow down wmllint, all for a change that needs to be made only once on any add-on (not to mention the code readability issues that we'll get).
At this point, I see two solutions:

  1. Add all these webp substitutions to a module, which will be imported and used only when enabled by a specific command-line switch;
  2. Make a completely new Python script, which UMC authors will have to use only once on their add-ons.

Maybe we should change the image-loading support so that UMC doesn't need to include the extension, or for 1.18 support automatically remapping .png and .jpg to .webp.

Removing the extensions from WML will certainly cause issues to wmlscope (which uses a regex to catch all the references to image files). Also, what's going to happen if someone decides to have two files with different extensions (for example, .png and .webp) but otherwise identical paths?

@Pentarctagon
Copy link
Member Author

Of those two options, do you have a preference one way or the other?

@Elvish-Hunter
Copy link
Contributor

Probably I'd go for a separate script and put the list of all the updated paths into a module. The script itself can be quite simple: use os.walk to generate a list of WML (and maybe Lua?) files, open each one of them, do a string.replace on each line and use argparse to handle the few command line switches needed (like a dryrun one, or maybe another to ignore paths outside the core directory).

@Pentarctagon
Copy link
Member Author

Alright. Lua files can contain image paths as well, so it would definitely would need to include lua.

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Apr 9, 2022

I get that converting everything vastly simplifies the task, but… does this actually work? If nothing else, I thought the terrain system hard-coded the png file extension…

Is there a central place that handles image loading, or is it more dispersed?

I believe there is a central place, yes. So adding a fallback to try multiple extensions if the first one fails is likely possible.

Also, what's going to happen if someone decides to have two files with different extensions (for example, .png and .webp) but otherwise identical paths?

I think this is also easily solved – try the file exactly as given first and only try appending a file extension if that file doesn't exist.

@Pentarctagon
Copy link
Member Author

I get that converting everything vastly simplifies the task, but… does this actually work? If nothing else, I thought the terrain system hard-coded the png file extension…

I don't know, but if it doesn't, it should probably be made to work.

@Pentarctagon
Copy link
Member Author

@Elvish-Hunter Is there a preferred way to write the edited lines back to the file? Or should it just write back the entire file if any line changes?

@doofus-01
Copy link
Member

I get that converting everything vastly simplifies the task, but… does this actually work? If nothing else, I thought the terrain system hard-coded the png file extension…

I don't know, but if it doesn't, it should probably be made to work.

I am trying to clean the terrain macros up, it would be a good time to convert them. Is it just .png -> .webp? Unless the concern is just hard coded C++, in which case there is probably no issue, since the file extension is specified in the terrain macros.

@Elvish-Hunter
Copy link
Contributor

@Elvish-Hunter Is there a preferred way to write the edited lines back to the file? Or should it just write back the entire file if any line changes?

The usual way is this one:

  • open the file ('r' mode)
  • read it into a variable
  • iterate over its lines and modify them
  • write the content of the variable back into the file ('w' mode).

It's also possible to open a file in read and write mode at the same time (r+ or w+ modes), however I've never used it and I'm suspecting that it'll be tricky to get everything right. Better safe than sorry.
If you're worried about the speed of this script, I've been thinking that, since any given file won't need to rely on the output of other files (unlike wmllint), it should be possible to use the multiprocessing module and have more files updated in parallel (unlike the threading module, this one is capable of bypassing the Global Interpreter Lock and use all the available cores).

@Pentarctagon
Copy link
Member Author

Using a read-write mode I assume would be essentially just the same as the steps you listed, just that the file wouldn't need to be opened twice?

I'll see how long it takes single-threadedly before looking into multi-threading though.

@Pentarctagon
Copy link
Member Author

@doofus-01 it's just png -> webp. There are also definitely some places in C++ where the extension is hard-coded, I just don't know if it's done in the terrain code or not. I'm not really sure why it would do that (why do the extra work of stripping off the extension?), but I could also see it being done since it was probably assumed everything would always be png files.

@CelticMinstrel
Copy link
Member

The reason for hard-coding an extension is, I think, because file paths are built up from components. Besides the terrain code (possibly), other examples might be damage icons or ellipses.

@Pentarctagon
Copy link
Member Author

Pentarctagon commented Apr 9, 2022

@CelticMinstrel Hmm, alright. On the other hand though, just from a quick grep, I do see a lot of the terrain graphics macros do include the .png extension in their names (as doofus-01 mentioned). I'll see what I find I guess.

@Elvish-Hunter Does wmllint have some way of dealing with renames of files used in the new(er) animation syntax? ie: image="units/human-magi/mage-idle-[1~4,4*2,4~1].png:[150*10]"

@newfrenchy83
Copy link
Contributor

LIke .png extension specified in both .cfg files and cpp files, i continue to belive what convert all images is a bad idea, because the addons develloper who call core images in their proper files must modify extensions in cfg files for these images. For me, converting in webp is interesting for big files (portraits, story). I alrrady said that before but with all discussion what i see here, perhaps what you could think to this.

@newfrenchy83
Copy link
Contributor

don't convert in wesnoth\images folder without cganging extension in src files.

@Elvish-Hunter
Copy link
Contributor

Does wmllint have some way of dealing with renames of files used in the new(er) animation syntax? ie: image="units/human-magi/mage-idle-[14,4*2,41].png:[150*10]"

No, it doesn't; then again, the only sprite paths that have been updated and added to the linechanges tuple until now are the Heavy Infantryman idle animations and the Dunefolk. wmlscope can parse and expand that syntax to look for unresolved references (it doesn't modify the files).

For me, converting in webp is interesting for big files (portraits, story).

Indeed, since portraits and story images don't have to deal with the animation syntax, they're easier to handle.

@doofus-01
Copy link
Member

LIke .png extension specified in both .cfg files and cpp files, i continue to belive what convert all images is a bad idea, because the addons develloper who call core images in their proper files must modify extensions in cfg files for these images. For me, converting in webp is interesting for big files (portraits, story). I alrrady said that before but with all discussion what i see here, perhaps what you could think to this.

I do pretty much agree with this, by the way. I'll hold off on the conversions in terrain-graphics - not that anyone was waiting on that, I anticipate merge conflicts with #6606.

@Pentarctagon
Copy link
Member Author

Well I guess,if the script can be updated to handle the animation syntax, what are people's thoughts about it? Very nearly every image in the repo gets smaller by being converted to webp, so the download size reduction will be much smaller if only portraits and story images are done.

@CelticMinstrel
Copy link
Member

Is it easy to make a copy with a slightly modified script that skips all images 72x72 and smaller, just to determine what the reduction would be in that case?

@Pentarctagon
Copy link
Member Author

I'm also fairly confident that if terrain graphics don't support webp images in practice currently, we will be getting a feature request to have that be possible at some point.

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Apr 9, 2022

Is it easy to make a copy with a slightly modified script that skips all images 72x72 and smaller, just to determine what the reduction would be in that case?

(That would still catch a few terrain and unit graphics, but it seems like a good starting point at least.)

I'm also fairly confident that if terrain graphics don't support webp images in practice currently, we will be getting a feature request to have that be possible at some point.

Yes, I'd say we definitely need to figure something out on this front.

@Pentarctagon
Copy link
Member Author

Is it easy to make a copy with a slightly modified script that skips all images 72x72 and smaller, just to determine what the reduction would be in that case?

It looks like it'd be fairly straight forward just from a quick google search, though it would require installing the Pillow library (if that matters).

@Elvish-Hunter
Copy link
Contributor

Or you can look at wmlscope's incorrectlysized() function, where I coded a way to read the size of a png file without using external libraries.

@github-actions github-actions bot added Campaign (any) Deprecated tag, replaced with separate tags for each mainline campaign SCons Issues involving the SCons build system. labels Apr 13, 2022
@Elvish-Hunter
Copy link
Contributor

Yes, if they're just 683 it's better to place them inside wmllint. Actually, they'll be even less because paths referring to mainline campaigns aren't updated by wmllint, so I'll just have to add everything from the core directory.

@Pentarctagon
Copy link
Member Author

Alright, looks like that'd be closer to 200 images actually. So remaining work then being:

  • Add the changed core images to wmllint.
  • Update all the images in WML/lua.

@github-actions github-actions bot added the WML Tools Issues involving WML maintenance tools. label Apr 14, 2022
@Elvish-Hunter
Copy link
Contributor

In af56f0d I've noticed an error. linechanges is a tuple of pairs, so every pair (which is also a tuple) must be followed by a comma; this doesn't happen except for the first three lines of the commit.

@Pentarctagon
Copy link
Member Author

Whoops, fixed.

@newfrenchy83
Copy link
Contributor

@Pentarctagon you must change extension in doxyfen.cpp and manual html help files for fix ubuntu/CI

@Pentarctagon
Copy link
Member Author

I'm getting to it, it just takes a while to manually update all the image paths.

@github-actions github-actions bot added AI Issues with the AI engine, including micro AIs. Docs Documentation issues, both in-game and otherwise (e.g. manual and manpages). MP Issues with multiplayer support or bundled multiplayer content. UI User interface issues, including both back-end and front-end issues. Units Issues that involve unit definitions or their implementation in the engine. labels Apr 23, 2022
@Pentarctagon Pentarctagon marked this pull request as ready for review April 23, 2022 18:15
@Pentarctagon
Copy link
Member Author

@Elvish-Hunter Just to double check the python bits by the way, not the entire thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI Issues with the AI engine, including micro AIs. Building Build-time issues. Campaign (any) Deprecated tag, replaced with separate tags for each mainline campaign Docs Documentation issues, both in-game and otherwise (e.g. manual and manpages). Graphics Issues that involve the graphics engine or assets. MP Issues with multiplayer support or bundled multiplayer content. SCons Issues involving the SCons build system. UI User interface issues, including both back-end and front-end issues. Units Issues that involve unit definitions or their implementation in the engine. WML Tools Issues involving WML maintenance tools.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants