Opened 18 months ago

Closed 14 months ago

Last modified 14 months ago

#4033 closed defect (fixed)

[PATCH] Siege Engines are not shown in the summary

Reported by: Imarok Owned by: Mate-86
Priority: Should Have Milestone: Alpha 21
Component: UI & Simulation Keywords: simple patch
Cc: Patch:

Description (last modified by Mate-86)

  1. Siege Engines should be listed in the summary
  2. The captured statistic for Siege Engines should be displayed/created too

http://trac.wildfiregames.com/raw-attachment/ticket/4033/captured-building.png

http://trac.wildfiregames.com/raw-attachment/ticket/4033/military-score.png

team summary fixed: http://trac.wildfiregames.com/raw-attachment/ticket/4033/teams_multiline.png

Attachments (10)

captured-units.png (616.8 KB) - added by Mate-86 15 months ago.
captured-building.png (608.3 KB) - added by Mate-86 15 months ago.
military-score.png (398.4 KB) - added by Mate-86 15 months ago.
siege-units-summary.patch (13.1 KB) - added by Mate-86 15 months ago.
v2
units-summary-fixed.png (485.4 KB) - added by Mate-86 15 months ago.
siege-units-summary.2.patch (13.7 KB) - added by Mate-86 15 months ago.
team-summary.png (824.5 KB) - added by Mate-86 15 months ago.
siege-units-summary.3.patch (14.0 KB) - added by Mate-86 15 months ago.
siege-units-summary.4.patch (13.9 KB) - added by Mate-86 15 months ago.
teams_multiline.png (859.5 KB) - added by Mate-86 15 months ago.

Change History (28)

comment:1 Changed 15 months ago by Mate-86

Owner: set to Mate-86
Status: newassigned

Changed 15 months ago by Mate-86

Attachment: captured-units.png added

Changed 15 months ago by Mate-86

Attachment: captured-building.png added

Changed 15 months ago by Mate-86

Attachment: military-score.png added

comment:2 Changed 15 months ago by Mate-86

feedback from bb_:

  • captureable summary for Siege units and Total only => fixed
  • horizontal ordering of unit types could be changed to "... siege, naval, trader" => fixed
  • need to double check values printed (captured are marked as lost for the other player) => fixed
  • units and building headers need to be in the same order => fixed
Last edited 15 months ago by Mate-86 (previous) (diff)

comment:3 Changed 15 months ago by Mate-86

Keywords: rfc patch added
Summary: Siege Engines are not shown in the summary[PATCH] Siege Engines are not shown in the summary

comment:4 Changed 15 months ago by elexis

Description: modified (diff)
Milestone: BacklogAlpha 21

Changed 15 months ago by Mate-86

Attachment: siege-units-summary.patch added

v2

Changed 15 months ago by Mate-86

Attachment: units-summary-fixed.png added

comment:5 Changed 15 months ago by Mate-86

Description: modified (diff)

comment:6 Changed 15 months ago by Mate-86

Some open points:

  • not sure whether I should update some unit tests for JS?
  • I've modified counter.js:calculateUnitsTeam as well but I don't know how to test it. Should I setup a match with teams?
  • There is a slight misalignment on my latest screenshot. I may need help how to align the counters to the center.

comment:7 in reply to:  6 Changed 15 months ago by elexis

Replying to Mate-86:

not sure whether I should update some unit tests for JS?

We don't have tests for the GUI yet and it's not the purpose of this ticket to lie the foundation to that.

I've modified counter.js:calculateUnitsTeam as well but I don't know how to test it. Should I setup a match with teams?

Yes

There is a slight misalignment on my latest screenshot. I may need help how to align the counters to the center.

Been there, #3351, nuke the trailing newline probably.

Changed 15 months ago by Mate-86

Attachment: siege-units-summary.2.patch added

comment:8 Changed 15 months ago by Mate-86

I've uploaded a new patch. Removing the newline has solved the problem but I could not check the team summary page even if I set up some teams. If no objection I'd move the ticket to review queue.

comment:9 Changed 15 months ago by Imarok

Have you set Locked Teams to true?

comment:10 Changed 15 months ago by Mate-86

Thx, now I get the team summary as well. Need to apply some further fixes tomorrow.

Changed 15 months ago by Mate-86

Attachment: team-summary.png added

Changed 15 months ago by Mate-86

Attachment: siege-units-summary.3.patch added

comment:11 Changed 15 months ago by Mate-86

Description: modified (diff)

Team summary fixed as well, screenshot and latest patch attached.

comment:12 Changed 15 months ago by bb

counters.js L235-6 trailing whitespace + 1 newline is enough. L240, L243, L252, L254, L259: trailing whitespace

StatisticsTracker?.js L352: trailing whitespace L355 indentation error

When the values start to get bigger e.g. get more digits the 4 values does not fit anymore on the one line. The lost vale will then fall of the line to the next line but it is only half visible then. It might be nice to split out the values over different lines as is done in the structure tab.

Changed 15 months ago by Mate-86

Attachment: siege-units-summary.4.patch added

Changed 15 months ago by Mate-86

Attachment: teams_multiline.png added

comment:13 Changed 15 months ago by Mate-86

Description: modified (diff)

comment:14 Changed 15 months ago by Mate-86

Description: modified (diff)

comment:15 Changed 15 months ago by Mate-86

New solution (screenshot + patch) uploaded. Whitespace issues should be fixed as well.

comment:16 Changed 15 months ago by bb

Keywords: review added; rfc removed

counters.js L251 trailing whitespace

Further is ok so pushing to review queue.

comment:17 Changed 14 months ago by elexis

Resolution: fixed
Status: assignedclosed

In 18780:

Siege statistics for the summary screen. Patch by Mate-86, fixes #4033.

comment:18 Changed 14 months ago by elexis

Keywords: review removed

Thanks for the patch!

Note: See TracTickets for help on using tickets.