[Freecol-developers] Modifiers

On Sat, 28 Jan 2017 17:26:46 +0100

Post by Enrico Weigelt, metux IT consult
I've seen that modifiers are sometimes collected in lists, sometimes in
sets. Is there a special reason behind that ?

Historical accident/inconsistency between developers.

Post by Enrico Weigelt, metux IT consult
Are there cases where duplicate modifiers need to be filtered out,
or could we just use lists anywhere ?
(eg. Unit::getMissionaryTradeModifiers())

I doubt anyone knows.

Post by Enrico Weigelt, metux IT consult
Also, I'm wondering, why sometimes lists are sorted, while in other
cases they are not. Isn't the sorting always required for applying
(so additions and multiplications remain in the correct order)

Sorting is now only required at the point the modifier is applied or for
display purposes. Modifier application used to involve some very brittle
special case code until we made a sustained effort to define a sorting
order, allowing the current sort+apply-incrementally algorithm.
Flexibility is required as there are almost certainly more Col1
incompatibilities in combat.

Post by Enrico Weigelt, metux IT consult
I'm also thinking about compound modifiers - for example Building's
poduction modifiers are union of several lists (building, colony,
etc, etc). That could be an own modifer type that calculates from
the individual lists.

I doubt this is useful, and note that you have not provided evidence that
this solves a problem. You will need a mechanism to recalculate the
compound modifier when one of the underlying modifiers changes, which is
asking for bugs due to forgetting one or more special cases --- years
after it was introduced we are still uncertain all the analogous
cases with the production cache have been caught. Also the individual
modifiers will still be needed for display purposes so you will not be
saving memory.

Cheers,
Mike Pope

Enrico Weigelt, metux IT consult

2017-01-30 16:49:40 UTC

Post by Michael T. Pope
On Sat, 28 Jan 2017 17:26:46 +0100

Post by Enrico Weigelt, metux IT consult
I've seen that modifiers are sometimes collected in lists, sometimes in
sets. Is there a special reason behind that ?

Historical accident/inconsistency between developers.

Ok. Seems that Set is used more often (especially in FeatureContainer),
therefore duplicate modifiers (incl. all attributes eg. source) are
not allowed.

Digging a bit further, it really seems the whole duplicity problem
(perhaps even the need for the source attribute) comes from the fact
that at some places modifiers of sub-/related objects (eg. buildings
and building types within colonies) are aggregated here and removed
there. Just have a look at Building::setType().

Looks like a good place for subtle and hard to find bugs (which actually
hit me in a completely different area).

Post by Enrico Weigelt, metux IT consult
Are there cases where duplicate modifiers need to be filtered out,
or could we just use lists anywhere ?
(eg. Unit::getMissionaryTradeModifiers())

I doubt anyone knows.

Meanwhile I've found out that it's required for the modifier
inheritance caching. But this, in many places, doesn't seem to be
the proper way to go (eg. building type modifiers propagated up into
individual colonies), and it makes the whole thing very fragile.
(for things like derived spec objects, like building or unit types,
it might make sense, in contrast to usual game objects)

Post by Michael T. Pope
Sorting is now only required at the point the modifier is applied or for
display purposes. Modifier application used to involve some very brittle
special case code until we made a sustained effort to define a sorting
order, allowing the current sort+apply-incrementally algorithm.

Matches my observation. Should we then get rid of all sorting outside
the GUI ?

Post by Michael T. Pope
You will need a mechanism to recalculate the compound modifier when one
of the underlying modifiers changes, which is asking for bugs due to
forgetting one or more special cases

... basicly the problem the current modifier caching has ... :o

Actually, there won't be any need for recalculation, as there wouldn't
be any need for any caches (outside the operators themselves), which
need to be maintained.

