XCSoar icon indicating copy to clipboard operation
XCSoar copied to clipboard

Huge icons with v7.42

Open Turbo87 opened this issue 1 year ago • 13 comments

XCSoar versions having and not having the problem

Which XCSoar version are you using when seeing the problem?

v7.42

Which XCSoar version is the last one you know that did not show the problem?

v7.41

System information

Android on OnePlus 8

Steps to reproduce the behavior

Start the app and look at the map 😅

Actual behavior

image

Expected behavior

Not quite as huge icons... :D

Do you have any idea what may have caused this?

https://github.com/XCSoar/XCSoar/commit/3b08f28a5addab8bd3655ef1a3d6e9fc636caac0 implemented high-DPI icons. I guess there might be a downscaling factor missing somewhere?

Do you have an idea how to solve the issue?

Turbo87 avatar Mar 02 '24 09:03 Turbo87

3b08f28a5addab8bd3655ef1a3d6e9fc636caac0 is not the cause. I bet a bisect would track it down to 0f0d6a63343c7f33e086f8086de0903a616c5ac5, which reverts a bad commit, but meanwhile another commit had modified the icon sizes to something else. For those map icons, we really need to get a proper concept how to size them. We have none, so each change randomly changes something, and nobody has any idea what is going on.

We need to define physical sizes for each icon; how big are they really supposed to be. Nobody has ever done that, and previous XCSoar versions just happened to appease most people.

MaxKellermann avatar Mar 02 '24 09:03 MaxKellermann

Can somebody please explain f2d0e21a95bba191f6e211456b6e6b06331622b2 to me? @Sundown3867 / @lordfolken ?

It enlarged map icons from 5x6 pixels to 22x22 pixels, for some definition of "pixel". But what definition of "pixel"? And why make all icons 4 times larger?

@Sundown3867 's commit message only says "Unification of icon size [...] to 22x22 for Map icons", but this isn't just a "unification", it's a 4x growth. But why?

What is the physical size of these icons? Why didn't you specify a physical size instead of a pixel size?

And, @Sundown3867, what does the rest of the commit diff do?

--- a/Data/icons/map_tower.svg
+++ b/Data/icons/map_tower.svg
@@ -1,84 +1,49 @@
 <?xml version="1.0" encoding="UTF-8" standalone="no"?>
