Treasure not being picked up

[elexis]

Sometimes unit's dont pickup treasure, which is annoying on survival of the fittest.

In the first replay attached, we see that Vercingetorix (player 4) isn't picking up the stone treasure (entity 1239), despite standing there for long enough at 8m 23s and shortly (turn 1038, 8m39s) resigning after that.

In the second replay, siole (player 4) isn't picking up the wood treasure (entity 2931) at 9:57.

[elexis]

vercingetorix replay

[elexis]

siole replay

[mimo]

I suppose that this is due to the MaxGatherer which is 1 for treasure, and is counted as soon as a unit is approaching and summed on all players. So if another player unit took the spot before, you won't be able to gather it even if the other unit is still far. May-be only counting the player's gatherers in case of treasure would work.

[Teiresias]

Demo for the "standing helpless before treasure" behavior with less overhead.

[Teiresias]

Created a demo for the "standing helpless before treasure" behavior (see attached .zip). On rev. 19000/FreeBSD build the problem is reproducible by unzipping the archive to binaries/data/mods and calling:

pyrogenesis -mod=bug4355 -mod=public -autostart=scenarios/Demo_4355 -autostart-ai=1:Bot4355 -autostart-ai=2:Bot4355

Player 1 has a long way to a treasure, player 2 immediately reaches it, then waits util player 1 has approached.

mimo's guess appears to be correct. Since the gatherer limit check is located in ResourceSupply.js (and common to all resources), a special test for treasures is due. A general player-wise gatherer limit test would actually allow up to n*MaxGatherers on a single resource entity.

Given that treasures are of instant-collect type, I would even go so far to not test their gatherer limit at all, or a single player could face the same problem again if he tries to collect a treasure with multiple units. Attached patch implements this approach and gives a reasonable behavior on the demo map attached at the ticket.

[mimo]

I think it would be better to check on the number of gatherers from the player instead of always returning true. And while at it, you could clean the previous code in that function

if (condition)
    return true;
return false;

could become

return condition;

[elexis]

Indeed mimo's assumption is right and players already adapted their clicking strategies.

allow up to n * MaxGatherers on a single resource entity.

It shouldn't allow 48 units of 2 players to gather a mine that is designed for 24 units.

I agree with mimo that checking this.gatherers[player] is preferable in the case of treasure over just returning true. This means that other units by the player that have finished picking up other treasure and are now looking to pick up more will skip this item if it's already in progress, making the units a bit smarter.

Code stlye:

• Usually we avoid hacks in the code as well as ticket references, proposed alternative implementations and explanations that the code will break when adding broken code (unless it's a real pitfall). Other places of the treasure code already state that treasure is about instant pickup.
  • HACK: The entire treasure implementation can be considered a hack. Not really useful to add the label.
• The left and right term of the condition in L133 should be swapped
• == is sufficient (=== only if we really need it)
• { to the next line
• While at cleaning, those unneeded parenthesis in GetKillBeforeGather are ugly

Adding a regression test wouldn't hurt (a new file test_ResourceSupply.js that for example verifies that GetNumGatherers is 2 after adding two gatherers and that IsAvailable works correctly for treasure) if you are interested.

[s0600204]

Related: (which may or may not be of interest)

• #1387 is the ticket where limiting the number of gatherers per resource was discussed and initially implemented
• r14552 changed the limits to be per-player
• r14848 changed the limits back to being global

​https://wildfiregames.com/forum/index.php?/topic/17461 was a discussion about gatherer-count limits and mentions several potential solutions to the problem of entities filling the gatherer-lists from a distance preventing closer units from gathering, namely:

• per-player gather limits
• removing entities from the list of gathering units while they return the resource (not really applicable in the case of this ticket, but meh)
• introducing the concept of "Enroute gatherers" which would be per-player whilst keeping the global "gatherer" list which would be changed so it only contains units that have actually arrived at the resource and successfully started resource gathering
• ratio approach so that groups of units tasked to gather will distribute themselves amongst similar-type resources near it, taking into account gatherers already present (again, not really applicable in the case of this ticket)

[Teiresias]

New attempt to solve the bug based on calling GetNumGatherersByPlayer()

[Teiresias]

Attached in the ResourceSupply.js.patch is a new attempt to solve the bug based on calling GetNumGatherersByPlayer() when testing treasures. A separate branch for handling these objects is required anyway. I tried to refactor IsAvailable() into the most legible(?) although not shortest form.

Regarding the formatting remarks of elexis:

• Swapped left and right term in the test condition (I'm an embedded guy - on the job I am supposed to write a condition in the "ugly way", so convention broke through)

• I moved the curly bracket into the next line, but according to "JavaScript: The good parts" that style may be dangerous as JS has a nasty capability of auto-inserting semicolons which can skrew up otherwise correct code

• I didn't remove the extra parenthesis in GetKillBeforeGather() because it a) seems to drift off-ticket-scope and b) the version without parenthesis seems less clean to me.

Additionally, I also added Jsdoc comments to the new GetNumGatherersByPlayer() and existing IsAvailable() functions in preparation of ticket #4057. While the description texts itself do not add too much value admittently, the type annotations (@param playerId {Number} ...) can be benefical in the future.

Also provided a minimalist test suite for the ResourceSupply component.

[mimo]

Usually, we omit braces when only one line block.

And why have this IsAvailable function changed so much (without being more readable imo). I would have done something like

let numGatherers = this.cachedType.generic == "treasure" ?
       this.GetNumGatherersByPlayer(player) : this.GetNumGatherers();
return numGatherers < this.GetMaxGatherers() ||
       this.gatherers[player].indexOf(gathererID) !== -1;

that is with minimal changes compared to previous code. I suppose that order of conditions (first the max number, then the id of the gatherer) was such because indexOf can be slow when looping on all gatherers

[Teiresias]

Ok, will remove the additional braces for the one-line blocks.

Regarding the large change of the IsAvailable function, my thoughts were as follows: There are in fact two maior use cases of IsAvailable:

1. The gatherer is 'new' to the resource supply, so the gatherer counting has to be done the usual way
2. The gatherer is already attached to the resource supply but for some reason re-checking availability: In such case the gatherer counting had to omit the gatherer in question when doing the check from 1), or miscounting will occur.

Use case 1 is further sub-divided into "treasure or normal resource" cases. My patch proposal is aimed to reflect this structure. Ordering of the if-structure is by rule "most special/interesting case(s) first".

I agree your snippet is more compact and less different from the previous version but what griefes me a bit is a) that "numGatherers" has a different semantics depending on the type of resource and b) the condition does multiple things at once. If we are to get rid of the ifs, i would prefer to 'elaborate' your version to

