Yield groups

[ShadesOT]

What?

I am suggesting to introduce groups of yield types or yield groups to handle problems with iterating the yields and to make the mod more generic and moddable.

Motivation

While refactoring, I thought a lot about how iterating the yield types can be made more generic. The challenge is, that in various places in the code, only parts of the yield types are iterated, depending on what is needed in the particular code section. This either requires the coder to know what are the iteration boundaries and to trust that the yield types of interest are adjacent within those boundaries without unwanted ones inbetween or to iterate all yield types and conditionally exclude the unwanted ones. A look at the code tells, that the later is the predominant case. Like this classic example:

for(int iYield = 0; iYield < NUM_YIELD_TYPES; iYield++)
{ 
    if (
    iYield != YIELD_FOOD 
    && iYield != YIELD_LUMBER 
    && iYield != YIELD_STONE 
    && eYield.isCargo()
    )
    {...}
}

Redundant checks are used instead of iteration boundaries. (This observation led to #57.) Naturally, I would say, because the idea for the yield types is, that they are moddable and new ones can be added and existing ones can be removed via modifying the xml. The coder on the other hand can not ensure, that the yield types are added at the correct place within the enumeration and thus, that they are within the iteration boundaries, one could have choosen. I believe, that is also the reason, why the yield types and their sequence are hard coded currently. This way everyone can see which are there, in what sequence they are listed and that this stays the same.

Hence, the mod code is in a quirks state of two extremes (everything moddable vs everything hard coded), inheriting the downsides of each extreme. The coders can not trust that the list of yield types stays the same and the modders can not add/remove yield types without changing hard code. Issue #58 suggest to better the situation by generating the enumeration of yield types at compile time and setting iteration boundries, while doing that.

What none of the above suggestions can handle is a generic number of groups of yield types. A hard coded enumeration, whether created at compile time or sooner, would require the yield types within the groups to be adjacent in the enumeration, so they can be grouped by a lower and a higher index number. That can not be made true for n groups or even like 10 groups, which we would need for the current game mechanics.

Thus I am suggesting groups of yield types or yield groups. They would solve all the problems, mentioned above and would make the code more generic, consistent, less bug prone and the mod easier moddable.

Yield groups

The characteristics of a yield group are

1. The relationship between a group and a yield is n:m, meaning a yield can be part of n groups and a group can have m yields.
2. Yields within a group are treated equally, meaning 2.a. the code for all of them is the same - no exceptions - and 2.b. the sequence in which they are processed is not important.
3. Yield groups are hard coded. It is known and documented what they do and when and where in the code it is done (which is important for modders and coders alike).
4. There are two types of yield groups: Essential and non-essential ones. The essential ones are attached to a game mechanic, the game can not be played without, and thus can not be empty (eg the food group, because the food and growth mechanics need at least one yield to work with). The domestic market goods group on the other hand is non-essential, the game can be played without it and thus can be empty.

How does a yield group look in the code?

XML

• A YieldGroup contains n references to yield types.
• For each yield group there MUST be a xml tag.

  <YieldGroup ID="food">
  <Yield ID="YIELD_WHEAT" />
  <Yield ID="YIELD_FISH" />
  </YieldGroup>

  C++

• If the tag for one of the groups is missing or an essential group is empty, the code must throw a stopping runtime error or there must be an ingame message that the game, modded this way, can not be played.
• There is a class YieldGroup.
• That class provides some sort of iteration mechanism.
• Each group gets its own instance of YieldGroup.
• It does reveal how many yields are in that object.
• It does not reveal which yields are in it. References to specific yields are forbidden. The only thing a coder knows, is that all yields in that group have to be handled in a certain way.

Thoughts, ideas, examples ...

Examples of possible yield groups for the current game mechanics

• Food; essential
• Cargo; ?; yields in this group can be transported around and fill warehouses
• Strategic yields; ?; yields like lumber and stone
• AI distribution cheat yields; non-essential; yields in this group get redistributed via cheat code, like lumber and stone now
• Domestic market yields; non-essential
• Production group; essential; hammers, stone, tools, rope, sail, silver, cannons, blades; maybe the contents of this group can be derived from the things which can be produced automatically? - just a thought
• bells; essential
• education; essential;
• health; essential;
• religion; essential; crosses
• culture; essential; and so on ...

  A distinct group is required for each distinct game mechanic

  The above examples show, that each game mechanic needs its own yield group. The rule of thumb is, as soon as there is distinct code to handle one or a group of yields, it needs a distinct yield group.

  Code to handle a yield group is unaware of the number and type of yields in it

  All code written to work with a specific group, must be able to handle the yield group and the yields in it, without knowing what the yield types are and how many there are. The number of yields is just there to be able initialize arrays and for stuff like the city screen and the display of goods in the warehouse.

  No more reliance on the sequence of yields in a group

  Currently the mod engine, heavily relies on the sequence in which the yields are listed in the enumeration. The mod works correctly as long as colord cloth is listed after cloth and indigo, and cloth is listed after cotton in the enumeration. If it was the other way around it would not work anymore. The calculation of production of colored cloth takes place after the calculation of cloth and indigo, when the production rate of those yields together with the warehouse stock and thus the available amounts of the prequisites are already known. Code for yield groups can not rely on a fixed sequence anymore. A dependency tree would have to be generated at xml read time.

  Yield groups can be used to store game mechanic specific values

  Imagine this example: The food group comprises two distinct yields, wheat and fish. And we implement a new game mechnic "food decay rate". Each yield needs a decay modifier (say wheat 10% per turn and fish 100%) to calculate the decay per turn if not used up. The code for this game mechanic would be spcific to this yield group. And the values of the decay modifier as well. It would not be used anywhere else in the code. So we could do this ...

  <YieldGroup ID="food">
  <Yield ID="YIELD_WHEAT">
      <DecayModifier>10</DecayModifier>
  </Yield>
  <Yield ID="YIELD_FISH">
      <DecayModifier>100</DecayModifier>
  </Yield>
  </YieldGroup>

  ... and by doing this, we could keep game mechanic specific values together, instead of spraying them all over the yields xml definitions. If for whatever reason a game mechanic will be sorted out, also all the values associated with it, are gone with two keystrokes.

  Yield groups can be used to keep game mechanic specific code together

  What is true for game mechanic specific values, is true for the code specific to that game mechanic as well. We actually could write a c++ class for each yield group, which derives from some base class (that handles the reading, containers, iterators, ... ) and put all code, handling the yields of it, into it. Bam - suddenly all code, specific to a game mechanic associated with a yield, is in the same place. And it would not be needed anywhere else anyways, not in CvCity, not in CvPlayer, ..Plot, ..Unit. If we want to remove that game mechanic - just delete that class and the function calls to the instance of it.

  Yield groups hamper fast implementations

  That is both an up- and a downside. Something like

  for(int iYield = 0; iYield < this->mNumYields; iYield++)
  { 
  if (iYield != YIELD_CANNONS)
  {...}
  }

  would be forbidden by the coding rules then.

If the need for code for a new subgroup of the yields arises, the coder would have to create a new yield group (first getting a clear idea about why it is necessary and what its characteristics are), create the xml for it (copying all wanted yields into it), document what it stands for, so the modders and other coders know what it is and does, create a new class deriving from the base class, register an instance of it with the mod engine. And after all that the coder could write the oneliner one had in mind.

Sounds really horrible. To me too. No fast implementations anymore regarding yields. It enforces code consistency though and prevents spaghetti code and bug prone code additions to functions.

Yield groups do not have to be used everywhere at once

The good thing regarding the above point is that yield groups and the current enumeration of yields can be used alongside. The current code does not have to be rewritten regarding yield iterations in full, if we decide to use yield groups and fast implementations are still possible in the future, as long as we keep the enumeration.

[ShadesOT]

I finished writing this issue ;-)

