[Freeciv-Dev] Re: (PR#8754) effects patch

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=8754 >

Vasco Alexandre da Silva Costa wrote:


> s/great_library/do_adv_parasite_effect

do_tech_parasite_effect.

The term "advance" isn't good, and "adv" or "a" aren't good shortenings.

>>I am so not a fan of game.palace_improvement (though I'm not sure yet
>>what you're attempting to accomplish here).

There are client-side special cases for palace and city walls.  Both get 
displayed specially by the client, so although the client doesn't know 
what buildings are in enemy cities it does need to know that the effects 
are present.  Currently this is handled by reading the short city packet 
and adding the buildings manually.  This is an ugly hack, and could give 
problems (for instance if multiple buildings could provide the effect).

Handling this is tricky.  I'm not sure what should be done in the long 
term.  In the short term a game.palace_improvement and 
game.citywalls_improvement value seems like a reasonable compromise. 
However these (at least game.citywalls_improvement) need not be 
specified by the ruleset as Vasco does now but can probably be deduced 
at runtime.

Although, since the palace has so many other special cases it may be 
necessary to specify _which_ EFT_PALACE building gets all the special 
cases.  Tying the special cases to the EFT_PALACE effect itself would be 
very hard since many of the cases are singular and more than one 
building may provide this effect.

And a game.citywalls_improvement variable may not be needed at all if 
it's possible to just call find_improvement_with_effect(EFT_CITYWALLS) 
in the client code to determine which improvement to "add" to the city.

> It is short for "element". It is like using "i", "it", "iterator" or
> "node" as variable names.
> 
>>Why Island->Continent?
> 
> I thought about this for quite some time. I never liked giving multiple
> names to the same thing. It just confuses someone reading the code.
> I did a "grep -i" of the source code and the term continent beats
> island nearly 2:1. So I renamed it to continent in this patch.

There are a lot of places where we use multiple names for the same 
thing.  Per and I discussed it a while ago and agreed that this is bad. 
  So we should pick one, document it, and stick to it.

island / continent => either is fine with me
advance / tech(nology) => advance is bad; tech is much better
building / improvement => building is more clear (terrain improvements)
infrastructure / improvement => both names are bad

jason