Opened 3 years ago

Closed 17 months ago

Last modified 17 months ago

#3216 closed enhancement (fixed)

[PATCH] Add capture statistics to summary screen

Reported by: sanderd17 Owned by: Imarok
Priority: Should Have Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

Since capturing in SVN, the summary screen with the building stats isn't of big value anymore.

It would be better if the captured and lost buildings would also be shown (in case of lack of space, they could be added to the created and destroyed buildings).

Attachments (28)

3216.patch (7.3 KB) - added by maveric 3 years ago.
3216.2.patch (7.2 KB) - added by maveric 3 years ago.
3216.3.patch (7.2 KB) - added by maveric 3 years ago.
pict.PNG (661.5 KB) - added by maveric 3 years ago.
pict2.PNG (661.2 KB) - added by maveric 3 years ago.
This color seems much better for me.
3216.4.patch (7.4 KB) - added by maveric 3 years ago.
pict3.PNG (662.9 KB) - added by maveric 3 years ago.
pict4.PNG (663.0 KB) - added by maveric 3 years ago.
pict5.PNG (264.3 KB) - added by maveric 3 years ago.
pict6.PNG (297.8 KB) - added by maveric 3 years ago.
pict7.PNG (299.3 KB) - added by maveric 3 years ago.
pict8.PNG (383.0 KB) - added by maveric 3 years ago.
pict9.PNG (373.9 KB) - added by maveric 3 years ago.
pict10.PNG (362.9 KB) - added by maveric 3 years ago.
3216.5.patch (7.8 KB) - added by maveric 3 years ago.
3216.6.patch (5.5 KB) - added by maveric 3 years ago.
pict11.PNG (286.5 KB) - added by maveric 3 years ago.
3216.7.patch (16.6 KB) - added by maveric 3 years ago.
3216.8.patch (16.9 KB) - added by maveric 3 years ago.
3216.9.patch (18.5 KB) - added by maveric 3 years ago.
pict12.PNG (279.2 KB) - added by maveric 3 years ago.
t3216_capture_statistics_v10.patch (21.7 KB) - added by elexis 2 years ago.
The unalignment is caused by #3351. This patch works around the issue by adding another newline in counters.js. Also uses let instead of var in some places and replaces the deprecated "for each" loop with "for ... in".
t3216_capture_statistics_style.patch (3.3 KB) - added by Imarok 18 months ago.
t3216_capture_statistics_v10_rebasedv2.patch (19.9 KB) - added by Imarok 18 months ago.
rebased the patch. Not ready for reviewing
t3216_capture_statistics_v11.1.patch (22.4 KB) - added by Imarok 18 months ago.
Ready to commit. But I don' know if I updated the GUIInterface test correctly
t3216_capture_statistics_v11.2.patch (23.6 KB) - added by Imarok 18 months ago.
some wrong indentation, move the regex used twice into a seperate function
t3216_capture_statistics_v11.3.patch (22.7 KB) - added by Imarok 18 months ago.
Remove all regex in one function. Some stylefix
t3216_capture_statistics_v11.4.patch (22.8 KB) - added by Imarok 18 months ago.
Put more code into the regex function

Change History (65)

comment:1 Changed 3 years ago by maveric

Owner: set to maveric
Status: newassigned

comment:2 Changed 3 years ago by maveric

Milestone: BacklogAlpha 19

Changed 3 years ago by maveric

Attachment: 3216.patch added

comment:3 Changed 3 years ago by maveric

The color might be not suitable (just chose random one).

Changed 3 years ago by maveric

Attachment: 3216.2.patch added

comment:4 Changed 3 years ago by sanderd17

Do you have a screenshot to show what's changed?

Did you also take localisations and supported screen resolution into account? Some languages will need more space than English.

Also don't forget to add the needed keywords when you think it's ready for review, see SubmittingPatches.

comment:5 Changed 3 years ago by maveric

Keywords: patch review added
Summary: Add capture statistics to summary screen[PATCH] Add capture statistics to summary screen

Changed 3 years ago by maveric

Attachment: 3216.3.patch added

Changed 3 years ago by maveric

Attachment: pict.PNG added

comment:6 in reply to:  3 Changed 3 years ago by elexis

Replying to maveric:

The color might be not suitable (just chose random one).

See #3205

Changed 3 years ago by maveric

Attachment: pict2.PNG added

This color seems much better for me.

Changed 3 years ago by maveric

Attachment: 3216.4.patch added

Changed 3 years ago by maveric

Attachment: pict3.PNG added

comment:7 in reply to:  4 Changed 3 years ago by maveric

Replying to sanderd17:

Did you also take localisations and supported screen resolution into account? Some languages will need more space than English.

The last screenshot was taken in window mode with 1024x768 resolution and it looks fine I think. I didn't find the way to change it to lesser one in game settings, so I think it's okay with that. Also changing localization doesn't affects on the width of the columns in summary tabs (i.e. "Total", "Houses" etc.), so it's okay too.

comment:8 Changed 3 years ago by leper

How does it look after actually playing for a while? I have the suspicion that numbers > 10 for at least one of the values will create an issue with the available space.

Changed 3 years ago by maveric

Attachment: pict4.PNG added

comment:9 Changed 3 years ago by maveric

Although with one value > 10 it looks better.

Changed 3 years ago by maveric

Attachment: pict5.PNG added

comment:10 Changed 3 years ago by maveric

Well, I can reduce player name column.

Changed 3 years ago by maveric

Attachment: pict6.PNG added

comment:11 Changed 3 years ago by maveric

What do you think about this?

comment:12 Changed 3 years ago by leper

I guess having 2 on each line would be better

Changed 3 years ago by maveric

