Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4187 closed defect (fixed)

[PATCH] Summary screen must show who has actually won / was defeated

Reported by: elexis Owned by: javiergodas
Priority: Must Have Milestone: Alpha 21
Component: UI & Simulation Keywords: simple patch
Cc: Patch:

Description

Sometimes a game ends and it isn't obvious who won. Often some players are defeated and the host quits the game before a winner has been declared officially.

The summary screen should show which players have been defeated and which won.

It would be handy to also capture and display the time of defeat (this information could also be reused when saving games, since the load-game dialog shows which players have been defeated too).

Attachments (7)

WinLosePatch.diff (4.8 KB ) - added by javiergodas 8 years ago.
Update (thx elexis for the review)
WinLose_fixed_whitespace.patch (5.3 KB ) - added by elexis 8 years ago.
Fixed whitespace issues.
winlose.png (152.8 KB ) - added by elexis 8 years ago.
Current state
sample.jpg (194.4 KB ) - added by elexis 8 years ago.
WinLosePatchV2.diff (4.6 KB ) - added by javiergodas 8 years ago.
WinLosePatchLaurels.diff (4.3 KB ) - added by javiergodas 8 years ago.
WinLosePatch_swords.diff (5.5 KB ) - added by elexis 8 years ago.
Fixed so much broken whitespace.... Use tabs instead of spaces and don't have space after the last character please! Patch uses swords icons as suggested by niektb.

Download all attachments as: .zip

Change History (26)

comment:1 by javiergodas, 8 years ago

Owner: set to javiergodas

comment:2 by elexis, 8 years ago

Keywords: patch rfc added
Milestone: BacklogAlpha 21
Summary: Summary screen must show who has actually won / was defeated[PATCH] Summary screen must show who has actually won / was defeated

Thanks for working on this!

  • Please add your name to the programming.json file, so you are mentioned in the credits.
  • Use correct indentation and tabs.
  • Rename playerOutcomet to playerOutcome
  • Use translate("Defeated") and translate("Victory") so that it will be translated into other languages
  • For "active" players, don't display anything (empty tooltip and sprite)
  • unneeded variable outcomeState, you can just reuse playerState

Other than that looks good to me.

by javiergodas, 8 years ago

Attachment: WinLosePatch.diff added

Update (thx elexis for the review)

comment:3 by Vladislav Belov, 8 years ago

There are still few indentation notes: tabs and spaces between "if(". And probably better to use "Won" instead of "Victory" as opposition to "Defeated", as in other places,

Version 1, edited 8 years ago by Vladislav Belov (previous) (next) (diff)

by elexis, 8 years ago

Fixed whitespace issues.

comment:4 by elexis, 8 years ago

The summary screen is alrady too filled with 1024x768, so perhaps we might want to commit the patch above that replaces the colored playerbox with the playerstate-icon.

But just for comparison, can you show us how the summary screen would look like if we had the icons right beside the playercolor box?

comment:5 by javiergodas, 8 years ago

I fear I can't cause I tried to place the outcome icons under another header, but no matter how I tried, I couldn't increase the stats colored box width. Came to this idea because of that problem: just replacing the colored box.

But sure I'll try to do that.

Thx vladislav for the whitespace fix, sry for not doing it myself.

comment:6 by elexis, 8 years ago

Correct me if I'm wrong, perhaps it's sufficient to

1) add the new lines of code added in the patch above attachment:WinLose_fixed_whitespace.patch 2) not remove the lines removed by that patch?

i.e. don't delete the old object. Just move it a bit to the left or right. Alternatively one could use an image editor to just get a preview of how it might look. I might try that later.

