Efficient icon storage in git
AbandonedPublic
Actions

Authored by Campbell Barton (campbellbarton) on Jan 10 2014, 4:01 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)

Maniphest Tasks

T37989: Efficient storage of icons in git

Summary

This patch implements proposal: https://developer.blender.org/T37989

adds the ability to store uncompressed pixmaps in git so icons can be updated without increasing the repository by almost 1mb for any icon change.

The patch can is 2 parts.

datatoc_icon_split.py - which splits the PNG into pixmaps (writing out pixmaps from a grid when they contain any non-transparent values). Only runs when updating icons.
datatoc_icon.c - which runs at build time and converts the pixmaps into a PNG. (only when its needed).

Works for both CMake and SCons.

Current conventions for naming are taken from UI_icons.h eg:

release/datafiles/blender_icons16/icon16_solid.dat
release/datafiles/blender_icons16/icon16_render_animation.dat

This is optional, we can have XXxYY.dat naming too if we want to apply this to other files which dont have UI_icons.h header.

One thing that would be good to have is color comparisons so icons wont be updated if there are only very minor changes (as may happen between different systems or inkscape versions).

Note on applying patch

After applying the patch, run this to generate dat files for the first time:

cd release/datafiles
./blender_icons.sh

... to generate the data files.

Diff Detail

Event Timeline

Updated to work with SCons

Made icon names use UI_icons.h

Why is there both a datatoc_icon.py and datatoc_icon.c? Can't we have just one of them?

source/blender/datatoc/CMakeLists.txt
45	On OS X, I had to add some things to fix link errors: link_directories(${PNG_LIBPATH} ${ZLIB_LIBPATH}) target_link_libraries(datatoc_icon ${PNG_LIBRARIES} ${ZLIB_LIBRARIES})

@Brecht Van Lommel (brecht). it is odd having datatoc_icon.py and datatoc_icon.c,
(This is already the case for SCons which uses a python-only datatoc),

But we could have SCons use datatoc.c and datatoc_icon.c.

Couple of general questions:

Do we really need blender as a dependency to update icons?
Naming of the scripts could be improved to be more verbose. For example, blender_icons.sh could be blender_update_icons.sh.
Patch should also remove blender_icons16.png and blender_icons32.png from the working tree i'd say and create blender_icons* folders
Someone need to create .bat file for Windows before actual commit
It is annoying to have .dat files which could not be investigated easily. Isn't it possible to have uncompressed png files?

Will do deeper check later today or tomorrow.

source/blender/datatoc/datatoc_icon_split.py
28	I don't see this file in the patch.

Patch should also remove blender_icons16.png and blender_icons32.png from the working tree i'd say and create blender_icons* folders
Someone need to create .bat file for Windows before actual commit

I assume that's planned but omitted from this patch to keep it smaller. (Or because binary files don't work in patches, though they do work if you use arc diff)

Do we really need blender as a dependency to update icons?
Naming of the scripts could be improved to be more verbose. For example, blender_icons.sh could be blender_update_icons.sh.

I guess that's because otherwise it would have to be written in C, since there's no simple way to read PNGs from python without an extra library. I don't mind it so much. Perhaps it would be slightly nicer if blender_icons.sh was a python script that you would run as ./blender release/datafiles/blender_update_icons.py?

It is annoying to have .dat files which could not be investigated easily. Isn't it possible to have uncompressed png files?

PNG files would be a bit nicer for inspecting or for looking at commit logs, not sure how much work it would be to get that working.

But we could have SCons use datatoc.c and datatoc_icon.c.

I would prefer this. The Scons code for compiling makesrna and makesdna is a bit messy, but perhaps it could be wrapped in some function to make it reusable.

Further this patch seems to work ok here, and I couldn't spot bugs scanning over the C and python code.

@Sergey Sharybin (sergey)

if we want to avoid blender as a dependancy, probably we can have inkscape write a different format which can be loaded in native python, but I dont think its so bad, if someone updates the icons they for sure have blender anyway.
naming scritps can be changed, am not so fussed on this, for this patch however Id keep the names unchanged.
I had the patch removing blender_icons16.png, blender_icons32.png and it caused arc to fail to apply the patch with some strange message about SHA1 mismatch, so I didnt touch with the update.
We should have a bat file (but we should have a bat file anyway, irrespective of this patch)

Regarding annoying to have .dat files...

The issue with not having .dat files is you need to store the icons metadata for its location in the image somewhere. PNG's can store metadata so its possible to store there... but this complicates things a little more. also its not certain that 2 different libpng's will write identical images which makes it harder to know which changes.

The data files are easy to inspect- just run:

datatoc_icon_split_to_png.py *.dat

Campbell Barton (campbellbarton) abandoned this revision.Jan 13 2014, 2:54 PM

Campbell Barton (campbellbarton) mentioned this in rBS4c9a3a53bd31: UI Icons: store icons in git as uncompressed pixmaps (D196).Nov 20 2014, 9:09 AM

Yevgeny Makarov (jenkm) mentioned this in D13068: T88467: Split icon SVG file to be 1 icon per file.Jan 4 2022, 10:57 AM