-<!-- Created with Inkscape (http://www.inkscape.org/) -->
-
 <svg
-   xmlns:dc="http://purl.org/dc/elements/1.1/"
-   xmlns:cc="http://creativecommons.org/ns#"
-   xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
-   xmlns:svg="http://www.w3.org/2000/svg"
-   xmlns="http://www.w3.org/2000/svg"
-   xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd"
-   xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape"
    version="1.1"
-   width="5"
-   height="6"
-   id="svg2816"
-   inkscape:version="0.48.3.1 r9886"
-   sodipodi:docname="map_tower.svg">
-  <metadata
-     id="metadata9">
-    <rdf:RDF>
-      <cc:Work
-         rdf:about="">
-        <dc:format>image/svg+xml</dc:format>
-        <dc:type
-           rdf:resource="http://purl.org/dc/dcmitype/StillImage" />
-        <dc:title />
-      </cc:Work>
-    </rdf:RDF>
-  </metadata>
+   viewBox="0 0 22 22"
+   id="svg2"
+   sodipodi:docname="map_tower.svg"
+   width="22"
+   height="22"
+   inkscape:version="1.2.2 (732a01da63, 2022-12-09)"
+   xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape"
+   xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd"
+   xmlns="http://www.w3.org/2000/svg"
+   xmlns:svg="http://www.w3.org/2000/svg">
+  <defs
+     id="defs6" />
   <sodipodi:namedview
+     id="namedview4"
      pagecolor="#ff00ff"
      bordercolor="#666666"
-     borderopacity="1"
-     objecttolerance="10"
-     gridtolerance="10"
-     guidetolerance="10"
+     borderopacity="1.0"
+     inkscape:showpageshadow="2"
      inkscape:pageopacity="0"
-     inkscape:pageshadow="2"
-     inkscape:window-width="1432"
-     inkscape:window-height="988"
-     id="namedview7"
-     showgrid="true"
-     inkscape:zoom="47.45"
-     inkscape:cx="-0.11827763"
-     inkscape:cy="1.0989805"
+     inkscape:pagecheckerboard="true"
+     inkscape:deskcolor="#d1d1d1"
+     showgrid="false"
+     inkscape:zoom="9.0769225"
+     inkscape:cx="-43.902545"
+     inkscape:cy="29.084748"
+     inkscape:window-width="2560"
+     inkscape:window-height="1417"
      inkscape:window-x="-8"
      inkscape:window-y="-8"
      inkscape:window-maximized="1"
-     inkscape:current-layer="layer5">
-    <inkscape:grid
-       type="xygrid"
-       id="grid4698"
-       empspacing="5"
-       visible="true"
-       enabled="true"
-       snapvisiblegridlinesonly="true" />
-  </sodipodi:namedview>
-  <defs
-     id="defs2818" />
-  <g
-     transform="translate(-6.9470643,-2.0097858)"
-     id="layer1" />
+     inkscape:current-layer="layer1" />
   <g
      inkscape:groupmode="layer"
-     id="layer5"
-     inkscape:label="NOAA_Image"
-     transform="translate(0.05293565,-12.009785)">
-    <path
-       style="fill:#000000;fill-opacity:1;stroke:#000000;stroke-width:1px;stroke-linecap:round;stroke-linejoin:miter;stroke-opacity:1"
-       d="m 1.4470643,17.50979 0,-3.300005 2,0 0,3.300005 -2,0 z"
-       id="path2827"
-       sodipodi:nodetypes="ccccc" />
-    <path
-       style="fill:none;stroke:#000000;stroke-width:1px;stroke-linecap:square;stroke-linejoin:miter;stroke-opacity:1"
-       d="m 0.44706435,12.509785 0,2 3.99999995,0 0,-2"
-       id="path3605"
-       sodipodi:nodetypes="cccc" />
-    <path
-       style="fill:none;stroke:#000000;stroke-width:1px;stroke-linecap:square;stroke-linejoin:miter;stroke-opacity:1"
-       d="m 2.4470643,12.509785 0,2"
-       id="path3607"
-       sodipodi:nodetypes="cc" />
+     id="layer1"
+     inkscape:label="Image">
+    <g
+       id="g8389"
+       style="stroke-width:2.12375"
+       transform="matrix(1.1777239,0,0,1.1766132,119.83135,66.416644)">
+      <path
+         style="color:#000000;fill:#000000;stroke-width:2.12375;stroke-linecap:round;stroke-linejoin:round;-inkscape-stroke:none"
+         d="m -96.082031,-56.447266 a 0.73004798,0.73004798 0 0 0 -0.728516,0.794922 c 0,0 0.82153,9.734943 -3.550783,16.78711 a 0.72997499,0.72997499 0 0 0 0.23438,1.005859 0.72997499,0.72997499 0 0 0 1.005856,-0.236328 c 4.413159,-7.11805 3.841845,-15.69608 3.75,-16.892578 h 5.923828 c -0.09184,1.196457 -0.661217,9.774508 3.751954,16.892578 a 0.72997499,0.72997499 0 0 0 1.005859,0.236328 0.72997499,0.72997499 0 0 0 0.234375,-1.005859 c -4.372312,-7.052167 -3.550781,-16.78711 -3.550781,-16.78711 a 0.73004798,0.73004798 0 0 0 -0.728516,-0.794922 z"
+         id="path4809" />
+    </g>
   </g>
 </svg>

What is all this? What does it do and why is it not documented in the commit message?

This is confusing and shouldn't have been merged, for formal reasons. Maybe some part of the change is good, but it's hard to see which part, and for sure the commit message is bad.

MaxKellermann avatar Mar 04 '24 08:03 MaxKellermann

Hallo Max, here are some of my reasons for resizing icons done in https://github.com/XCSoar/XCSoar/commit/f2d0e21a95bba191f6e211456b6e6b06331622b2. As you can see, old icon .svgs have all physical sizes - from small 10x10px, ( airspace_intercept.svg), throug 32x32px (clock.svg) to really large 794x1123px (map_reporting_point.svg). Actual size did not mattered, because rsvg-convert function in buil/resource.mk converted all different sized .svgs to uniform13x13px .png for default 100PPI (eg 320x240 4" display) or 22x22px for 160PPI (eg 640x480 5" display) (This is my assumption - I am not a coder). But I did not like the chaos in svg, because for example icons, which should have simmilar size on map - (all naviter-type poins icons) varied in size from 6x6px to 1000x1000px. I made all icons just two sizes - 32x32px .svg for interface icons and 22x22px .svg for map icons. We could actually use just one size for all as the vector graphics .svg was then converted to raster .png with defined size - so yes I enlarged some of icons .svgs and also reduced some a lot to get some unification for simpler editing.

  • for the .svg code - that is default output of the Inkscape .svg file, which contains some bloat. It can be reduced by saving as optimized graphics - I can export them in this simplified form, just let me know how big should I made them

Sundown3867 avatar Mar 04 '24 17:03 Sundown3867

@Sundown3867, you answered none of my questions.

As you can see, old icon .svgs have all physical sizes

Pixels are not "physical". A physical size unit would be "mm" (or "inch" if you must). But not pixels. Pixels have different physical size on different screens. Saying an icon should be 32x32 means it will be very large on low-res displays and very tiny on high-res displays. Pixels are a bad metric for this.

MaxKellermann avatar Mar 04 '24 17:03 MaxKellermann

for the .svg code - that is default output of the Inkscape .svg file, which contains some bloat. It can be reduced by saving as optimized graphics - I can export them in this simplified form, just let me know how big should I made them

I have really no idea what you're trying to say here. If you're trying to justify the big confusion your commit diff caused - no matter what your writing means - your change was bad because it mixes several changes into one commit. Please never do that. And to @lordfolken: please never merge bad commits like that. That only causes confusion, wastes time and causes more work to clean up the mess caused by such commits.

MaxKellermann avatar Mar 04 '24 17:03 MaxKellermann

@Sundown3867, you answered none of my questions.

As you can see, old icon .svgs have all physical sizes

Pixels are not "physical". A physical size unit would be "mm" (or "inch" if you must). But not pixels. Pixels have different physical size on different screens. Saying an icon should be 32x32 means it will be very large on low-res displays and very tiny on high-res displays. Pixels are a bad metric for this.

32x32px is 8.467x8.467mm in Inscape if that helps

Sundown3867 avatar Mar 04 '24 17:03 Sundown3867

~No, that doesn't help, sorry - it's bullshit.~

Sorry, that was actually not so much bullshit. Turns out SVG (and CSS) don't mean "pixels" when they say "pixels", but 1/96th of an inch (at a viewing distance of an arm's length).

