Opened 3 years ago

Closed 19 months ago

Last modified 14 months ago

#3113 closed enhancement (fixed)

[PATCH] Provide information about carried resources of a selection

Reported by: Kalle Richter Owned by: Imarok
Priority: Nice to Have Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

It'd be nice to be an overview of which resources (and eventually how much of it) is carried in sum by a selection of units. This would serve to avoid sending a group of units carrying a valuable amount of resources of one type to collect another type and thus dropping the carried resources.

Attachments (9)

total_carried_resources_v1.patch (2.5 KB) - added by Imarok 19 months ago.
Shows the carried resources of the whole selection as a tooltip of the total number of units in the selection
total_carried_resources_v1.1.patch (2.5 KB) - added by Imarok 19 months ago.
used BARTER_RESOURCES instead of defining it again
total_carried_resources_v1.2.patch (2.4 KB) - added by Imarok 19 months ago.
Now using RESOURCES
total_carried_resources_v1.3.patch (2.4 KB) - added by Imarok 19 months ago.
removed one parenthesis
total_carried_resources_v1.4.patch (2.4 KB) - added by Imarok 19 months ago.
merged line 392 and 393
total_carried_resources_v1.4.1.patch (2.4 KB) - added by Imarok 19 months ago.
Updated to the new revision
total_carried_resources_v1.5.patch (2.4 KB) - added by Imarok 19 months ago.
removed brackets
total_carried_resources_v1.6.patch (8.2 KB) - added by Imarok 19 months ago.
rename getCostComponentDisplayName to getCostComponentDisplayIcon
total_carried_resources_v1.7.1.patch (6.3 KB) - added by Imarok 19 months ago.
followed the suggestions of comment:17

Download all attachments as: .zip

Change History (30)

comment:1 Changed 3 years ago by sanderd17

Component: Core engineUI & Simulation
Keywords: simple added

comment:2 Changed 3 years ago by mimo

You already have that info in the tooltips of the unit icons of the central panel. I don't know if that is what you wanted, or if this ticket is specifically to add the sum on the different groups ? which I'm not sure it is that useful :-)

comment:3 Changed 19 months ago by Imarok

Owner: set to Imarok

Changed 19 months ago by Imarok

Shows the carried resources of the whole selection as a tooltip of the total number of units in the selection

comment:4 Changed 19 months ago by Imarok

Keywords: review patch added
Milestone: BacklogAlpha 21

comment:5 Changed 19 months ago by Imarok

Summary: Provide information about carried resources of a selection[PATCH] Provide information about carried resources of a selection

Changed 19 months ago by Imarok

used BARTER_RESOURCES instead of defining it again

Changed 19 months ago by Imarok

Now using RESOURCES

Changed 19 months ago by Imarok

removed one parenthesis

comment:6 Changed 19 months ago by elexis

Keywords: simple review removed
  • As getCostComponentDisplayName returns an icon, then concatenation is ok with i != 3.
  • You copy & pasted that code from selection_panels.js. Better move that code to a function and use that.
  • getCostComponentDisplayName should be renamed, as it returns an icon, not names. Can be done in a separate commit.

comment:7 Changed 19 months ago by Lionkanzen

Talk me if you need a new icon, guys.

comment:8 in reply to:  7 ; Changed 19 months ago by elexis

Replying to Lionkanzen:

Talk me if you need a new icon, guys.

Completely unrelated, but the fishing net was discussed to get a new icon (the one that looks like a spooky ghost). Might want to ask mimo / sanderd17 / forum about further opinion.

comment:9 in reply to:  8 Changed 19 months ago by Lionkanzen

Replying to elexis:

Replying to Lionkanzen:

Talk me if you need a new icon, guys.

Completely unrelated, but the fishing net was discussed to get a new icon (the one that looks like a spooky ghost). Might want to ask mimo / sanderd17 / forum about further opinion.

Sorry, but I don't know if you guys needs a icons, for the description of the problem may be need symbol ( capacity symbol like a cart) isn't my intention Elexis bother anyone here. Ok this is the last time. And sorry too much.

comment:10 Changed 19 months ago by elexis

No problem Lionkanzen! We appreciate your offers. Better ask and get rejected rather than not asking when we could actually use your help.

For this ticket we use the existing food / wood / metal / stone icons. I will ask mimo if we could use an improvement of http://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/art/textures/ui/session/portraits/technologies/fishing_net.png

comment:11 Changed 19 months ago by sanderd17

Small comment on the code: you should try to avoid relying on the knowledge that there are 4 resources, and they are called these 4 names. If we ever want to add or remove a resource, it will already be a PITA to search all the places where the names are used. So better not add new places like these.

comment:12 in reply to:  11 Changed 19 months ago by Imarok

Replying to sanderd17:

Small comment on the code: you should try to avoid relying on the knowledge that there are 4 resources, and they are called these 4 names. If we ever want to add or remove a resource, it will already be a PITA to search all the places where the names are used. So better not add new places like these.

Where in the patch do I rely on the knowledge there are 4 resources?

Changed 19 months ago by Imarok

merged line 392 and 393

Changed 19 months ago by Imarok

Updated to the new revision

comment:13 Changed 19 months ago by elexis

Please fix the second point of comment:6

Changed 19 months ago by Imarok

removed brackets

comment:14 in reply to:  13 Changed 19 months ago by Imarok

Replying to elexis:

Please fix the second point of comment:6

I thought this should be done in a seperate comment

comment:15 Changed 19 months ago by Imarok

I'll add it

Changed 19 months ago by Imarok

rename getCostComponentDisplayName to getCostComponentDisplayIcon

comment:16 Changed 19 months ago by Imarok

Keywords: review added

comment:17 Changed 19 months ago by elexis

Keywords: review removed
  • 2 revisions changed that code, the patch doesn't apply anymore
  • 369-372 can be merged into a single line using totalResourcesCarried[carrying.type] = (totalResourcesCarried[carrying.type] || 0) + carrying.amount
  • the substr call is ugly and could be replaced with trim'. Better avoid adding that unneeded space in the first place and replace the whole loop, if and trimming with Object.keys(totalResourcesCarried).map(res => getCostComponentDisplayIcon(res) + totalResourcesCarried[res]).join(",")`
  • this allows allows to init totalResourcesCarried to {} immediately and removing the if ( totalResourcesCarried) check
  • .length !== 0 only needs != 0

Changed 19 months ago by Imarok

followed the suggestions of comment:17

comment:18 Changed 19 months ago by Imarok

Keywords: review added

comment:19 Changed 19 months ago by elexis

Resolution: fixed
Status: newclosed

In 18154:

Display the carried resources when selecting a group of units. Patch by Imarok, fixes #3113.
Shows the information for all selected units and for all units of a given type.
Rename g_CostDisplayNames to g_CostDisplayIcons.

comment:20 Changed 19 months ago by elexis

Keywords: review removed

Thanks for the patch. I changed few characters:

-whitespace around Object.keys -length != 0 -> length -str -> tooltip

Your code assumes there will be only one type of resource carried per unit. Since thats actually the case, likely won't change soon and if it would, it would be easy to identify by searching for resourceCarrying, it is acceptable.

comment:21 Changed 14 months ago by elexis

In 18763:

Order the carried resources as usual. Patch by Imarok, refs #3113.

Note: See TracTickets for help on using tickets.