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

[Windows] Add icon to the console wrapper, add option to set icon for the console wrapper on export. #68699

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Nov 15, 2022

  • Adds icon to the console wrapper executable (same as main one).
  • Adds option to set icon for the console wrapper on export (uses main icon is console wrapper icon is not set).

Pros: Console executable have an icon.
Cons: Console executable is 350 KB larger (~400 KB, was ~50 KB).

@akien-mga
Copy link
Member

Cons: Console executable is 350 KB larger (~400 KB, was ~50 KB).

Why is our godot.ico so heavy? Here's one of the embedded PNGs, and the same one created with convert godot_6_256x256x32.png godot6.png:

-rw-r--r-- 1 akien akien 262548 Nov 15 23:08 godot_6_256x256x32.png
-rw-r--r-- 1 akien akien  11289 Nov 15 23:08 godot6.png

file identifies both the same way:

godot_6_256x256x32.png: PNG image data, 256 x 256, 8-bit/color RGBA, non-interlaced
godot6.png:             PNG image data, 256 x 256, 8-bit/color RGBA, non-interlaced

Here's the difference in output for identify -verbose on the two images:

--- 1.log	2022-11-15 23:13:20.248916468 +0100
+++ 2.log	2022-11-15 23:13:29.411882190 +0100
@@ -1,5 +1,5 @@
 Image:
-  Filename: godot_6_256x256x32.png
+  Filename: godot6.png
   Format: PNG (Portable Network Graphics)
   Mime type: image/png
   Class: DirectClass
@@ -663,7 +663,7 @@
              1: (255,255,255,220) #FFFFFFDC srgba(255,255,255,0.862745)
              1: (255,255,255,222) #FFFFFFDE srgba(255,255,255,0.870588)
   Rendering intent: Perceptual
-  Gamma: 0.454545
+  Gamma: 0.45455
   Chromaticity:
     red primary: (0.64,0.33)
     green primary: (0.3,0.6)
@@ -682,9 +682,12 @@
   Compression: Zip
   Orientation: Undefined
   Properties:
-    date:create: 2022-11-15T22:08:37+00:00
-    date:modify: 2022-11-15T22:08:37+00:00
-    date:timestamp: 2022-11-15T22:13:20+00:00
+    date:create: 2022-11-15T22:08:58+00:00
+    date:modify: 2022-11-15T22:08:58+00:00
+    date:timestamp: 2022-11-15T22:13:29+00:00
+    png:bKGD: chunk was found (see Background color, above)
+    png:cHRM: chunk was found (see Chromaticity, above)
+    png:gAMA: gamma=0.45455 (See Gamma, above)
     png:IHDR.bit-depth-orig: 8
     png:IHDR.bit_depth: 8
     png:IHDR.color-type-orig: 6
@@ -692,14 +695,15 @@
     png:IHDR.interlace_method: 0 (Not interlaced)
     png:IHDR.width,height: 256, 256
     png:sRGB: intent=0 (Perceptual Intent)
+    png:text: 3 tEXt/zTXt/iTXt chunks were found
     signature: 1eb7b3fb0f4a402b6312aa801ee96a25cd9e220c6c74980e748d1a008315f9fb
   Artifacts:
     verbose: true
   Tainted: False
-  Filesize: 262548B
+  Filesize: 11289B
   Number pixels: 65536
   Pixel cache type: Memory
-  Pixels per second: 7.18983MP
+  Pixels per second: 8.2755MP
   User time: 0.010u
-  Elapsed time: 0:01.009
+  Elapsed time: 0:01.007
   Version: ImageMagick 7.1.0-52 Q16-HDRI x86_64 20549 https://imagemagick.org

@bruvzg
Copy link
Member Author

bruvzg commented Nov 15, 2022

At some point icon was converted fo uncompressed bitmaps (IIRC, it's to allow overwriting it without rcedit, since it's constant size and do not need exe/resource metadata modification). I do not think it was the best idea, but I guess at least someone who did it need this for some reason.

@bruvzg
Copy link
Member Author

bruvzg commented Nov 15, 2022

#57850

@bruvzg
Copy link
Member Author

bruvzg commented Nov 15, 2022

All mentioned issues related to embedded PCK should be already fixed, so if 250 KB is a big deal it might be worth reverting icon conversion.

@akien-mga
Copy link
Member

At some point icon was converted fo uncompressed bitmaps (IIRC, it's to allow overwriting it without rcedit, since it's constant size and do not need exe/resource metadata modification). I do not think it was the best idea, but I guess at least someone who did it need this for some reason.
#57850

Yeah I wasn't convinced at the time and I'm still unconvinced by that change. The fact that it requires the PR author's own tool to author icons in a specific format doesn't strike me as more user-friendly than using rcedit, which is required anyway to edit any other data aside from the icon.

#64073 seems like it was possibly caused by that change too.

Anyway, that's outside the scope of this PR, we can assess that further independently.

@akien-mga akien-mga modified the milestones: 4.x, 4.0 Nov 15, 2022
@akien-mga akien-mga merged commit 29e2aa4 into godotengine:master Nov 15, 2022
@akien-mga
Copy link
Member

Thanks!

@Calinou
Copy link
Member

Calinou commented Nov 15, 2022

The fact that it requires the PR author's own tool to author icons in a specific format doesn't strike me as more user-friendly than using rcedit, which is required anyway to edit any other data aside from the icon.

We could modify the editor's export code to perform hex editing on the icon (and text-based metadata, if we add space padding as I mentioned in #57850).

This would remove the need to use the author's external tool while also removing the need to use rcedit (and WINE on Linux).

@bruvzg bruvzg deleted the win_con_icon branch November 16, 2022 05:42
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