(Neither of us knew this, so it doesn't actually make you right or your changes correct - you still didn't answer my questions.)

MaxKellermann avatar Mar 04 '24 17:03 MaxKellermann

I have really no idea what you're trying to say here. If you're trying to justify the big confusion your commit diff caused - no matter what your writing means - your change was bad because it mixes several changes into one commit. Please never do that. And to @lordfolken: please never merge bad commits like that. That only causes confusion, wastes time and causes more work to clean up the mess caused by such commits. This is such bullshit. There is nothing wrong with that commit.

You where warned what undoing that commit will do. You went knee-jerk and did it anyway, ontop of that you broke 5 other things and released the third unusable release in a row.

And thanks to that we are now on an expedited timeline to fix what was broken for years anyway, completely unnecessary.

Stop developing on master and do PRs like the rest of us. And coordinate releases. And especially don't do far reaching changes when its the beginning of the season!

lordfolken avatar Mar 04 '24 18:03 lordfolken

There is very much wrong with that commit, but you ignore everything I said about it (you don't have answers to my questions, do you?), and reframe the breakages caused by a combination of bad commits and blame me for tipping off this big pile of fragile changes - and no, I wasn't warned to undo this commit.

After I reverted the bad part of your commit, you complained, but obviously either you didn't understand what was bad about it, or you didn't want to understand. And then you didn't help cleaning up the mess you caused (revealed by my commits); instead you kept blaming me.

Enough of this ignorance, and bye.

MaxKellermann avatar Mar 04 '24 18:03 MaxKellermann

These questions are not questions. They are accusations. As to the reason why you think X is better you leave people in the dark, and hope that they somehow arrive at your conclusion.

Had you said: "Look I think we need physical size in the SVGs, because we take them do x to them and that would have a better result in the end at y." People may have been happy to oblige.

lordfolken avatar Mar 04 '24 20:03 lordfolken

No, these are questions, not accusations. When somebody does something, but doesn't document it properly, I always assume they have a plan, and then I ask questions to find out about this plan. Yet most of the time, I'm disappointed that there was no plan at all and they simply didn't know what they were doing (they never admit, they just don't answer my questions and cease responding - just like you never admitted that the commit I reverted was bad).