Attachment: pict7.PNG added

comment:13 Changed 3 years ago by qwertz

I would remove the second slash and I would use the colour that is used for the actual numbers in the heading (green for Constructed, red for Lost etc.). I'm however noone that can decide if this is really better :-P

Changed 3 years ago by maveric

Attachment: pict8.PNG added

Changed 3 years ago by maveric

Attachment: pict9.PNG added

comment:14 Changed 3 years ago by maveric

Colored heading looks great, but I wouldn't remove the second slash.

comment:15 Changed 3 years ago by qwertz

Why aren't the two lines identically indented in pict9? I think it would look better if the values for Destroyed and Constructed would start with the same indentation.

comment:16 Changed 3 years ago by trompetin17

IMO would be more interesting if you have

Constructed/Destoyed? Capture/Lost?

comment:17 Changed 3 years ago by trompetin17

btw could you maveric come to IRC #0ad-dev?

Changed 3 years ago by maveric

Attachment: pict10.PNG added

comment:18 in reply to:  17 Changed 3 years ago by maveric

Here is the version with same (almost) indentation and changed order as trompetin17 proposed.

Replying to trompetin17:

btw could you maveric come to IRC #0ad-dev?

Yeah, sure.

Last edited 3 years ago by maveric (previous) (diff)

comment:19 Changed 3 years ago by elexis

I like it, good work!

comment:20 Changed 3 years ago by trompetin17

Could you upload your current patch(with identation) i will review this ticket

Changed 3 years ago by maveric

Attachment: 3216.5.patch added

Changed 3 years ago by maveric

Attachment: 3216.6.patch added

comment:21 Changed 3 years ago by maveric

I excluded changes in StatisticsTracker? from patch (not sure whether I got what you meant about recommendations).

Changed 3 years ago by maveric

Attachment: pict11.PNG added

Changed 3 years ago by maveric

Attachment: 3216.7.patch added

Changed 3 years ago by maveric

Attachment: 3216.8.patch added

Changed 3 years ago by maveric

Attachment: 3216.9.patch added

comment:22 Changed 3 years ago by maveric

Added colored headings for all panels.

Changed 3 years ago by maveric

Attachment: pict12.PNG added

comment:23 Changed 3 years ago by maveric

Here is screenshot.

comment:24 Changed 3 years ago by leper

Can you fix the unalignment of the two lines?

comment:25 Changed 2 years ago by leper

Any updates?

comment:26 in reply to:  25 Changed 2 years ago by maveric

Replying to leper:

Any updates?

I was a little busy, so didn't work on the patch. Also don't expect updates soon, because I have to prepare for the exams at university.

comment:27 Changed 2 years ago by leper

Ok, good luck for your exams.

comment:28 in reply to:  24 Changed 2 years ago by elexis

Replying to leper:

Can you fix the unalignment of the two lines?

The only problem with this is that the second row of each player has a like 2 pixel offset to the right?

Changed 2 years ago by elexis

The unalignment is caused by #3351. This patch works around the issue by adding another newline in counters.js. Also uses let instead of var in some places and replaces the deprecated "for each" loop with "for ... in".

comment:29 Changed 2 years ago by elexis

Keywords: review removed

Mentioned by leper in irc on August 1st:

(21:35:54) leper: Why does that use c,l,t and captured,lost,destroyed in some places that do nearly the same (21:36:05) leper: also move that one regex thingy to some function (21:37:04) leper: it also does something with tooltip_style in the summary screen (21:37:17) leper: might that revert the fix for that one ticket? (#3072) (21:38:21) leper: StatisticsTracker.prototype.CapturedBuilding also doesn't check if components are present before doing anything

Mentioned by Itms today:

(16:09:16) Itms: you will have to update the GUIInterface tests (16:09:52) Itms: because the Statistics tracking will be saved in the extended simulation state

comment:30 Changed 2 years ago by stanislas69

Milestone: Alpha 19Alpha 20

Pushing as neither I or elexis will have time to work on it before FF.

comment:31 Changed 2 years ago by leper

Any updates?

comment:32 Changed 22 months ago by elexis

Milestone: Alpha 20Backlog

Backlogging due to lack of progress.

comment:33 Changed 18 months ago by Imarok

Owner: changed from maveric to Imarok
Status: assignednew

Changed 18 months ago by Imarok

comment:34 Changed 18 months ago by Imarok

Keywords: review added

Changed 18 months ago by Imarok

rebased the patch. Not ready for reviewing

Changed 18 months ago by Imarok

Ready to commit. But I don' know if I updated the GUIInterface test correctly

Changed 18 months ago by Imarok

some wrong indentation, move the regex used twice into a seperate function

Changed 18 months ago by Imarok

Remove all regex in one function. Some stylefix

Changed 18 months ago by Imarok

Put more code into the regex function

comment:35 Changed 17 months ago by elexis

Resolution: fixed
Status: newclosed

In 18395:

Capture statistics summary. Patch by maveric and Imarok, fixes #3216.

Also colorize the headings, use two lines per player and remove duplication of an ugly regex hack.

comment:36 Changed 17 months ago by elexis

Keywords: simple review removed
Milestone: BacklogAlpha 21

Thanks, that one was needed in alpha 19 and 20 already.

  • rewrote the statistics tracker part to not cancel if there are no costs
  • formatCaptured(trained, killed, captured, lost) -> buildings are not trained
  • added comment for the inclusion change
  • missing semicolon
  • if(
  • trailing whitespace
  • multiple consecutive spaces in xml
  • those names like playerNamet should be renamed

comment:37 Changed 17 months ago by elexis

In 18397:

Missing semicolon, rephrase comment, refs #3216.

Note: See TracTickets for help on using tickets.