timmaffett / material_symbols_icons

Complete Flutter support for google's Material DesignMaterial Symbols Icons
Apache License 2.0
32 stars 5 forks source link

Icons don't fill right when weight is 100 #1

Closed rares45 closed 1 year ago

rares45 commented 1 year ago

Screenshot_2023-04-28-07-26-53-798_com.android.chrome.jpg

Screenshot_2023-04-28-07-28-10-592_com.android.chrome.jpg

timmaffett commented 1 year ago

First thank you for finding (and reporting!) this!

It seems to be caused by flutter's new icon font tree shaking operation, which is supposed to remove unused glyphs, but in this case of a variable font has also removed hinting that is required to render the icon correctly. I have uploaded a new build of the web example and it should work now. It does for me.

I will be investigating the cause of this further so that we can get the icon font tree shaking working in the flutter builder.

It is now built with the --no-tree-shake-icon' flag:

flutter build web --release --web-renderer canvaskit --no-tree-shake-icons --base-href "/material_symbols_icons/"

For reference the previous build was build with:

flutter build web --release --web-renderer canvaskit --base-href "/material_symbols_icons/"

There probably should have been red flags when I noticed that the fonts got 30% smaller even though we are using every glyph for every icon in the example:

Font asset "CupertinoIcons.ttf" was tree-shaken, reducing it from 283452 to 1236 bytes (99.6% reduction). Tree-shaking
can be disabled by providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialIcons-Regular.otf" was tree-shaken, reducing it from 1645184 to 10808 bytes (99.3% reduction).
Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialSymbolsOutlined[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 6944756 to 4781576 bytes
(31.1% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialSymbolsRounded[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 9361824 to 6397020 bytes
(31.7% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app.
Font asset "MaterialSymbolsSharp[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 5848492 to 4079548 bytes
(30.2% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app.
Compiling lib\main.dart for the Web...                             51.2s
timmaffett commented 1 year ago

Just last night I modified by build to have the capability to use pyftsubset to trim the font to only the used icon glyphs:

pyftsubset 'MaterialSymbolsOutlined[FILL,GRAD,opsz,wght]_metricsfixed.ttf' --unicodes-file=icon_unicodes.txt --output-file='../lib/fonts/MaterialSymbolsOutlined[FILL,GRAD,opsz,wght].ttf'

and I found a <3% decrease in size. I started to investigate what icon tree shaking was removing that was 30%... and then I woke up to your bug report. Well, I guess we found out! lol.
I will now delve deeper and see what font tables the icon tree shaking is removing that amounts to 30% and causes this fill/weight 100 bug you discovered. I suspect that whatever font table that is missing is causing other, less obvious, artifacts as well with other options in rendering the variable font.

timmaffett commented 1 year ago

It turns out that the icon tree-shaking is breaking the material symbols variable fonts by dropping the GSUB table. This causes rendering errors for some variations (such as Fill->1 Weight->100) I filed an issue addressing this, https://github.com/flutter/flutter/issues/125704 , as well as a PR fixing font-subset so that it preserves these tables for variable fonts - https://github.com/flutter/engine/pull/41592.

This dropping of the GSUB table was the reason for the 30% saving in font size, even in the case of using every material symbol icon.

The live example has been updated to use the new font-subset for icon font tree-shaking.

timmaffett commented 1 year ago

I am happy to report that https://github.com/flutter/engine/pull/41592 was just merged into the engine and https://github.com/flutter/flutter/issues/125704 has been closed as fixed. The material symbols variable fonts will no longer get broken during icon tree shaking of the material symbols icons variable fonts.

Thank you @rares45 for finding this problem and creating this issue so this could all be possible!

The live example for the material_symbols_icons package will be rebuild with the new engine, including icon font tree-shaking.

Thanks again @rares45 !