[PATCH] Percentage of mapcontrol not updated when phasing by cheat

[elexis]

Reproduce:

1. Start a game with cheats enabled on the default skirmish map
2. Type "back to the future" into the chat
3. Quit the game.

Notice in the summary screen it says "5%" territory control at peak but 9% territory control at finish.

Interestingly enough, if you build houses and then ordinarily click on aging without doing any further things (territory / grid changes), it will compute the correct number.

After cheat-phasing, StatisticsTracker.prototype.OnTerritoriesChanged will be called, but GetPercentMapControlled() returns the wrong value.

[elexis]

CalculateTerritories(); is called in CCmpTerritoryManager::ComputeBoundaries, CCmpTerritoryManager::GetOwner, CCmpTerritoryManager::GetNeighbours, CCmpTerritoryManager::IsConnected, CCmpTerritoryManager::SetTerritoryBlinking and GetTerritoryGrid. So calling it there too is consistent and also fixes the bug.

[elexis]

Also fixes the one issue found in #3399.

[s0600204]

As far as I can tell, the issue was caused by a race condition, with JS requesting the percentage before C++ getting round to recalculating it - from studying backtraces, CalculateTerritories was being called, but not always in time.

Anyway, the proposed patch fixes the problem, and IMHO is preferable to the one over in #3399.

[Josh]

Replying to s0600204:

As far as I can tell, the issue was caused by a race condition, with JS requesting the percentage before C++ getting round to recalculating it - from studying backtraces, CalculateTerritories was being called, but not always in time.

Anyway, the proposed patch fixes the problem, and IMHO is preferable to the one over in #3399.

I'm not sure that this is a better solution CalculateTerritories looks like a fairly expensive function (At least O(MapHeight*MapWidth*InfluenceEntities)) and it is called is several locations.

Is there any chance of just fixing the race condition and adapting #3399's fix?

[elexis]

(Patch reviewed by s0600204 in comment:3)

Replying to Josh: CalculateTerritories looks like a fairly expensive function

See IRC 2015-10-29:

18:41 < elexis> s0600204: can you estimate how much performance CalculateTerritories costs? 18:44 < sanderd17> elexis, IIRC, it should be pretty fast. It's possible to move buildings in Atlas, which will call that function every frame. While for the main game, it's only called on building completed, demolished and captured. 18:45 < sanderd17> If you want to test the speed, just run the profiler in Atlas while you move a builing around 18:51 < s0600204> elexis, I was worried about that too, but it should only inflict a performance hit if m_Territories needs recalulating which shouldn't happen too often

[s0600204]

Josh, you're correct - CalculateTerritories is a fairly expensive function (although not as expensive as it once was) - but the function usually returns early courtesy of the first couple of lines. It's only when MakeDirty has been run before it, clearing m_Territories, that the expensive stuff happens.

As to the race condition, when phasing normally, CCmpTerritoryManager's functions get called in this order:

GetOwner (x?)
 -> CalculateTerritories // early return
MakeDirty
Interpolate
 -> UpdateBoundaryLines
     -> CalculateBoundaries
         -> CalculateTerritories // expensive
             -> CalculateCostGrid
Interpolate (x10)
GetOwner (x4,685)
 -> CalculateTerritories // early return
GetPercentageMapControlled (x4)
GetOwner (x2)
 -> CalculateTerritories // early return
Interpolate (x9)
GetOwner (x?)
 -> CalculateTerritories // early return

And when using the cheat to phase:

GetOwner (x?)
 -> CalculateTerritories // early return
MakeDirty
GetPercentageMapControlled (x4)
GetOwner
 -> CalculateTerritories // expensive
     -> CalculateCostGrid
GetOwner
 -> CalculateTerritories // early return
Interpolate
 -> UpdateBoundaryLines
     -> CalculateBoundaries
         -> CalculateTerritories // early return
Interpolate (x5)
GetOwner (x?)
 -> CalculateTerritories // early return

(The (x4) indicates how many times that function (and everything it calls) was called.)

I ran it a few times to make sure. I'm using the latest git state at time of writing (815e672).

As you can see, CalculateTerritories gets called a lot already. (Usually by GetOwner which is itself called a lot). You can also see that it does get called after MakeDirty the second time, but not in time for GetPercentageMapControlled.

My current theory for the race condition is that when researching a technology normally, the tech is added to a research queue, and there is a point in the game simulation update sequence where the timers for all production/research queues are incremented on. If there are any techs who are due to complete in that frame, then they are processed then, before the simulation update moves on.

When using the cheat, the tech is processed in the next tick (or whatever), rather than at a time dictated by a production timer, and thus outside of the normal/expected simulation update/interpolation sequence.(Forgive me if I'm not using the correct terms, I hope my meaning is clear enough)

I may be wrong however, so feel free to correct me. (Nicely, please)

[Josh]

Really great research! Thank you for putting so much work into the problem.

I will commit your patch in the near future...

[JoshuaJB]

In 17171:

Fix #3525 and #3399 by recalculating territories on request to GetTerritoryPercentage. Patch by elexis

[Josh]

Thanks for the patch