Opened 3 years ago

Last modified 3 months ago

#4355 new defect

[PATCH] Treasure not being picked up

Reported by: elexis Owned by:
Priority: Must Have Milestone: Work In Progress
Component: Simulation Keywords: rfc patch
Cc: Patch:

Description (last modified by 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.

Attachments (5)

commands.txt (582.8 KB) - added by elexis 3 years ago.
vercingetorix replay
commands.2.txt (1.3 MB) - added by elexis 3 years ago.
siole replay (3.5 KB) - added by Teiresias 3 years ago.
Demo for the "standing helpless before treasure" behavior with less overhead.
ResourceSupply.js.patch (6.5 KB) - added by Teiresias 2 years ago.
New attempt to solve the bug based on calling GetNumGatherersByPlayer?()
ResourceSupply.js.2.patch (6.5 KB) - added by Teiresias 2 years ago.
Adapted ResourceSupply?.IsAvailable?() as by mimos suggestions

Download all attachments as: .zip

Change History (21)

Changed 3 years ago by elexis

Attachment: commands.txt added

vercingetorix replay

Changed 3 years ago by elexis

Attachment: commands.2.txt added

siole replay

comment:1 Changed 3 years ago by 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.

comment:2 Changed 3 years ago by elexis

Priority: Should HaveMust Have

Changed 3 years ago by Teiresias

Attachment: added

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

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by stanislas69

Keywords: rfc patch added
Milestone: BacklogWork In Progress
Summary: Treasure not being picked up[PATCH] Treasure not being picked up

comment:5 Changed 3 years ago by 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;

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by 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 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)
Last edited 3 years ago by elexis (previous) (diff)

Changed 2 years ago by Teiresias

Attachment: ResourceSupply.js.patch added

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

comment:8 Changed 2 years ago by 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.

comment:9 Changed 2 years ago by 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

comment:10 Changed 2 years ago by 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.

comment:11 Changed 2 years ago by 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();


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.

Changed 2 years ago by Teiresias

Attachment: ResourceSupply.js.2.patch added

Adapted ResourceSupply?.IsAvailable?() as by mimos suggestions

comment:12 Changed 2 years ago by 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).

comment:13 Changed 2 years ago by elexis

Description: modified (diff)
Milestone: Work In ProgressAlpha 22

comment:14 Changed 2 years ago by elexis

Milestone: Alpha 22Work In Progress

comment:15 Changed 21 months ago by fatherbushido

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

comment:16 Changed 3 months ago by Imarok

Component: UI & SimulationSimulation

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

Note: See TracTickets for help on using tickets.