HomeWildfire Games

Add a system component to handle stat modifiers, make technologies and auras…
AuditedrP22767

Description

Add a system component to handle stat modifiers, make technologies and auras use this common interface.

The ModifiersManager system component provides an interface to add and remove modifiers, and get modified stats.
The goal is to merge all the different stat-modifying systems 0 A.D. has implemented over the years.
This commit makes technologies and auras use ModifiersManager. Some cheats and AI bonuses also have a similar stat-modifying effect that have not yet been updated.

Further, this system component makes it possible for e.g. triggers to easily add modifiers, enabling the writing of Castle Blood Automatic, RPG or Tower Defense maps without the need for mods or hacks.

The 'Modifier' name was preferred over 'Modification' as it is shorter and more readable, along with the logic that 'modifiers' store 'modifications' and this stores modifiers. Renaming of other functions and classes has been left for future work for now.

Internally, this uses a JS data structure. If performance issues arise with it in the future, this data structure or the whole component could be moved to C++.
The performance has been tested to be about as fast as the current implementations (and specifically much faster for global auras with no icons). Testing showed that sending value modification messages was by far the slowest part.

Comments by: leper, Stan, elexis

Differential Revision: https://code.wildfiregames.com/D274

Event Timeline

Nescio added a subscriber: Nescio.Aug 25 2019, 1:19 PM

Does this change the order in which they're applied?
Technologies are permanent, auras temporary; values used to be processed as follows:

  1. all xml template stats
  2. all technology multiplications
  3. all technology additions
  4. all aura multiplications
  5. all aura additions