let numGatherersToBeLimited = this.cachedType.generic == "treasure" ?
       this.GetNumGatherersByPlayer(player) : this.GetNumGatherers();
let isTesteeAlreadyGathering = (this.gatherers[player].indexOf(gathererID) !== -1);

return numGatherersToBeLimited < this.GetMaxGatherers() || isTesteeAlreadyGathering;

Performance-wise I am not sure whether always calling indexOf() is worse as the other version usually calls GetNumGatherers() which calls reduce() which calls back the accumulator (a, b) => a + b.length.

While we are on it, another thing that still worries me is my ad hoc inline stub of Resources at the start of test_ResourceSupply.js. I would prefer a regular mock instead of the pseudo-definition at the file start but don't know a simple way to fix it.

[mimo]

I agree with you that (as most of our supplies have a small number of gatherers and as performance mainly matters when you have a lot of players) exchanging the order of the tests is a good move. But if you precompute both as in the example you gave, that's just a waste. And concerning your problem of different meanings depending on the resource type, i agree that a function should not have different meanings according to the context (as if you had changed the output of GetNumGatherers() according to the supply type instead of adding the new function GetNumGatherersByPlayer() which is fine). But i don't see why this is a problem with a temporary variable which is done for that purpose. But in fact, if we exchange the order of the tests, we should no more precompute the intermediate variable which will become useless most of the time and only have

return this.gatherers[player].indexOf(gathererID) !== -1 ||
	(this.cachedType.generic == "treasure" ? this.GetNumGatherersByPlayer(player) : this.GetNumGatherers()) < this.GetMaxGatherers();

or

if (this.gatherers[player].indexOf(gathererID) !== -1)
	return true;
let numGatherers = this.cachedType.generic == "treasure" ?
       this.GetNumGatherersByPlayer(player) : this.GetNumGatherers();
return numGatherers < this.GetMaxGatherers();

if it is considered more readable.

[Teiresias]

Adapted ResourceSupply.IsAvailable() as by mimos suggestions

[Teiresias]

Ok, I changed the structure of IsAvailable() according to your second draft, which I think is easier to comprehend for those non-familiar with the topic (attached as ResourceSupply.js.2.patch - I missed to tick the "replace existing file" checkbox, is there a way to repair this?)

As mentioned before, I renamed your temporary variable numGatherers to numGatherersToBeLimited to emphasize that it contains the fraction of gatherers the limit applies to (whichever that is).

[fatherbushido]

Ah I didn't notice there were tests for the ResourceSupply component here. So refs r20161

[Imarok]

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

[Freagarach]

In r24036:

Don't store the gatherers per player.

r14848 / ​Phab:rP14848 deprecated the use of storing the gatherers on a per player basis. This removes it entirely thus saving a few useless calls and operations.

Refs #1387. Differential Revision: D2755 Reviewed by: @bb.

Above mentioned commit should be reverted probably/perhaps for properly fixing the described issue.

[Freagarach]

Fixed in r24989 by splitting treasures from ResourceSupply and not implementing a limited amount of collectors for treasures.