Add a tile owner check in territory decay
ClosedPublic
Actions

Authored by temple on Jan 15 2018, 9:50 PM.

Details

Reviewers

mimo

Commits

rP20949: Add a tile owner check in territory decay

Trac Tickets

#4787
#4681
#4749

Summary

If we own a building in a territory that isn't ours and isn't rooted, then we should decay but we shouldn't have an effect on whether the territory blinks or not (since it's not our territory). See #4787.

(Full disclosure: I think we should have a different system, as described in D840.)

Test Plan

(Currently we must be adjacent to an ally's rooted territory to not decay, so it's consistent to decay if our bulding's in an ally's rootless territory, even if that territory isn't blinking.)

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

temple created this revision.Jan 15 2018, 9:50 PM

temple mentioned this in D1226: Decay to gaia or the territory owner, rather than its neighbors.

elexis edited the summary of this revision. (Show Details)Jan 15 2018, 10:13 PM

@mimo I saw you have done some commits on this file, I read it for the first time now, can you have a look at this diff and possibly D1226?

Accepted as the problem quoted in the ticket is fixed.
I nonetheless noted something i was not aware of before, that is that we don't have any recursive check: if A(ally 1, non connected) is connected to B(ally 2 , non connected) which is itself connected to C(ally 1 or another ally, connected), A will decay. Can be seen as a feature? or we can try to add at least one level of recursivity? i'm not sure if needed, any feeling about it?
[if we add it, the line 48 of the fix would become "return this.connectedNeighbours.some(i => cmpPlayer.IsMutualAlly(i))", but the fix is more complex when the building has territory].

This revision is now accepted and ready to land.Jan 16 2018, 8:10 PM

In D1225#49834, @mimo wrote:

I nonetheless noted something i was not aware of before, that is that we don't have any recursive check: if A(ally 1, non connected) is connected to B(ally 2 , non connected) which is itself connected to C(ally 1 or another ally, connected), A will decay. Can be seen as a feature? or we can try to add at least one level of recursivity? i'm not sure if needed, any feeling about it?
[if we add it, the line 48 of the fix would become "return this.connectedNeighbours.some(i => cmpPlayer.IsMutualAlly(i))", but the fix is more complex when the building has territory].

This is (2) in #4749. Usually buildings have territory, so we'd need to figure out the complex fix, and I'm not sure it's worth the effort. (Again, I have my own ideas about territory, where this wouldn't be an issue.)

The fill function doesn't extend the length of an array, so this was a bug. When later e.g. we set this.connectedNeighbours[2] = 1, then it would make the array [,,1] rather than [0,0,1,0] or something (depending on the number of players). Using this array for the neighbors in Capturable should fail, but apparently the function's (always?) called twice, and on the second pass it first fills [,,1] with zeroes to get [0,0,0], and then the neighbor is set to get [0,0,1]. Again, this should really be [0,0,1,0], but it works in Capturable.

Anyway, that's fixed now. Also I set the neighbor to be gaia in the case of a building in another player's unrooted territory. Capturable defaults to gaia, but I think it's better the specify it explicitly.

mimo added inline comments.Jan 17 2018, 8:55 PM

binaries/data/mods/public/simulation/components/TerritoryDecay.js
20 ↗	(On Diff #5361)	setting the size to numPlayers could better be done in init

temple updated this revision to Diff 5363.Jan 17 2018, 9:35 PM

temple added inline comments.

binaries/data/mods/public/simulation/components/TerritoryDecay.js
20 ↗	(On Diff #5361)	Right, I was thinking you're not suppose to access other components during init, but this is a system entity so it should be fine.