Icons with images of size 256 w/h cannot be read. #143

Closed
sorlok opened this Issue Sep 4, 2014 · 23 comments

Comments

Projects
None yet
3 participants
@sorlok
Contributor

sorlok commented Sep 4, 2014

Trying to open a game with a 256x256 icon in LGM gives me the following:
http://pastie.org/9527505

I am fairly sure that this is because the BitmapDescriptor class ignores the magic number "0" for width/height:

width = pDec.read();
height = pDec.read();

According to Wikipedia, .ico files use "0" to mean "256":

Specifies image width in pixels. Can be any number between 0 and 255. Value 0 means image width is 256 pixels.

I assume the appropriate fix looks something like this, but have not tested it:

int w = pDec.read();
int h = pDec.read();
width = w==0 ? 256 : w;
height = h==0 ? 256 : h;
@IsmAvatar

This comment has been minimized.

Show comment
Hide comment
@IsmAvatar

IsmAvatar Sep 4, 2014

Owner

Looks good. Could you indicate what tool(s) you used to create these icons? This will help us QA the fix.

Owner

IsmAvatar commented Sep 4, 2014

Looks good. Could you indicate what tool(s) you used to create these icons? This will help us QA the fix.

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 4, 2014

Contributor

I used icoutils to convert a simple, 256x256 png to an ico file. Here is the actual sample I used:
https://drive.google.com/file/d/0B1P7NepPcOslLVYzM3JHTGNKSFE

I expect any tool that exports 256x256 icons (fully supported since Vista) should be sufficient for QA.

Contributor

sorlok commented Sep 4, 2014

I used icoutils to convert a simple, 256x256 png to an ico file. Here is the actual sample I used:
https://drive.google.com/file/d/0B1P7NepPcOslLVYzM3JHTGNKSFE

I expect any tool that exports 256x256 icons (fully supported since Vista) should be sufficient for QA.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 4, 2014

Collaborator

If you want to submit the changes I'll merge them.

Collaborator

RobertBColton commented Sep 4, 2014

If you want to submit the changes I'll merge them.

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 4, 2014

Contributor

Sure, I'll play around with it over the weekend.

Contributor

sorlok commented Sep 4, 2014

Sure, I'll play around with it over the weekend.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 9, 2014

Collaborator

@sorlok I applied the fix locally and i can confirm that it does stop the exception from occurring and does allow the icon file to load properly. I also edited this icon in regular Paint and the size was 256x256 so if you are certain this is the correct fix I will push it upstream with my game settings/preferences changes.

Collaborator

RobertBColton commented Sep 9, 2014

@sorlok I applied the fix locally and i can confirm that it does stop the exception from occurring and does allow the icon file to load properly. I also edited this icon in regular Paint and the size was 256x256 so if you are certain this is the correct fix I will push it upstream with my game settings/preferences changes.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 9, 2014

Collaborator

I pushed the fix in 7a60cf4 so I am just going to close this because I have confidence in you and I already know it fixes the exception. If there is more to this please post back and I will listen.

Collaborator

RobertBColton commented Sep 9, 2014

I pushed the fix in 7a60cf4 so I am just going to close this because I have confidence in you and I already know it fixes the exception. If there is more to this please post back and I will listen.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 9, 2014

Collaborator

Wait a minute, it appears you ran into my http://www.converticon.com issue sorlok and also found a fix.
http://enigma-dev.org/forums/index.php?topic=1734

So your changes here fix 256x256 icons exported from that site but not 512x512, if I change your code though then the 512 one works but the 256 one does not and vice versa.
512 Works With Modified Code
256 Does Not Work

Collaborator

RobertBColton commented Sep 9, 2014

Wait a minute, it appears you ran into my http://www.converticon.com issue sorlok and also found a fix.
http://enigma-dev.org/forums/index.php?topic=1734

So your changes here fix 256x256 icons exported from that site but not 512x512, if I change your code though then the 512 one works but the 256 one does not and vice versa.
512 Works With Modified Code
256 Does Not Work

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 9, 2014

Contributor

Good work; it loads fine now. The only (minor) issue is that a 256x256 icon won't fit in the settings frame. I was trying to figure out earlier how to fix this; here's my (very basic) fix, which just scales all icons (for viewing) down to 32x32:
http://pastebin.com/1B7VZsHr

There is probably a better solution, such as redoing the frame's layout when the image icon changes.
In terms of the actual icon loading, however, everything is working fine.

Contributor

sorlok commented Sep 9, 2014

