Move vetDamage total inside instigator check

[clyfordv]

Previously, all damage taken from any source (including an allied unit or self) would be included in the veterancy dispersal calculations. This moves the addition operation to VetDamageTaken (the denominator in veterancyDispersal) inside an "IsEnemy" check, which prevents (for example) the Paragon from stealing 10,000 damage of veterancy experience for itself as it dies.

Downside is a particular unit may get more credit than it is "rightfully" owed for a unit that was hit by friendly fire--not a big downside, in the scheme of things.

[clyfordv]

... also, with the pitiless attitude of working with code I didn't write, is there a reason not to condense VetDamage (<EntityId, Unit>) and VetInstigators (<EntityId, damage>) into <Unit, damage>, and eliminate a whole table declaration on every unit in the game? (see most recent commit, tested it with the Mavor + Paragon and appears to work correctly)

[Garanas]

... also, with the pitiless attitude of working with code I didn't write, is there a reason not to condense VetDamage (<EntityId, Unit>) and VetInstigators (<EntityId, damage>) into <Unit, damage>, and eliminate a whole table declaration on every unit in the game? (see most recent commit, tested it with the Mavor + Paragon and appears to work correctly)

There certainly is!

When you use a table as a key the memory reference is used as the actual 'key' value. This memory reference is not deterministic and is therefore almost guaranteed to be different among players. As a result iterations on this collection are in a different order for each player. And the moment that happens all bets are off and the game can randomly desync, especially when mods or future contributors may suspect it is safe to use for other purposes.

All in all; no - we can't eliminate the table 🤠

[Garanas]

We could maybe however use this:

https://github.com/FAForever/fa/blob/ceb09cea1360407050a8b19c2647dcbfce991efc/engine/Core.lua#L192-L195

I'm not sure how efficient that function is, we'd need to benchmark it. But that way we can entirely rely on the VetInstigators table and get rid of the VetDamage damage.

[lL1l1]

Would be nice to annotate what stats units usually have.

[Garanas]

Would be nice to annotate what stats units usually have.

How would we annotate that?

[Garanas]

The use of GetEntityById and GetUnitById appears to be fast - I can do 900 queries in 0.4ms. That's sufficient for this type of task!

[Garanas]

@clyfordv can you write a changelog snippet? Make sure to also describe the practical change for the players

[clyfordv]

Dropping the VetInstigators table introduces the issue where a new unit with a reused ID might get credit for kills by the previous (now dead) unit that had the same entity id. Thoughts?

[Garanas]

Dropping the VetInstigators table introduces the issue where a new unit with a reused ID might get credit for kills by the previous (now dead) unit that had the same entity id. Thoughts?

Is there a way to detect how often this occurs? If it barely, if ever happens then I don't see an issue.

[clyfordv]

Unfortunately, I think it might happen... all the time? Basically, any time:

1. Unit does damage to target
2. Unit dies
3. New unit is created with same entity Id
4. Original target dies

Which feels like it could be an extremely common (inevitable, really) occurrence.

[Garanas]

But is there a way to test and see how often it happens? What if we re-introduce the backup table and see how often they're not the same unit for an AI vs AI game?

[clyfordv]

With titans, ravagers as a backstop:

So 3-5% is probably a good estimate.

(apropos nothing the dynamics of the waves of bots sloshing back and forth is very interesting)

[clyfordv]

Relevant code here/"I know how to github":

[Garanas]

Okay; that is super interesting. Let's let this sink in and see if we can come up with a solution.

[Garanas]

I've given it some thought and to me the only correct solution was the old solution where we use a separate table. Did you manage to come up with something?

[clyfordv]

I did not, I think it's inescapable, will revert the changes.

[MrRowey]

@clyfordv can we get this finalised so we can have as a part of the game patch

[clyfordv]

It's done, just needed to update the changelog.