[PATCH] Add ability to query percentage of map controlled

[s0600204]

The first of the two patches adds a function to the c++ component TerritoryManager that adds the ability for JS Component scripts to query what percentage of the game world is controlled by the player, as defined by territory boundaries.

This can be useful for several reasons, including:

• Permitting a possible new win condition - when a player controls a given quantity of the map (ie. 90%) then they win.
• Permitting the possibility for triggers to trip when a player exceeds a certain limit, or gets too small.
• Permitting the JS ResourceTrickle component to be amended to be conditional on territory control (simulating taxation, perhaps)

The second of the two patches, included with the above as an example of how this new function might be used, implements the last of these options, rewriting the ResourceTrickle component to add a new optional "Type" field with three choices:

• global - the default, and is what is currently implemented, with a constant rate as long as the entity exist
• garrison - the trickle rate is per garrisoned unit. Good for corral or a mountain/shaft mine
• territory - the trickle rate is limited by the percentage of the map the player controls as defined by territory boundaries

Please note that I'm very new to coding in c++, so... please go easy on me for any (newbie) mistakes...

[s0600204]

First patch (see ticket description)

[s0600204]

Second patch (see ticket description)

[leper]

Is the casting to float needed? Couldn't we just multiply with 100 (possibly cast to u16 to avoid issues) and then divide?

Why does Gaia own every tile? Shouldn't it be easier to just have ++m_TerritoryCounts[owner] and remove the if and the increment for Gaia?

Apart from that it seems like it would be better either calculating all of it only once (when queried and marking that function as expensive), or doing a proper init (all owned by Gaia) and then handle each ownership changed message (which we already do, well needn't be message but we do handle tile ownership changes in there).

[s0600204]

Is the casting to float needed? Couldn't we just multiply with 100 (possibly cast to u16 to avoid issues) and then divide?

Yeah, that would make more sense. Fixed.

Why does Gaia own every tile? Shouldn't it be easier to just have ++m_TerritoryCounts[owner] and remove the if and the increment for Gaia?

I can see why you might be led to that assumption, but index 0 of m_TerritoryCounts != gaia. Gaia cannot own territory, and so I was using index 0 to store a count of total passible cells.

Hopefully the updated patch is better.

[Niek]

"Permitting the JS ResourceTrickle component to be amended to be conditional on territory control (simulating taxation, perhaps)"

I've suggested a similar idea before in the forums but the reaction was generally negative. (should be somewhere in the Suggestions topi)

[leper]

You should do some init in Init(), and I'd prefer a normal array over std::array, though a vector could also be used. That would also be nicer than having that hardcoded 9 in there. (Using the player count (for a vector) or ARRAY_SIZE for an array. (Or .size() for std::array.)

GetTerritoryPercentage should do some range checks (ENSURE or returning 0, though I'd prefer ENSURE there).

CCing Sander since he worked on that code last and might have some other suggestions.

[Itms]

Hi, finally taking a look ;)

• I suggest you replace your array by a vector. The cmpPlayerManager will always be here (or it has to be fixed) on line 393. So you can lazily initialize the vector size at that point.
• Linked to the previous bullet point: don't store percentages, just store the number of cells (so your vector line 394 can be removed), and compute the actual percentage directly in GetTerritoryPercentage. That way you won't have initialization order problems.
• I would rather initially set m_TerritoryTotalPassableCellCount to 0 and ENSURE it is non-zero when you have to divide by it. Else errors won't be noticeable.
• Remove the new line 90 so it's clear that those variables work together.
• line 539, you probably want to exclude player 0 (gaia). You should return 0 instead of crashing in that kind of situations, because modders will most probably try to call the function for wrong players at some point.

After that, it would be nice to actually use that code somewhere. You could create a second patch adding that value to the statistics tracking (you can take a look at the percentage of explored map for inspiration). That would allow you to track down any issue before we commit that patch.

Thanks a lot for working on this! :)

[s0600204]

Updated patch with Itms' suggestions applied

[s0600204]

Use the new functionality in the Statistics component

[s0600204]

Hi Itms, thanks for the review. Hopefully the improved patch should satisfy your comments.

After that, it would be nice to actually use that code somewhere. [...] That would allow you to track down any issue before we commit that patch.

This is why the second patch above (2_ResourceTrickle.patch) exists. I was trying to avoid touching the statistics/summary code seeing as there is already two/three pending tickets with patches that make changes in that area... however, since you asked so nicely...

[Itms]

The patch looks way better this way IMHO :) I have only one suggestion, line 532 you could cast to size_t to compare with an object size, but that's more or less cosmetics here.

Sorry for missing your trickle patch, that's the problem of going through a number of reviewable patches :D It is a nice idea but subject to design discussions, so focusing on the statistics patch is a better choice I think. I don't have time to review it tonight but I think you will have to rebase it onto r16932, if you have time to do that before I review it ;)

Thanks for your work.

[s0600204]

Rebased on top of r16932

[Itms]

Committed the patch and the summary patch in r16933. I only changed the code in UpdateTerritories to avoid reallocating the vector each time.

Thanks for your work!