Good work; it loads fine now. The only (minor) issue is that a 256x256 icon won't fit in the settings frame. I was trying to figure out earlier how to fix this; here's my (very basic) fix, which just scales all icons (for viewing) down to 32x32:
http://pastebin.com/1B7VZsHr

There is probably a better solution, such as redoing the frame's layout when the image icon changes.
In terms of the actual icon loading, however, everything is working fine.

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 9, 2014

Contributor

I don't think the ico format even supports 512x512. What's your reference for this?

Contributor

sorlok commented Sep 9, 2014

I don't think the ico format even supports 512x512. What's your reference for this?

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 9, 2014

Collaborator

Well I want to only scale it in terms of preview. But first we have something more important to correct. When does 0 mean 256 and when does 0 mean 512? Because it can literally mean both according to this. :(

If I change your code from 256x256 support to 512x512 then the 512x512 icons exported from http://converticon.com do in fact work. But I've also used other exporters to make 512x512 icons before and they also worked fine in LateralGM.

Collaborator

RobertBColton commented Sep 9, 2014

Well I want to only scale it in terms of preview. But first we have something more important to correct. When does 0 mean 256 and when does 0 mean 512? Because it can literally mean both according to this. :(

If I change your code from 256x256 support to 512x512 then the 512x512 icons exported from http://converticon.com do in fact work. But I've also used other exporters to make 512x512 icons before and they also worked fine in LateralGM.

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 9, 2014

Contributor

I guess the right thing to do is to see what Win32 does. The sample code for displaying an icon seems easy enough:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms648051(v=vs.85).aspx#_win32_Displaying_an_Icon

Contributor

sorlok commented Sep 9, 2014

I guess the right thing to do is to see what Win32 does. The sample code for displaying an icon seems easy enough:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms648051(v=vs.85).aspx#_win32_Displaying_an_Icon

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 9, 2014

Collaborator

It's not much help without looking at the internal code of the WinAPI. But they also mention on the about page themselves that you should have up to 48x48
http://msdn.microsoft.com/en-us/library/windows/desktop/ms648050%28v=vs.85%29.aspx
I haven't done much reverse engineering enough to find out what WinAPI does. I was hoping you could figure out what is going on because this actually eludes me why the format would be this whacky.

Collaborator

RobertBColton commented Sep 9, 2014

It's not much help without looking at the internal code of the WinAPI. But they also mention on the about page themselves that you should have up to 48x48
http://msdn.microsoft.com/en-us/library/windows/desktop/ms648050%28v=vs.85%29.aspx
I haven't done much reverse engineering enough to find out what WinAPI does. I was hoping you could figure out what is going on because this actually eludes me why the format would be this whacky.

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 9, 2014

Contributor

It's wacky because they didn't add a version field, so they couldn't just make the width/height be 2 bytes. Actually, you can store PNGs in ICO files, which include the PNG header (specifying the size). But converticon uses BMPs (I checked), and BMPs are far more common anyway.

By the way, converticon uses "0" for both 256x256 icons and 512x512 icons. I'm digging through their files in a hex editor to figure out how they differentiate after that, then I'll code up a Win32 example to make sure Windows actually behaves this way too.

Contributor

sorlok commented Sep 9, 2014

It's wacky because they didn't add a version field, so they couldn't just make the width/height be 2 bytes. Actually, you can store PNGs in ICO files, which include the PNG header (specifying the size). But converticon uses BMPs (I checked), and BMPs are far more common anyway.

By the way, converticon uses "0" for both 256x256 icons and 512x512 icons. I'm digging through their files in a hex editor to figure out how they differentiate after that, then I'll code up a Win32 example to make sure Windows actually behaves this way too.

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 9, 2014

Contributor

Ok, I figured it out. I think Windows actually ignores the icon sizes, and uses the BMP header (or PNG header) included in the raw data. Like so:
http://i.imgur.com/jHuw5gA.png

I'll write a few sample icons to test out in Windows, just to be sure. In the worst case, we only need to rely on this behavior when the icon's width/height are 0.

Contributor

sorlok commented Sep 9, 2014

Ok, I figured it out. I think Windows actually ignores the icon sizes, and uses the BMP header (or PNG header) included in the raw data. Like so:
http://i.imgur.com/jHuw5gA.png

I'll write a few sample icons to test out in Windows, just to be sure. In the worst case, we only need to rely on this behavior when the icon's width/height are 0.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 9, 2014

Collaborator

Excellent, thank you sorlok! I will make sure to get whatever fix you come up with included. And yes that was originally one of my interpretations but I didn't follow though with it mainly because I couldn't find any evidence of it being a BMP embedded in the icon but I was able to read the PNG signature. Just holler when you have the fix/code or something for me to look at I am just practicing calc right now.

Collaborator

RobertBColton commented Sep 9, 2014

Excellent, thank you sorlok! I will make sure to get whatever fix you come up with included. And yes that was originally one of my interpretations but I didn't follow though with it mainly because I couldn't find any evidence of it being a BMP embedded in the icon but I was able to read the PNG signature. Just holler when you have the fix/code or something for me to look at I am just practicing calc right now.

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 9, 2014

Contributor

Sure, I am still testing to make sure I've covered everything. Expect a few hours.

Contributor

sorlok commented Sep 9, 2014

Sure, I am still testing to make sure I've covered everything. Expect a few hours.

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 10, 2014

Contributor

Ok, here we go:

This fixes the actual problem: http://pastebin.com/BebwV9da
This fixes a related problem, and is optional: http://pastebin.com/he1Fr1if

The first patch will allow icons of any size to be loaded. This causes very weird scaling in LGM, as your sample screenshots with the 512x512 demonstrate. If this is ok, then skip patch 2.

The second patch scales the game's icon in LGM only (NOT the game) down to MAX_VIEWABLE_ICON_SIZE, which is 64x64.

Note that these patches are git-happy; you can apply them with, e.g.,:
git apply 0001-Always-rely-on-the-width-height-as-set-in-the-header.pa
git apply 0002-Shrink-very-large-icons-down-for-viewing-only.patch

Let me know what you think.

Contributor

sorlok commented Sep 10, 2014

Ok, here we go:

This fixes the actual problem: http://pastebin.com/BebwV9da
This fixes a related problem, and is optional: http://pastebin.com/he1Fr1if

The first patch will allow icons of any size to be loaded. This causes very weird scaling in LGM, as your sample screenshots with the 512x512 demonstrate. If this is ok, then skip patch 2.

The second patch scales the game's icon in LGM only (NOT the game) down to MAX_VIEWABLE_ICON_SIZE, which is 64x64.

Note that these patches are git-happy; you can apply them with, e.g.,:
git apply 0001-Always-rely-on-the-width-height-as-set-in-the-header.pa
git apply 0002-Shrink-very-large-icons-down-for-viewing-only.patch

Let me know what you think.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 10, 2014

Collaborator

@sorlok awesome I'll take both of the fixes but if you could tell me how exactly I apply the hot fixes. I've never done that before and I cd'd there and it says it can't apply the patch because the file don't exist. So obviously I need to download or fetch it from somewhere.

Collaborator

RobertBColton commented Sep 10, 2014

@sorlok awesome I'll take both of the fixes but if you could tell me how exactly I apply the hot fixes. I've never done that before and I cd'd there and it says it can't apply the patch because the file don't exist. So obviously I need to download or fetch it from somewhere.

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 10, 2014

Contributor

Oh yeah, just download from the link (there is a "Download" button at the top of each link's page). Then, copy them from your Downloads folder to the LateralGM repo folder, then perform the commands in Git Bash.

If it's still too tricky, I'll just fork the repo and make a pull request like normal. Let me know what you prefer.

Contributor

sorlok commented Sep 10, 2014

Oh yeah, just download from the link (there is a "Download" button at the top of each link's page). Then, copy them from your Downloads folder to the LateralGM repo folder, then perform the commands in Git Bash.

If it's still too tricky, I'll just fork the repo and make a pull request like normal. Let me know what you prefer.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 10, 2014

Collaborator

Uhm, what link? I do not see any links or in the terminal either? :(
@sorlok

Collaborator

RobertBColton commented Sep 10, 2014

Uhm, what link? I do not see any links or in the terminal either? :(
@sorlok

@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

This comment has been minimized.

Show comment
Hide comment
@sorlok

sorlok Sep 10, 2014

Contributor

Any luck? If it's too much trouble, I can just make a fork+pull request.

Contributor

sorlok commented Sep 10, 2014

Any luck? If it's too much trouble, I can just make a fork+pull request.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Sep 10, 2014

Collaborator

@sorlok oh I see so I have to save those files then apply the patches? Just go ahead and send a pull request for revision purposes I'd rather have it linked to your account anyway.

Collaborator

RobertBColton commented Sep 10, 2014

@sorlok oh I see so I have to save those files then apply the patches? Just go ahead and send a pull request for revision purposes I'd rather have it linked to your account anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment