Freeciv
Register
Advertisement
This is a historic page, please remove the {{historic}} tag if you plan to revive it.

The effects code got in 2.0.1 and 2.0.0 gives a lot of flexibility, yet it suffers from several weaknesses, like the poor Sources Cache. I have been thinking about some things Jason told me and how the present effects work and found several flaws in the present concept.

Say I want to add an effect. It is active for all cities with a Temple in them and an Oracle in their player owner. Where does this effect definition belong? In the Temple building definition? In the Oracle definition? Either way will cut it, but this will make it unreasonably hard to find an effect. This means the current way of defining effects is a poor model of our desired feature set.

If we want to run a search for effects the problem rears its ugly head again. The help system should say Oracle boosts the effect of Temples and that Temples are boosted by Oracle. The effect definition should reference all sources.

If we generalize effects further to Techs, Governments, etc this will only make the problem worse.


I think the best solution is to put all the effects in a special effects.ruleset file. A proposed EBNF syntax follows:

<effects_ruleset> ::= <effect>*
<effect> ::= <effect_type> <effect_value> <enabled_by> <disabled_by>
<enabled_by> ::= <requirement_sequence>
<disabled_by> ::= <requirement_sequence>
<requirement_sequence> ::= <requirement>*
<requirement> ::= <req_type> <req_id> <req_range> <req_survives>

Translated into a real life example in a proposed registry format:

[effect]
type = "Make_Content"
value = 1
enabled_by =
  { "type", "id", "range"
    "Building", "Temple", "City"
  }
[effect]
type = "Make_Content"
value = 1
enabled_by =
  { "type", "id", "range"
    "Building", "Temple", "City"
    "Tech", "Mysticism", "Player"
  }
[effect]
type = "Make_Content"
value = 1
enabled_by =
  { "type", "id", "range"
    "Building", "Temple", "City"
    "Building", "Oracle", "Player"
  }
[effect]
type = "Make_Content"
value = 1
enabled_by =
  { "type", "id", "range"
    "Building", "Temple", "City"
    "Building", "Oracle", "Player"
    "Tech", "Mysticism", "Player"
  }
[effect]
type = "Enable_Space"
value = 1
enabled_by =
  { "type", "id", "range", "survives"
    "Building", "Apollo Program", "World", "true"
  }

Or in a XML format:

<effect type="Make_Content" value=1>
  <enabled_by>
    <req type="Building" id="Temple" range="City" />
  </enabled_by>
</effect>
<effect type="Make_Content" value=1>
  <enabled_by>
    <req type="Building" id="Temple" range="City" />
    <req type="Tech" id="Mysticism" range="Player" />
  </enabled_by>
</effect>
<effect type="Make_Content" value=1>
  <enabled_by>
    <req type="Building" id="Temple" range="City" />
    <req type="Building" id="Oracle" range="Player" />
  </enabled_by>
</effect>
<effect type="Make_Content" value=1>
  <enabled_by>
    <req type="Building" id="Temple" range="City" />
    <req type="Building" id="Oracle" range="Player" />
    <req type="Tech" id="Mysticism" range="Player" />
  </enabled_by>
</effect>
<effect type="Enable_Space" value=1>
  <enabled_by>
    <req type="Building" id="Apollo Program" range="World" survives="true" />
  </enabled_by>
</effect>


NOTE: The enabled_by and disabled_by sequences are just like these C expressions:

enabled_by = req0.active() && req1.active() && ... && reqN.active()

disabled_by = !req0.active() && !req1.active() && ... && !reqN.active()

For an effect to be active:

active = enabled_by && disabled_by


Ideas for Optimization[]

I suspect this code will be nearly as fast as the current code. But it is possible to make it faster! Inside an effects query, it is possible to temporarily cache req.active() values, because the game state is frozen. This could be done via an hashtable keyed on a {req_type, req_id, req_range, req_survives} triple with a boolean data with the cached value.

Another Idea for Optimization[]

Make enabled_by an unordered set of requirements instead of a sequence. This allows some interesting optimizations to be made, courtesy of a static cache created at ruleset loading time:

struct requirement {
  type
  id
  range
  survives
};
#
struct {
  struct {
    struct requirement enabler
    effect_id
  } enabled_bys[]
  #
  struct {
    struct requirement disabler
    effect_id
  } disabled_bys[]
  #
  struct {
    effect_value
    #
    struct requirement enabled_by[]
    struct requirement disabled_by[]
  } effects[]
} cache[EFT_LAST];

This also saves recomputing req.active() values, at the expense of the mandatory requirement of calculating *all* unique requirement, unlike the previous method. A hybrid of both optimizations would not have that problem.

On Speed[]

This code will *not* be slower than the present code even without these optimizations, unlike what may be thought. Why? Because the current code and this code will produce the same basic execution traces!

This is what the current code does for a get_city_bonus:

int get_city_bonus(const struct city *pcity, enum effect_type effect_type)
{
  int bonus = 0;
  building_vector_iterate(get_buildings_with_effect(effect_type), pbldg) {
    effect_list_iterate(*get_building_effects(pbldg, effect_type), peffect) {
      if (is_effect_active(pbldg, peffect)) {
        bonus += peffect->value;
      }
    } effect_list_iterate_end;
  } building_vector_iterate_end;
  return bonus;
}

This is what the new code will do for a get_city_bonus:

int get_city_bonus(const struct city *pcity, enum effect_type effect_type)
{
  int bonus = 0;
  effect_list_iterate(*get_effects(effect_type), peffect) {
    if (is_effect_active(peffect)) {
      bonus += peffect->value;
    }
  } effect_list_iterate_end;
  return bonus;
}

In other words, it loops over *all* effects of a certain type (in a twisted way) and checks if those effects are active. It then sums up the values of the active effects. Which is exactly what the new effects code will do anyway! :-)

Advertisement