Also you had sent some screenshots in IRC (see also http://irclogs.wildfiregames.com/), can you link them here?

by elexis, 8 years ago

Attachment: winlose.png added

Current state

by elexis, 8 years ago

Attachment: sample.jpg added

comment:7 by elexis, 8 years ago

Current state of the patch: http://trac.wildfiregames.com/raw-attachment/ticket/4187/winlose.png

Was thinking of this (created using image editor): http://trac.wildfiregames.com/raw-attachment/ticket/4187/sample.jpg

I checked with 1024x768 and large names. The playername column doesn't become smaller (on the other hand the units tab panel won't work with 1024 already, so it will need scrolling or a new page). Long names use multiple lines and never collide with anything (rather don't show the complete name).

But to be honest I don't think we will miss that color sprite, it just seems redundant.

comment:8 by Itms, 8 years ago

The icons used here are not very consistent (they're not the same size and they don't use the same symbols), plus it's rather unclear that the crossed swords mean victory.

I would rather use ps/trunk/binaries/data/mods/public/art/textures/ui/session/icons/stances/passive.png for losing and ps/trunk/binaries/data/mods/public/art/textures/ui/session/icons/stances/violent.png for winning. The symbol is clear, the icons are coherent, and you have a clear opposition between swords towards the sky or towards the ground.

comment:9 by elexis, 8 years ago

And we settled on having both a colorbox and the sword icons. So don't remove the old code, only add new code (and potentially resize).

comment:10 by javiergodas, 8 years ago

Hi elexis,

About the size of the icons, I can search something fitting for win/lose icons in OpenGameArt website.

I'll try to keep the color box, but before that I have to figure out how to expand the player box.

I told you I'ld upload the screenshots myself but thx anyway.

comment:11 by javiergodas, 8 years ago

Found this icon for Victory:

http://opengameart.org/content/laurels

comment:12 by javiergodas, 8 years ago

Hi guys,

I've been tampering with the size attribute like I knew what I was doing, and as a result (since you decided to keep the color box along with the outcome), I've created some kind of hybrid that you might like.

http://s18.postimg.org/sozwolpl5/Win_Lose_V2.png

New patch is on the attachments section as V2.

by javiergodas, 8 years ago

Attachment: WinLosePatchV2.diff added

comment:13 by javiergodas, 8 years ago

Could also reuse the laurels png to mix it with champion recruitment slots background so you don't have to look at its types to see it's a champion.

Last edited 8 years ago by javiergodas (previous) (diff)

comment:14 by javiergodas, 8 years ago

"A persian anusya with laurels on its recruitment png for example".

Last edited 8 years ago by javiergodas (previous) (diff)

comment:15 by Itms, 8 years ago

Hi, I'm sorry but I really don't like at all this mix you propose :/

  • The win/lose icons are ugly when put on top of this solid color.
  • You're mixing two things that have nothing to do with each other. In your screenshot it is completely impossible to understand the color is the player color.
  • The solid color is disgusting, you need to keep the border around.

Can you rather try to do what elexis proposed? The laurels are nicer than the 'aggressive' swords in any case. Keep up the good work!

comment:16 by javiergodas, 8 years ago

Tried to do the thing that elexis said since the beggining, but there is a problem with the XML (quite confusing) size attribute that gets the sprites distorted when you try to move them, so either they mix with the colorbox, or they get cut.

Also tried to modify the layout.js, tried everything I saw to increase the player box but the image didn't got any better.

So unfortunately ATM that's the best I can do to make it look good.

Modified the first patch to include the laurels. I know you agreed on keeping the colored box, but the background still does the trick, and as I told, I didn't find a way to keep both things.

http://s21.postimg.org/is0035u8n/Win_Lose_Laurels.png

by javiergodas, 8 years ago

Attachment: WinLosePatchLaurels.diff added

by elexis, 8 years ago

Attachment: WinLosePatch_swords.diff added

Fixed so much broken whitespace.... Use tabs instead of spaces and don't have space after the last character please! Patch uses swords icons as suggested by niektb.

comment:17 by elexis, 8 years ago

http://i.imgur.com/SjOeWi0.png

Background player color opacity increased, so its more visible. Icons as suggested by Itms, niektb not a huge fan of, I have no strong opinion on the icon.

comment:18 by elexis, 8 years ago

Resolution: fixed
Status: newclosed

In 18771:

Show an icon with tooltip in the summary screen indicating which player was defeated, has won or is still playing. Patch by javiergodas, reviewed by niektb, fixes #4187.

comment:19 by elexis, 8 years ago

Keywords: rfc removed

Thanks for the patch and quick updates :)

Note: See TracTickets for help on using tickets.