[Nightinggale]

A quick note to add here is the CivEffect branch adds EnumTypeCacheArray and InfoCacheArray.

The idea is that you can make a BoolArray, set the contents and then init a cache array with the BoolArray as argument. What it does is it generates an array of the true values while ignoring the false values.

It's used for domestic market yields. First it loops all buildings and units, sets the bool to true for each consumed yield and then it generates an array, which is stored in CvGlobals. During doYields, it works like this:

const YieldTypeArray& kYieldArray = GC.getUnitYieldDemandTypes();
for (int i = 0;; ++i)
{
        YieldTypes eYield = kYieldArray.get(i);

The code then moves on to break if eYield is NO_YIELD. This will make one iteration for each yield, which was true in the BoolArray while it will completely skip the yields, which were false. This means if 8 yields are true based on xml values, the loop will iterate 8 times (+ break iteration) regardless of the total amount of yields being 8 or 80.

The reason why there are two classes for this is one returns enum indexes while the other returns info class pointers. Which one you prefer depends on what you need while looping. Stop condition is returning NO_TYPE (-1) or NULL pointer. You can also use getLength() if you prefer.

The CivEffect branch introduces a function, which is called as soon as the xml files are done loading and it's intended to set up arrays and other cache, which is based on xml data.

I think this is what you are aiming at with this ticket. I think it would be best if we can use the same classes for this all over, particularly because I plan on extending this to exposing the classes to python, allowing the python GUI code to have read only access to whatever the arrays contain. Also I wrote the classes with const correctness in mind, meaning it's not by chance that CvGlobals returns a const reference. It's because only CvGlobals is allowed to change the contents while CvCity can read the contents. Since CvCity (or any other non CvGlobals classes) can't alter the contents, bugs are less likely to occur and easier to find if they do happen.

[LibSpit]

On the note of the 'longer implementation' issue, could something like a code template be made that has X% of the work done and you just replace specific elements (like the group name and yield types) so you copy the whole template, fill in all the specifics, and then be back to the 'short version' starting point to do whatever specific thing that is planned from that point forward?

[Nightinggale]

I believe, that is also the reason, why the yield types and their sequence are hard coded currently. No, the reason is that enums are known at compile time while xml values aren't. Switch case requires the cases to be known at compile time, meaning the yield enum is present to allow switch case code for yields. Remove the switch cases from the code and we can get rid of the yield enum, but we can't get rid of the enum without getting rid of the switch cases.

Thus I am suggesting groups of yield types or yield groups. They would solve all the problems, mentioned above and would make the code more generic, consistent, less bug prone and the mod easier moddable. I added something to M:C, which I think I called yield categories, though groups is not a bad name either.

What it does is it adds an enum value to YieldInfo, telling which category the yield is and then CvYieldInfo has getCategory(). The design goal was to make switch case statements rely on the yield category rather than the yield. This way the yield enum could be removed and the DLL would no longer require yield hardcoding. In other words the goal was to aid xml modders. The implementation actually works in the M:C development branch.

Another trick used in M:C to gain this goal is global YieldTypes variables. If YIELD_HAMMERS is removed from the enum, make it a global variable and when xml is loaded, assign a value to it. This way the existing code will not need to be updated, yet it will use the value from xml. The global variable(s) will be disabled by a compile flag if the enum hardcoding is enabled. This way we can maintain the performance boost from hardcoding in release builds while assert builds (the one xml modders should use) will listen for xml changes.

C++
- If the tag for one of the groups is missing or an essential group is empty, the code must throw a stopping runtime error or there must be an ingame message that the game, modded this way, can not be played.
- There is a class YieldGroup.
- That class provides some sort of iteration mechanism.
- Each group gets its own instance of YieldGroup.
- It does reveal how many yields are in that object.
- It does not reveal which yields are in it. References to specific yields are forbidden. The only thing a coder knows, is that all yields in that group have to be handled in a certain way.

My previous post more or less covers everything here. When the xml files are loaded, make some BoolArrays, loop all yields, set the BoolArrays according to the yield category. When finished, init cache arrays in CvGlobals. This will live up to everything you wrote here and it will not require DLL hardcoding.

Domestic market yields; non-essential Already implemented in the CivEffect branch (as proof of concept for the class). It's autogenerated based on unit and building requirements. There is no need to include it here.

YIELD_HAMMER and all yields after it seems essential and each will have a category for itself (most likely).

Bools in yields

We can also consider adding a bunch of bools to Yield xml. This wil allow us to add a 32 bit unsigned int and use each bit for a bool, which is fast and memory efficient. This can be used to add differences in yields despite being in the same category, which could be useful for the AI.