Imperformat Coding: Many Trait Modifiers bypass "processTrait" [WORK IN PROGRESS]

[raystuttgart]

Hi guys,

I have no idea why or how this really happened but a huge amount of Trait Modifiers is "bypassing" this central methods:
(I was most likely also contributing to this mess over the years.)

void CvPlayer:processTrait(TraitTypes eTrait, int iChange)

To explain, the method centrally collects the single modifiers from the Traits and stores an according attribute in the Player object.
Thus later the "functional code" does not need to loop the Traits every time it needs a modifier but can instead directly read from the attribute in the Player object.

Using "processTrait" would have several benefits:

• The code using those modifiers get cleaner by reading it from a central space (instead of looping)
• The code using those modifiers gets a lot more performant by reading a single value (instead of looping)

Summary:
A code refactoring might be a good idea and would probably also result in a performance improvement.

@Nightinggale
What is your opinion on this? Personally I know it will be tedious effort, but I think it might be worth it.
Should we clean it up and have all Trait modifiers handled and stored by "process Trait"?

 

[Nightinggale]

This is part of of the concept for CivEffects. Have a cache in the player to tell if a certain effect is active. For code reusability it would be best to move the code in question into CivEffects and then let the trait provide a CivEffect providing the effect in question.

The way it is supposed to work is this: Let's say we add a feature called Bingo (whatever that is).

• CivEffect xml has AllowBingo (int, usually given as 1, 0 or -1, default being 0)
  
• CvPlayerCivEffect (in CvPlayerCivEffect.h) has an int storing the combined value of all owned CivEffects
• CvPlayerCivEffect has a bool canDoBingo() function, which returns the bool iCache > 0
  
• CvPlayerCivEffect.cpp sets the cache to 0 by default and adds the values from all owned CivEffects

This way CvPlayer can call CivEffect()->canDoBingo() really fast. It's true if one or more traits/CivEffects enables it and the engine running this can also support -1 to disable it for a player if needed.

CivEffects are looped during init, but after that they are never looped. This is what you are asking for and what we all want from a performance point of view. Using CivEffects rather than CvTrait is however taking it a step further in regards of xml freedom.

Also while on the topic, I added AI() and CivEffect() to CvPlayer. It is returning the this pointer as those two classes. This makes accessing CivEffect specific functions easier while also keeping them in a dedicated class as that provides cleaner code.

 

[raystuttgart]

This is what you are asking for and what we all want from a performance point of view.

Exactly, avoid unnecessary looping through giant list of traits in functions called hundreds of times (e.g. Citzens, Cities, Plots, ...) if instead a Trait or CivEffect can be processed once and "modifers" stored at the Player object.

However your answer is again confusing me - sorry.
It not clearly tell me "Yes do it." or "No need to waste your time, because my solution is better.".

Should I refactor the functional code that does unnecessary looping of Traits or not?
Because I was planning to do so the upcoming weekend and really do not want to waste my time.

Basically I would channel almost all the traits through here first:

void CvPlayer:processTrait(TraitTypes eTrait, int iChange)

 

[raystuttgart]

Also bump ... so it is not forgotten.

 

[Nightinggale]

I already did something for this, which is moving learning by doing modifier from runtime looping to being part of the CivEffect cache. diff

Also I added an issue to keep track of this progress. issue 528