Let's take a little example: a colony's warehouse capacity.
(see Colony::getGoodsCapacity().

It's influenced by Buildings (mainly various depot/warehouse types).
So, for a calculation, we just ask the buildings how much storage
capacity they provide to the colony. (note that's yet global to all
goods types, so eg. buildings w/ storage capacity of goods they produce
or consume, isn't supported yet - maybe a nice future feature ?)

Assuming we dont just go the easy way of adding some getGoodsCapacity()
method to Building (which IMHO makes it a lot easier to understand),
but rather have very generic approach, the buildings could be asked
for an modifier operator with Id warehouse. This operator would just
act on the modifiers coming from the building's type. We can even easily
cache that operator (actually, it could even directly come from the
building type), or even do a partial solving at load time.

If the buildings change (eg. new warehouse being built) we dont need
any further action here, because the colony now asks a different set
of buildings.

Post by Michael T. Pope
Also the individual modifiers will still be needed for display purposes
so you will not be saving memory.

Do we really need the individual modifiers, or just groups of them ?

--mtx

Michael T. Pope

2017-01-31 09:03:05 UTC

On Mon, 30 Jan 2017 17:49:40 +0100

Post by Michael T. Pope
On Sat, 28 Jan 2017 17:26:46 +0100

Post by Enrico Weigelt, metux IT consult
I've seen that modifiers are sometimes collected in lists, sometimes in
sets. Is there a special reason behind that ?

Historical accident/inconsistency between developers.

Ok. Seems that Set is used more often (especially in FeatureContainer),
therefore duplicate modifiers (incl. all attributes eg. source) are
not allowed.

I can not think of a case where exactly equal Modifiers can coexist.
Goods parties come close but would have different timing. However FreeCol
has a history of surprising its developers with unexpected corner cases.

Post by Enrico Weigelt, metux IT consult
Digging a bit further, it really seems the whole duplicity problem
(perhaps even the need for the source attribute)

Source attributes probably started as a GUI feature.

Post by Enrico Weigelt, metux IT consult
Looks like a good place for subtle and hard to find bugs (which actually
hit me in a completely different area).

Correct. It has been.

Post by Enrico Weigelt, metux IT consult
Matches my observation. Should we then get rid of all sorting outside
the GUI ?

No. As I said, it is needed when the final value is calculated.

Post by Enrico Weigelt, metux IT consult
Actually, there won't be any need for recalculation, as there wouldn't
be any need for any caches (outside the operators themselves), which
need to be maintained.

I do not understand what you are saying here. AFAICT you were proposing
a type of compound modifier. Many(most?) modifiers can change, at least
in the sense of being added and/or removed. If its underlying modifiers
change, a compound modifier will need recalculation. It is effectively a
cache.

Post by Enrico Weigelt, metux IT consult
Let's take a little example: a colony's warehouse capacity.
(see Colony::getGoodsCapacity().
...

I take the point that we could stop shovelling features up from the
buildings to the colony, and indeed that would be cleaner.
The main virtue of the current scheme for warehouses is that at least now
they are expressible in the spec, rather than being hard coded. IIRC when
that code was written we were careful to keep the change minimal due
to major reworking of the production system at the time.

Post by Michael T. Pope
Also the individual modifiers will still be needed for display purposes
so you will not be saving memory.

Do we really need the individual modifiers, or just groups of them ?

If they can change, certainly you need them. You definitely can *not*
break the listing of applicable modifiers feature. The users want that
and demand accuracy. I do not want to have to fix those bugs again.

Cheers,
Mike Pope

Enrico Weigelt, metux IT consult

2017-01-31 16:14:49 UTC

Post by Enrico Weigelt, metux IT consult
Ok. Seems that Set is used more often (especially in FeatureContainer),
therefore duplicate modifiers (incl. all attributes eg. source) are
not allowed.

I can not think of a case where exactly equal Modifiers can coexist.

Okay, then most uses of Set could be lifted to List (except the few
cases where specific modifiers need to be removed from the caches),
correct ?

Post by Michael T. Pope
However FreeCol
has a history of surprising its developers with unexpected corner cases.

Can you name some, which I should consider ?

Post by Enrico Weigelt, metux IT consult
Digging a bit further, it really seems the whole duplicity problem
(perhaps even the need for the source attribute)

Source attributes probably started as a GUI feature.

Okay, what exactly are they used for here ?

I see some use in modifier propagation, but that's something I'm
currently trying to get rid of (at least except the spec objects)

Post by Enrico Weigelt, metux IT consult
Matches my observation. Should we then get rid of all sorting outside
the GUI ?

No. As I said, it is needed when the final value is calculated.

Seems I've got you wrong, and my thougts (operator order) still need
them ...

Do you know any case where that's also required across several object
types ? (IOW: need to sort *after* modifier lists from separate sources
had been cat'ed together, instead of just the source lists.).

OTOH: what about using TreeSet and giving Modifier the appropriate
compareTo() method, so they're always sorted automatically ?

I do not understand what you are saying here. AFAICT you were proposing
a type of compound modifier. Many(most?) modifiers can change, at least
in the sense of being added and/or removed.

Seems I wasn't a bit unclear. My proposal wouldn't change individual
modifiers anymore (and no modifier propagation). Instead the operators
would directly call through to the appropriate objects where the
modifiers originally came from.

For example, Colony could have a StorageCapacity operator, which
operates on the Colony's building list, calling their StorageCapacity
operators.
(okay, maybe not the best example, as we could implement that explictly
in Colony::getStorageCapacity()).

Post by Michael T. Pope
I take the point that we could stop shovelling features up from the
buildings to the colony, and indeed that would be cleaner.

Exactly. I'm currently working on that (can post patches if you like).

Post by Michael T. Pope
The main virtue of the current scheme for warehouses is that at least now
they are expressible in the spec, rather than being hard coded.

Yeah, but you need to know *exactly* how the modifier propagation
works - the whole spec is almost its own programming language, which
makes the whole thing pretty hard to understand and maintain.

Post by Michael T. Pope
IIRC when
that code was written we were careful to keep the change minimal due
to major reworking of the production system at the time.

Well, the problem with that is that this moves complexity from the
application to the spec and creates subtle interdependencies.

I'd like to get away from that complexity and just add new logic when
really necessary - with precise documentation.

For example, Colony's goods capacity just depends on the buildings,
which may only add capacity. Until we want some fancy magic like
Tile types adding X per turn, etc., I'd really like to KISS, like that:

https://github.com/oss-qm/freecol/commit/605e481100fe17d3838fe625593d425e95b34932

Post by Michael T. Pope
Also the individual modifiers will still be needed for display purposes
so you will not be saving memory.

Do we really need the individual modifiers, or just groups of them ?

Perhaps I was a bit unclear: certainly we've got several user visible
calculation steps, eg. how much defense points certain buildings
contribute, etc. But these always 1:1 mapped to an specific modifier
(individual Modifier instance) or instead all the modifiers from certain
source (eg. specific building) ?

--mtx

Michael T. Pope

2017-02-01 02:36:23 UTC

On Tue, 31 Jan 2017 17:14:49 +0100

Post by Enrico Weigelt, metux IT consult
Okay, then most uses of Set could be lifted to List (except the few
cases where specific modifiers need to be removed from the caches),
correct ?

I can not think of a counterexample. Indeed I have done this in a few
places in the past. I counsel small steps and plenty of regression
testing.

Post by Michael T. Pope
However FreeCol
has a history of surprising its developers with unexpected corner cases.

Can you name some, which I should consider ?

Post by Enrico Weigelt, metux IT consult
Digging a bit further, it really seems the whole duplicity problem
(perhaps even the need for the source attribute)

Source attributes probably started as a GUI feature.

Okay, what exactly are they used for here ?

I can answer both the above questions with reference to the comment in
Colony.getReducePopulationMessage.

More generally, source attributes are used (surprise!) so we can see what
object is providing the feature. There are a few places where a feature is
needed and we want to find the objects that can provide it. IIRC the AI
used to be full of hardcoded references to building/unit types, which I
replaced with calls to find type/s with the desired ability.

Post by Enrico Weigelt, metux IT consult
Do you know any case where that's also required across several object
types ? (IOW: need to sort *after* modifier lists from separate sources
had been cat'ed together, instead of just the source lists.).
OTOH: what about using TreeSet and giving Modifier the appropriate
compareTo() method, so they're always sorted automatically ?

AFAICT there are two places where sorting is required, at point of
calculation of the final result, and for display purposes to the player.
Actually, the first probably precedes the second anyway. Nevertheless,
both uses are terminal uses --- at this point all modifiers have
been collected, and the list is stable and not used further. Thus
it is actually disadvantageous to spend the effort keeping it sorted
while under construction.