You may or may not like my approach of asking questions and trusting other people's competence, but calling it "accusations" is ignorant. That makes yourself guilty of the thing you say about me.

This is my last post here. I have removed myself from this GH org and disabled all notifications. I'll no longer be the shit filter for this project, and I hope somebody else will play this unpopular role from now on. It is a necessary role, but few people understand this (and are able to withstand the social pressure that comes with doing this job well).

MaxKellermann avatar Mar 04 '24 21:03 MaxKellermann

32x32px is 8.467x8.467mm in Inscape if that helps

Sorry. One very last post, because I had a simple idea I had to verify, and somebody is wrong on the internet; spoiler: it's not me. Let's collect the facts:

  • @Sundown3867 resized the icons to be 8.467x8.467mm.
  • For phones, XCSoar adjusts the physical size by a factor of 2/3 (because the viewing distance of phones in a cockpit is usually not an "arm's length" but considerably less).
  • Thus, according to your design, @Sundown3867, the physical size of those icons on a phone should be 5.6mm.
  • This bug report complains that the icons are too large.
  • On the screenshot, they are ~ 90x90 pixels (real pixels, not CSS/SVG "pixels").
  • The OnePlus 8 has a display density of 402 dpi.
  • 90 pixels at 402dpi is 90/402 inch = 0.224 inch.
  • 0.224 inch = 5.7mm.

The icons should be 5.6mm, and they are actually 5.7mm. I say my calculation/rendering code/concept is as accurate and correct as it can be. It's the icons that are wrong.

If something's wrong, the correct way forward is to analyze what is really wrong, what caused it and fix it. You guys did not do that. Instead, you went the lazy route: analyze nothing and blame me. It's oh-so easy to say "this is the first bad commit", but that's short-sighted (and wrong in this case). It's fighting the symptoms, not fighting the actual problem. You can get a long way only fighting symptoms and ignoring the problems (most people do that), so much that it may feel like you're right, but you're not.

I demonstrated that my code calculated the icon sizes correctly, that my concept of using physical sizes works well, and that the changes I reverted were indeed wrong (though there is more wrongness that hasn't yet been reverted/fixed). I now predict that you never admit it, and you may even fabricate more bullshit to keep your bogus world view intact, but, uh, welcome to the internet. Not my problem anymore. I only felt it was important to prove my point, even if I know nobody would understand (or even listen).

MaxKellermann avatar Mar 05 '24 05:03 MaxKellermann

Yes Max, If you use scale 1 my icons are big. I created a pull reqest fixing this issue. Because I usually mess my GitHub stuff I enclose reduced icons in .zip file icons.zip I consider it closed case as I do not owe anything to anybody here now

Sundown3867 avatar Mar 05 '24 20:03 Sundown3867