Page MenuHomeWildfire Games

Reduce aura defeat lag by avoiding useless recomputation of all auras globally
ClosedPublic

Authored by elexis on Mar 31 2018, 11:05 PM.

Details

Summary

In the replay profiled in #5099, we observed up to 20 seconds of freeze when someone was defeated.
fatherbushido pointed out that auras of units and structures are recomputed in both OnOwnershipChanged and OnGlobalPlayerDefeated,
whereas the latter is only relevant for playerauras, refs rP19093.

This patch adds the missing check.

Secondly this patch fixes a missing check in CalculateAffectedPlayers for defeated players (forgotton in rP19093).

The third change in this patch is that the PlayerDefeated and PlayerWon messages aren't broadcasted anymore, but only sent to the affected playerentities and entities that subscribe globally.

Test Plan

If you have a team of two ptolemians, both players may not receive food trickle after the defeat of one player.
Add a warn(this.entity) to see which entities are updated on defeat.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5724
Build 9614: Vulcan Build
Build 9613: arc lint + arc unit

Event Timeline

elexis created this revision.Mar 31 2018, 11:05 PM
temple added a comment.Apr 1 2018, 2:50 AM

Fix test.
The Iberian cheap skirm bonus isn't removed, maybe others too, I didn't go through them all. That's a problem with rP19093.

temple added a comment.Apr 1 2018, 4:03 AM

I played around a little more, and apparently with two on the team the bonus will be removed but with three it'll stay. There's lots of diplomacy changes at the start, maybe that could be somewhere to look, just a guess.

elexis added a comment.Apr 2 2018, 3:07 PM

Confirmed. CalculateAffectedPlayers` computes the correct players however and RemoveTemplateBonus seems to be called correctly too, but I didn't investigate further.
Also found #5109.

fatherbushido added a comment.EditedApr 2 2018, 5:19 PM

The Iberian cheap skirm bonus isn't removed,
That's a problem with rP19093.

Which is fun as the test plan was

Test Plan
You can check that your ally iberian team bonus is indeed removed when he's defeated.
Take care to test resign and defeat.

And that's exactly the cost one which is broken.
edit: (already in a22)
edit edit: the issue should be in rP19092 (not only for defeat but also for diplo changes)

temple added a comment.Apr 3 2018, 2:48 AM
In D1426#58338, @elexis wrote:

Also found #5109.

That was annoying me. Guess I was too focused on this other stuff to realize it was a bug. :/

temple added a comment.Apr 8 2018, 8:59 PM

We should decide something on gaia.
I think at the moment gaia isn't affected by technology? At least the advanced/elite bonuses.
Currently auras affect gaia but this patch would change it so that they wouldn't affect gaia.
However, gaia entities can still have auras that affect players.

mimo added a subscriber: mimo.Apr 8 2018, 10:30 PM
In D1426#58808, @temple wrote:

We should decide something on gaia.
I think at the moment gaia isn't affected by technology? At least the advanced/elite bonuses.
Currently auras affect gaia but this patch would change it so that they wouldn't affect gaia.
However, gaia entities can still have auras that affect players.

I'm fine with both features: auras do not affect gaia (does not have use on gameplay, so if it can save performance it's a good change) and gaia auras can affect players (i don't think we have currently such use case, but can be nice for scenarios).

In D1426#58809, @mimo wrote:

I'm fine with both features: auras do not affect gaia (does not have use on gameplay, so if it can save performance it's a good change)

For example that sele hero that affects enemy building health, with the patch it won't apply to gaia buildings.

and gaia auras can affect players (i don't think we have currently such use case, but can be nice for scenarios).

For example the kush relic which gives enemy women -15% gather rate.

So I'd probably lean the opposite way, player auras can affect gaia but gaia can't have its own auras.

mimo added a comment.Apr 8 2018, 11:13 PM
In D1426#58814, @temple wrote:
In D1426#58809, @mimo wrote:

I'm fine with both features: auras do not affect gaia (does not have use on gameplay, so if it can save performance it's a good change)

For example that sele hero that affects enemy building health, with the patch it won't apply to gaia buildings.

but gaia is supposed to be only a nuisance, not a real oponent, so it's not a problem.
For some maps (like jebel barkal) where the main interest is gaia, i think that should only be a temporary state with as a goal to move it as an extra player (would need changes in gamesetup, with possibly a dedicated ai).

and gaia auras can affect players (i don't think we have currently such use case, but can be nice for scenarios).

For example the kush relic which gives enemy women -15% gather rate.

That can be useful when designing some scenarios, don't you think so? But i agree not really useful in a standard game as long as it affect all players equally.

In D1426#58817, @mimo wrote:

but gaia is supposed to be only a nuisance, not a real oponent, so it's not a problem.

Suppose I have a choice between one hero that decreases enemy's attack by 20% and another that boosts my health by 25%. Normally those should have the same effect (1/.8 = 1.25), but if gaia's not affected by auras then the first one's useless.

For some maps (like jebel barkal) where the main interest is gaia, i think that should only be a temporary state with as a goal to move it as an extra player (would need changes in gamesetup, with possibly a dedicated ai).

Yes, it's better if the city was a player. Once we have that then gaia will basically be irrelevant, so okay for it not to be affected by techs or auras.

(Gaia also has the disadvantage that it doesn't have territory, so most buildings are set uncapturable currently. Having a fixed playerID means either max 7 players or finally increasing that number which needs some engine changes and scrolling for the gamesetup)

hero that decreases enemy's attack by 20%

I agree that it would be useful to the player, but the argument is only valid for gaia-maps (units of defeated players may be neglected) and it would also be useful for gaia maps if we could spawn Elite and Advanced units that actually have the according stats too.
The IMO most relevant part would be consistency.
But actually looking at the 3 changes in this patch, all of them are independent of the gaia question.
I'm ok with ignoring gaia and moving to a playerID > 0 eventually, but please in a separate patch to not add a 4th diffs to this diff. I can write a ticket.

binaries/data/mods/public/simulation/components/Auras.js
133

The only logic change in this hunk is to ignore defeated players, the rest shortens the code.

See also https://code.wildfiregames.com/rP19093#inline-2114

488

But everyone agrees that this early return is urgently necessary? I can commit it separately if so.

binaries/data/mods/public/simulation/components/Player.js
498

If people agree to this change, I will account for the new addition in Capturable.

fatherbushido added inline comments.Apr 11 2018, 6:38 PM
binaries/data/mods/public/simulation/components/Auras.js
125

It seems that the i range yields the questions.

elexis added inline comments.Apr 11 2018, 6:43 PM
binaries/data/mods/public/simulation/components/Auras.js
125

Indeed, I forgot about this change. I can use GetAllPlayers equally and postpone the gaia question. But we really need to reduce the number of open issues, which can be done by identifying the accepted subset of each patch.

temple accepted this revision.Apr 11 2018, 8:50 PM

Include gaia or not, either way is fine. Be sure to check tests.

This revision is now accepted and ready to land.Apr 11 2018, 8:50 PM
elexis closed this revision.Apr 13 2018, 6:17 PM

I will leave gaia unchanged for now.
Thanks for the reviews and feedback temple, fatherbushido and mimo.