Opened 13 years ago

Closed 3 years ago

Last modified 3 years ago

#643 closed enhancement (fixed)

[PATCH] Add current gatherers count next to player's resource count

Reported by: Kieran P Owned by: Freagarach
Priority: Nice to Have Milestone: Alpha 24
Component: UI – In-game Keywords: patch
Cc: Patch: Phab:D3155

Description (last modified by Nescio)

Add the amount of workers in a number next to the current amount of that resource, along with a tooltip.

e.g. If user has 5000 food, and 10 people gathering food (from any source), show

[food_icon] 5000 / 10

And when user hovers over the amounts, both should display a tooltip with:

Food resource: 5000
Number of gatherers: 10

Do the same for all resource types.

Attachments (5)

Patch643.patch (9.8 KB ) - added by Matt Doerksen 12 years ago.
gatherer-count.patch (11.2 KB ) - added by Deiz 12 years ago.
gatherer-count-2.patch (11.0 KB ) - added by Deiz 12 years ago.
gatherer-count-3.patch (13.2 KB ) - added by Deiz 12 years ago.
gatherer-count-11052012.patch (13.3 KB ) - added by historic_bruno 11 years ago.

Download all attachments as: .zip

Change History (53)

comment:1 by Kieran P, 13 years ago

Description: modified (diff)

comment:2 by Kieran P, 13 years ago

From IRC:

Brian: brackets might be better if there is room (since the slash is usually used to represent a fraction). Either could work though.

comment:3 by Kieran P, 13 years ago

Milestone: Alpha 3Alpha 4

comment:4 by Kieran P, 13 years ago

Priority: majorminor

comment:5 by Kieran P, 13 years ago

Alternatively, we do something like AoK and have a button that toggles the display of them, as we'll have builders we probably want to display a count for, but don't have an icon at the top to add a tooltip for construction.

comment:6 by Kieran P, 13 years ago

Owner: Brian removed

comment:7 by Kieran P, 13 years ago

Milestone: Alpha 4Alpha 5

comment:8 by Kieran P, 13 years ago

Milestone: Alpha 5Alpha 6

comment:9 by Kieran P, 13 years ago

Component: Game engineSimulation
Milestone: Alpha 6Backlog
Priority: Nice to HaveIf Time Permits

comment:10 by gerbilOFdoom, 12 years ago

Owner: set to gerbilOFdoom
Status: newassigned

Alright, UI modification... seems to be the limit of my abilities so far. 

by Matt Doerksen, 12 years ago

Attachment: Patch643.patch added

comment:11 by Matt Doerksen, 12 years ago

Keywords: patch review added
Milestone: BacklogAlpha 10
Owner: changed from gerbilOFdoom to Matt Doerksen
Status: assignednew
Summary: Add current gatherers count next to resource count[PATCH] Add current gatherers count next to resource count

Modified the GUI to include the number of people gathering each resource. Correctly assigns/unassigns when changing to gathering other resources and handles other commands. E.g. Assigning them to build removes them from the count of whatever they're gathering. Correctly handles the situation where a resource runs out and the units become idle.

TODO: look at potential efficiency issues since it currently gets all of a player's units and cycles through them instead of filtering them directly by workers (I don't know if such functionality exists in the engine, I didn't see anything). It could become expensive if there are a lot of units since this runs every time updatePlayerDisplay(simState) in session.js is executed.

EDIT: This patch should be used in conjunction with Patch 1191 since they both modify session.xml. Alternatively, you can use the latest xml file which should result in the same behavior but the mouse over text for each resource will not be updated.

Last edited 12 years ago by Matt Doerksen (previous) (diff)

comment:12 by Jonathan Waller, 12 years ago

I have had a quick look at your current patch.

Scanning the entire list of entities does seem inefficient. It would be better if you used messages which would be sent every time a unit started or stopped gathering a resource. This is currently being done for the unit's idle status in UnitAi (line 635). Note that you would need to send the message to the player entity if you want to pick it up in GUIInterface. You would have a counter in GUIInterface which would keep rtack of each resource type.

For actually working out whether a unit is gathering a resource I don't think your current code is working how players expect. Units should still be counted as gathering a resource when walking to the dropsite and back.

Also I think it would be better to put the starting/stopping gathering logic into UnitAI.js rather than ResourceGatherer.js and Commands.js. You can use the enter/leave functions to look for changes.

comment:13 by Kieran P, 12 years ago

Keywords: review removed
Milestone: Alpha 10Alpha 11

in reply to:  12 comment:14 by Matt Doerksen, 12 years ago

Replying to quantumstate:

I have had a quick look at your current patch.

Scanning the entire list of entities does seem inefficient. It would be better if you used messages which would be sent every time a unit started or stopped gathering a resource. This is currently being done for the unit's idle status in UnitAi (line 635). Note that you would need to send the message to the player entity if you want to pick it up in GUIInterface. You would have a counter in GUIInterface which would keep rtack of each resource type.

For actually working out whether a unit is gathering a resource I don't think your current code is working how players expect. Units should still be counted as gathering a resource when walking to the dropsite and back.

Also I think it would be better to put the starting/stopping gathering logic into UnitAI.js rather than ResourceGatherer.js and Commands.js. You can use the enter/leave functions to look for changes.

I did a little bit of (ugly) profiling on my machine (i7-720QM, max turbo of 2.8 GHz) and it looks like it only takes 0-2 (avg. 0-1) ms per call when I had about 200 units so it actually might not be too bad, even on a low power machine. I'm not sure if you saw the forum thread, but I had talked with another member about trying to not use a static count since it would be fairly easy to miss the edge cases (die, convert, ...) and could add a lot of complexity (given the solution seems to not be too inefficient in terms of CPU time). If we could limit the number of calls to every 1-2 seconds (or whatever is adequate) then it actually would be pretty easy to amortize the cost.

And regarding your last point, when I tested the code, it did count the workers as gathering while they were walking back to deposit resources. I'll double check this and post back if they don't stay in the count.

comment:15 by Matt Doerksen, 12 years ago

Just following up on this, it does correctly keep workers in the "gathering" count while they go back to drop off resources.

P.S. Can anyone think of a better way to do this that wouldn't introduce a lot of complexity (like a static count would) and keep computational overhead low (the execution time ranges from 0-2 ms in release mode for me)?

comment:16 by Deiz, 12 years ago

Perhaps it's possible to combine the best aspects of both methods. Namely, gatherers could be tracked statically, without needing to handle every case where they stop gathering.

My idea is to use four static arrays, one per resource. When actual gathering starts, you'd check the unit's lastGathered, and if it's undefined or doesn't match the resource type being gathered, then it's set properly and the unit is appended to the relevant gatherer array.

Incidentally, it seems like it'd be better to count gatherers as soon as the animation starts (inside the AI's gathering code), rather than once they've actually gathered 1 resource.

To avoid the complexity of tracking every possible failure case, you could run a pruning function periodically (presumably right before updating the GUI). It would iterate over each array and remove any unit that no longer exists or is no longer gathering its lastGathered resource.

To avoid potential duplicates I suppose you'd also need to run the pruning function whenever a unit was given a new gathering command, but that's quite infrequent.

comment:17 by historic_bruno, 12 years ago

What about using something like StatisticsTracker as a counter for these things? I agree the last patch doesn't seem the best solution, I would also prefer UnitAI directly updating the counter (we could use the Player component but a separate statistics component seems cleaner). The problem with the current solution is it's not very extensible to other order-based counters we might want, e.g. current attackers/repairers/healers and it adds clutter to Commands.js and GUIInterface. Of course we'd have to be careful to handle ownership changes properly. I agree with quantumstate that messages are a good idea for improvement, too.

comment:18 by Matt Doerksen, 12 years ago