( https://wildfiregames.com/forum/index.php?/topic/21083-how-to-modify-0-ad/page/2/&tab=comments#comment-336823 )

Does this change the order in which they're applied?
Technologies are permanent, auras temporary; values used to be processed as follows:

  1. all xml template stats
  2. all technology multiplications
  3. all technology additions
  4. all aura multiplications
  5. all aura additions

( https://wildfiregames.com/forum/index.php?/topic/21083-how-to-modify-0-ad/page/2/&tab=comments#comment-336823 )

Mh, actually it does because auras and technologies will now be processed as one. So if a tech was adding 5, multiplying by 2, and an aura was doing the same, it would before have done ((x * 2) + 5) * 2 + 5, whereas it now will do x * 4 + 10, which isn't quite the same.

Further techs will still be processed in semi-random order stopping at the first "replace", for example.

I've disliked that for quite some time and I wanted to fix this with "priority", which I had implemented early in the diff, but ended up scrapping as it generated much debate. I'll probably re-implement it at some point.

Mh, actually it does because auras and technologies will now be processed as one. So if a tech was adding 5, multiplying by 2, and an aura was doing the same, it would before have done ((x * 2) + 5) * 2 + 5, whereas it now will do x * 4 + 10, which isn't quite the same.

Further techs will still be processed in semi-random order stopping at the first "replace", for example.

I've disliked that for quite some time and I wanted to fix this with "priority", which I had implemented early in the diff, but ended up scrapping as it generated much debate. I'll probably re-implement it at some point.

So first all multiplications, then additions, sounds logical? Or can the techs/auras be still in random order (x * t1 + t2) * t3 + t4?

So first all multiplications, then additions, sounds logical? Or can the techs/auras be still in random order (x * t1 + t2) * t3 + t4?

Well "replace" is random, but so long as there is only one replacing tech, it should still work correctly.
For the rest it's all multiplications then all additions indeed.

So first all multiplications, then additions, sounds logical? Or can the techs/auras be still in random order (x * t1 + t2) * t3 + t4?

Well "replace" is random, but so long as there is only one replacing tech, it should still work correctly.
For the rest it's all multiplications then all additions indeed.

Sounds a lot easier to explain to me! (Though not entirely correct indeed.)

Well "replace" is random, but so long as there is only one replacing tech, it should still work correctly.

Shouldn't it be chronological? So if A and B both replace the same attribute, and you research B before A, then A replaces B's replacement.

Well "replace" is random, but so long as there is only one replacing tech, it should still work correctly.

Shouldn't it be chronological? So if A and B both replace the same attribute, and you research B before A, then A replaces B's replacement.

I think it's chronological right now but that's not really enforced in the code, nor has it been part of the specs anytime Afaik.
I actually think all modifiers should be chronological (that's the only thing that makes sense to me) but that has its own problems (such as researching techs in a particular order might give a better bonus).

but that has its own problems (such as researching techs in a particular order might give a better bonus).

Yeah, Cossacks had that; it wasn't great.

elexis added a subscriber: elexis.Aug 27 2019, 11:59 AM

There were some discussions about order in D274.
I didn't investigate, but perhaps deterministic order might also play a role for serialization / deserialization safety.

Freagarach added inline comments.Aug 28 2019, 5:46 PM
/ps/trunk/binaries/data/mods/public/simulation/components/ModifiersManager.js
166

I've done some digging, here the wonder aura stops. It seems to be correct before this. But I did not make this component ;)

wraitii added inline comments.Aug 28 2019, 6:24 PM
/ps/trunk/binaries/data/mods/public/simulation/components/ModifiersManager.js
166

What do you mean 'stops'?

Freagarach added inline comments.Aug 28 2019, 6:30 PM
/ps/trunk/binaries/data/mods/public/simulation/components/ModifiersManager.js
166

Yeah sorry I rushed this too much, I was already writing an elaboration ;)
I traced the wonder aura to here, but apparantly !cmpIdentity == true. What I meant with But I did not make this component was: I am not fully aware of what is going on here, but I figured the "trace" might already be helpful.

Added helper test: refs. D151.

elexis raised a concern with this commit.Aug 30 2019, 1:04 AM
This commit now has outstanding concerns.Aug 30 2019, 1:04 AM
elexis removed an auditor: elexis.Sep 1 2019, 11:15 AM

I suppose the audit on the impact of effect order might be continued.

This commit no longer requires audit.Sep 1 2019, 11:15 AM
Silier added a subscriber: Silier.Sep 13 2019, 5:55 PM
Silier added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
211

removing this introduced issue #5588 fixed by D2286

Stan added a subscriber: Stan.Sep 19 2019, 11:50 AM
Stan added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/components/Auras.js
389

This is useless. Reported by SonarQube. (The variable is not being used)

469

Same here.

wraitii marked 3 inline comments as done.Sep 19 2019, 1:30 PM
elexis raised a concern with this commit.Sep 25 2019, 1:42 PM

Prior to this commit, one would receive chat notifications when phasing up with cheats.

This commit now has outstanding concerns.Sep 25 2019, 1:42 PM
Stan added inline comments.Sep 25 2019, 2:38 PM
/ps/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
333

I guess we needed this.

Prior to this commit, one would receive chat notifications when phasing up with cheats.

Not only with cheats ;) (Also missing when researching.)

wraitii added inline comments.Sep 25 2019, 4:44 PM
/ps/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
333

Er, indeed, that seems like something that shouldn't have been deleted. Feel free to make a diff.

elexis added inline comments.Sep 25 2019, 5:11 PM
/ps/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
333

Why let others write the diff?

wraitii added inline comments.Sep 25 2019, 5:13 PM
/ps/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
333

I'll do it when I get the chance, but it Stan for example wants to do it first, I don't think that's an issue at all.

Nescio removed a subscriber: Nescio.Sep 26 2019, 10:20 AM
elexis removed an auditor: elexis.Sep 29 2019, 11:59 AM
This commit no longer requires audit.Sep 29 2019, 11:59 AM
Freagarach raised a concern with this commit.Apr 6 2020, 8:10 PM
Freagarach added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
211
This commit now has outstanding concerns.Apr 6 2020, 8:10 PM
Freagarach accepted this commit.May 11 2020, 7:10 PM

Capture points initialisation fixed in rP23633.

All concerns with this commit have now been addressed.May 11 2020, 7:10 PM