Page MenuHomeWildfire Games

Add window icon
ClosedPublic

Authored by vladislavbelov on Aug 5 2017, 3:18 PM.

Details

Reviewers
ffffffff
s0600204
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP20067: Add an icon to pyrogenesis' application window
Trac Tickets
#4363
Summary

Add an icon to the game window. It works for me on Windows 8.1 and for @Imarok on Windows and Linux. Also this change should be tested on all supported platforms (Windows, Linux, macOS) by someone else too.

Before:

After:

Test Plan
  1. Apply patch and compile the game
  2. Run game
  3. Test that the icon looks ok

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.Aug 5 2017, 3:18 PM
Vulcan added a subscriber: Vulcan.Aug 5 2017, 4:05 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1799/ for more details.

Stan added a subscriber: Stan.Aug 5 2017, 4:10 PM
Stan added inline comments.
source/ps/VideoMode.cpp
270 ↗(On Diff #3011)

RGBA maybe ?

275 ↗(On Diff #3011)

bgra -> rgba and maybe rgba_icon ?

vladislavbelov added inline comments.Aug 5 2017, 4:35 PM
source/ps/VideoMode.cpp
270 ↗(On Diff #3011)

We don't have TEX_RGB.

Stan awarded a token.Aug 5 2017, 4:41 PM
Stan rescinded a token.
Stan awarded a token.
vladislavbelov added a subscriber: s0600204.EditedAug 5 2017, 5:08 PM

Window icon (PNG, 128x128):

Should be at the art/textures/icons/window.png. Thanks to @s0600204 for the remind to attach.

Initially (before @vladislavbelov attached the icon image file separately) this patch caused crash on pyrogenesis start for me:

tex_codec.cpp(70): Function call failed: return value was -120101 (Unknown error (-120101, 0xFFFFFFFFFFFE2ADB))
Function call failed: return value was -120101 (Unknown error (-120101, 0xFFFFFFFFFFFE2ADB))
Location: tex_codec.cpp:70 (tex_codec_for_header)

This seems to be down to arc not acquiring the new image from phabricator properly, causing the image file to... not be an image file.

Also note that if the image doesn't exist at all, I get a different error message:

terminate called after throwing an instance of 'PSERROR_System_VmodeFailed'
  what():  System_VmodeFailed
Aborted (core dumped)

After manually copying the attached image into the appropriate location, the patch works fine (Arch Linux).

However, the fact that 0ad crashes at start if the image file doesn't exist or isn't an image concerns me.

Imarok added a comment.EditedAug 9 2017, 5:01 PM

Works on Ubuntu Gnome, but is quite small.
Looks ok, when cutting all the unused space away (I removed the borders, so that its 128x128)

ffffffff accepted this revision.Aug 15 2017, 3:45 PM
ffffffff added a reviewer: ffffffff.

works kubuntu 17.04 (kde plasma)

This revision is now accepted and ready to land.Aug 15 2017, 3:46 PM
ffffffff added a comment.EditedAug 15 2017, 7:28 PM

Initially (before @vladislavbelov attached the icon image file separately) this patch caused crash on pyrogenesis start for me:

tex_codec.cpp(70): Function call failed: return value was -120101 (Unknown error (-120101, 0xFFFFFFFFFFFE2ADB))
Function call failed: return value was -120101 (Unknown error (-120101, 0xFFFFFFFFFFFE2ADB))
Location: tex_codec.cpp:70 (tex_codec_for_header)

This seems to be down to arc not acquiring the new image from phabricator properly, causing the image file to... not be an image file.

Also note that if the image doesn't exist at all, I get a different error message:

terminate called after throwing an instance of 'PSERROR_System_VmodeFailed'
  what():  System_VmodeFailed
Aborted (core dumped)

After manually copying the attached image into the appropriate location, the patch works fine (Arch Linux).
However, the fact that 0ad crashes at start if the image file doesn't exist or isn't an image concerns me.

this is true we should add seperate function to load that file, that can fail but sdl init can continue

ffffffff requested changes to this revision.Aug 15 2017, 7:29 PM
This revision now requires changes to proceed.Aug 15 2017, 7:29 PM

this is true we should add seperate function to load that file, that can fail but sdl init can continue

Why we should continue if the icon wasn't found?

vladislavbelov edited edge metadata.

Do not fail SDL loading if the window icon not found.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1856/ for more details.

This revision is now accepted and ready to land.Aug 15 2017, 10:05 PM
s0600204 accepted this revision as: s0600204.Aug 28 2017, 8:52 PM

Much better.

This revision was automatically updated to reflect the committed changes.