Yes, I think it could work to add in 4 arrays (or more later if we want to include things like attackers/healers/etc.) to StatisticsTracker. I should be able to make it work without too much trouble. But, just as a sidetrack, dealing with static arrays and messages will be a headache when finding bugs and all potential scenarios. As an idea (thinking out loud here), what if we were to keep the same code I have but run it every 5 seconds or something which would ensure the count is relatively accurate at all times, particularly when lots of action is going on (e.g. enemy attacking your base).

comment:19 by Deiz, 12 years ago

Here's my implementation of what I described in my earlier comment, based on the original patch. Information is stored in one object in the player state, with four arrays inside it.

Gatherer entity IDs are put into the relevant array once they pass several sanity checks. This is done inside the AI, so it's instantaneously reflected in the GUI (well, the next time the simulation state changes, which is ~200ms max for me) and the culling function is run every second.

I stuck it into the player state rather than statistics because the gatherer count is a transient thing, much like the current resource counts (which are also in the player state).

I also made a few minor GUI tweaks, to prevent "1000 (10)" (any combination of six digits) from wrapping in an ugly manner. Seven digits will still wrap, but that likely involves >= 10000 resources, which is an edge-case.

by Deiz, 12 years ago

Attachment: gatherer-count.patch added

comment:20 by Deiz, 12 years ago

My earlier patch is somewhat flawed, in that it tracks gatherers for all players but only removes non-gatherers for the local player.

Philip informed me that OnOwnershipChanged triggers for all of the edge-cases we might run into. As such, I've revised my patch and done away with polling.

The current implementation adds units when they start gathering a new resource, and removes them if they leave the gathering task without another unforced (UnitAI-created) gathering order queued up for the same resource, or if their ownership changes.

The relevant objects in the player state are also defined if necessary, for compatibility with older saves.

by Deiz, 12 years ago

Attachment: gatherer-count-2.patch added

comment:21 by Kieran P, 12 years ago

Milestone: Alpha 11Alpha 12

comment:22 by leper, 12 years ago

Keywords: review added

I haven't really looked at the patch, but you should remove the compatibility function as breaking saves between different alpha releases is to be expected (and I'm sure something else will break).

comment:23 by Deiz, 12 years ago

Owner: changed from Matt Doerksen to Deiz
Priority: If Time PermitsShould Have
Status: newassigned

Yeah, it's one of my earlier patches. Since then I've been responsible for breaking save compatibility a half dozen times or so.

The patch has some other flaws and misses two cases.

It only hooks into the gather order's "leave" and OnOwnershipChanged. The former only runs if it's the active order, but gatherers are counted as long as they're doing a gathering-related task (actual gathering, return to drop site, etc.) so I added an isGatherer boolean per historic_bruno's suggestion, and remove gatherers when replacing their orders, if necessary.

The second (minor) case is promotion. It's minor because it's really only relevant for hunters (who gain a small amount of experience from killing animals), and with no workaround the gatherer count would only be wrong until they actually started gathering meat. In any case, it was simple enough to handle nicely, so I did.

I'd quite like to see this in alpha 12 (at the latest) so I'm going to bump the priority, as I'm confident the patch works as it should.

by Deiz, 12 years ago

Attachment: gatherer-count-3.patch added

by historic_bruno, 11 years ago

comment:24 by historic_bruno, 11 years ago

Keywords: review removed

Seems to work, I made a few minor changes and fixed the conflicts with latest SVN. I changed the resource count spacing to relative instead of absolute, otherwise with 10+ gatherers or 10000+ resource counts it's very easy to overrun a single line.

I find the first missing case most problematic: when units search for a new gather target, they are temporarily removed from the gatherer count. As a result the count fluctuates quite a bit more than it should.

comment:25 by Kieran P, 11 years ago

Milestone: Alpha 12Alpha 13
Priority: Should HaveNice to Have

comment:26 by wraitii, 11 years ago

With the "Limit gatherer count per resource" patch now in, wouldn't it be easier to handle that in the same place in UnitAI.js?

comment:27 by Kieran P, 11 years ago

Milestone: Alpha 13Alpha 14

comment:28 by michael, 11 years ago

How is this going? Seems like a nice little enhancement to throw into Alpha 14.

comment:29 by Kieran P, 11 years ago

Milestone: Alpha 14Alpha 15

comment:30 by sanderd17, 11 years ago

Resolution: duplicate
Status: assignedclosed

Duplicate of #1871 (well, in fact #1871 is a duplicate of this, but the other fix has already been commited)

comment:31 by leper, 11 years ago

Milestone: Alpha 15Alpha 14

comment:32 by Kieran P, 11 years ago

Milestone: Alpha 14Alpha 15
Resolution: duplicate
Status: closedreopened

No, #1871 is different functionality. This is showing total workers in the top bar (either all the time or when hovering over a resource).

Version 0, edited 11 years ago by Kieran P (next)

comment:33 by historic_bruno, 11 years ago

Summary: [PATCH] Add current gatherers count next to resource count[PATCH] Add current gatherers count next to player's resource count

comment:34 by sanderd17, 10 years ago

There is still a problem with this patch, when you set the rallypoint of a building on a resource, and you train a batch of units, the batch is only counted as 1. I must say the code is rather forgiving to mistakes, as the moment the units go to the dropsite for the first time, and return, the number of workers is corrected.

Btw, I don't think it's a good idea to bind it with the limit on resource gatherers, as that limit fluctuates even more.

Last edited 10 years ago by sanderd17 (previous) (diff)

comment:35 by wraitii, 10 years ago

Relinking to this topic: http://www.wildfiregames.com/forum/index.php?showtopic=17461#entry271483 This is probably linked to formations, it'll have to be looked at.

comment:36 by leper, 10 years ago

Milestone: Alpha 15Backlog

comment:37 by wraitii, 10 years ago

In 14552:

Change the defense system used by Aegis to use more modular armies. This should be faster and easier to extend, though right now it might not be as efficient as before.
Fix a few bugs, including a few bad ones in the economy.
Change the way messages are handled, should be marginally faster in the later game.
Makes gatherers count limit be per-player (refs #1387 and #643).

comment:38 by historic_bruno, 10 years ago

Owner: Deiz removed
Status: reopenednew

comment:39 by Raymond, 9 years ago

Any progress? Would be very nice to have that feature.

comment:40 by scythetwirler, 8 years ago

This is pretty much already fixed, isn't it?

in reply to:  40 comment:41 by Lionkanzen, 8 years ago

Replying to scythetwirler:

This is pretty much already fixed, isn't it?

Indeed mostly is done... But I'm not sure if the full ticket is only show the gatherers in the tooltip (that is done).

comment:42 by Nescio, 6 years ago

Description: modified (diff)

It would be really nice if someone could update this and incorporate it in the game. Unfortunately I'm not skilled enough to understand the code.

comment:43 by Imarok, 5 years ago

Component: UI & SimulationIn-game UI

Move tickets to In-game UI as UI & Simulation got some sub components.

comment:44 by Marco Amadori, 3 years ago

Revamped the patch here: https://code.wildfiregames.com/D3155

comment:45 by Freagarach, 3 years ago

Milestone: BacklogWork In Progress
Patch: Phab:D3155

comment:46 by Freagarach, 3 years ago

Milestone: Work In ProgressAlpha 24

comment:47 by Freagarach, 3 years ago

Owner: set to Freagarach
Resolution: fixed
Status: newclosed

In 24357:

Show gatherers next to resource icons.

And total gatherers next to the population icon.

Closes #643
Patch by: @mammadori
Differential Revision: D3155
Earlier revisions reviewed by: @Angen, @wraitii
Comments by: @Nescio, @s0600204, @Stan

comment:48 by Freagarach, 3 years ago

In 25345:

Count resource gatherers also when returning their resources.

Followup on r24357 / rP24357.
This gives a better image of the amount of units which are gathering.
By decision, entities are _not_ accounted from when the player tasks them to return their resources.

Refs. #643
Differential revision: https://code.wildfiregames.com/D3226
Comments by: @wraitii

Note: See TracTickets for help on using tickets.