[Freeciv-Dev] Re: Merged the both tech patches a little bit

[sebauer@xxxxxxxxxxx (Sebastian Bauer)]

Hello David

> I had a look at this, and there were a few things I
> wasn't that happy with.  Problems/comments:

> - Didn't compile, but not really your fault: turns out 
> AF_BRIDGE is a define for something included in packet.c 
> (on Linux); could do it a bit different (AF_BUILD_BRIDGES?) 
> or change the AF_ prefix (TF_*?)  Perhaps second is better 
> since AF_* = "address families" for some Linux/Unix something.

Didn't know that ;-)
Ok, I'll take TF_#?.

> - You didn't merge my government stuff well enough IMO: 
>  +#define A_MONARCHY  (get_government(G_MONARCHY)->required_tech)
> etc is ok in aitech.c: get_government_tech(), because that will 
> go away when the dynamic government eval stuff is integrated;
> its not so ok in aicity.c, because that is not planned to change
> (well, not by anything implemented or intended so far).
> Also, replacing A_CODE with req of B_COURTHOUSE is I think wrong:
> A_CODE is important (here) because its a common req to Republic
> and Monarchy, nothing to do with Courthouse.  Hence my more
> involved change (changing ruleset files) for this stuff.

See below (the last thing)

> - player_num_techs_with_flag() is going to be rather slow because 
> have to go though all the techs every time.  I guess its may be
> acceptable (eg, until profiling), but it worries me a little, eg, 
> if have to call this function lots of times somewhere (eg, several 
> times per city on updates).  A better way to do this would be 
> something like the role_unit_precalcs() stuff, so would only have 
> to iterate through small number of techs.  But this is a bit more 
> complicated -- actually, it was thinking of having to do something 
> like this which made me not bother to implement tech flags yet :-)

Will be changed later, of course ;-)

Actually I don't bother about optmization. I use a 68040 processor
with 25Mhz and it's fast enough ;-)
Of course optimization is important but function is more important
at the moment IMO.

But I thought on something differnt, how to speed up this.
The number of the tech flags only change when a player gets
a new tech and so I'd like try to change the function to simply
return a number without counting anything.
Should be the fastest which is possible, because this even
can be inlined (or a macro) ;-)
I only hope that this is possible.

> - I would prefer to make the number of tech variable
> separately, _after_ a patch to take out the hardwired techs.

Ok. You have done this already ;-)

> - The Temple tech effect is of the same sort as the Colloseum
> and Cathedral tech effects, and I would prefer these all be
> done the same way: either do Temple like others, or change 
> them all to use tech flags (of first former, later latter).

Connecting techs flag with buildings is in fact, not so good,
(the same counts for units) IMO (that's why I stated that
the flag might be removed). The tech.ruleset should be as
simple as possible because is hard enough to create a good
tech tree.

> - Its a bit disappointing that having done tech flags, one
> still has to use a tech list for the Partisan techs.  I guess
> the main reason for this is that you think Partisan techs should
> be in units ruleset.  Well, I'm not sure this is necessary (its
> related to partisan units, but it can also be considered a tech 
> properly), and even if it appears in the units ruleset, it could
> be implemented as tech flags internally?

No I think it's not good to implement the partisan as tech
flags. The reason is simple: Maybe a mod pack writer
wants to have two or more partians units (e.g. for
differnt tech trees)?

Ok, this wouldn't be solved by the list thing, too. But
if I remove it, we also can remove the code associated
with that too. But this code might be useful for other
things. And since I will try to chance this before the
next release it really doesn't matter.
But with doing the partisan_req in unit.ruleset we can
say that we are nearly ready (execpt the A_NONE thing)
with the techs.ruleset.
It's is really more work to add such a flag now which
will be removed then anywhy soon. And since you already
have implemented partisan_req it really makes no sense...
(IMO ;-) )

> - player_can_build_air_units() is defined but never used?

Has been used by my old patch (alpha) but it's not used
anymore. I'll remove it of course.

> - Following changes:
> int unit_flag(enum unit_type_id id, int flag)
> {
>   assert(flag>=0 && flag<F_LAST);
> -  return unit_types[id].flags & (1<<flag);
> +  return (int)(unit_types[id].flags & (1<<flag));
> }
> Is this necessary/useful?  (Same for unit_has_role().)
> The return type is int anyway, so does the cast achieve
> anything at all?

Yes, at least here it produce warnings without this cast.
Also in some other positions, which I haven't changed.
The warning of my compiler is correct because
unit_types[id].flags in unsigned int.

But my solution is not the best one ;-)
It better should be changed to:

-  return unit_types[id].flags & (1<<flag);
+  return (unit_types[id].flags & (1<<flag))?TRUE:FALSE;

But I actually don't know if it makes problems
(e.g. if a caller assumes that the flag is on the correct
place)
This change would be necessary anywhy if we get someday more
than 32 flags.
I will check this and provide a seperate patch later
(if nobody else do it)

> - get_units_with_flag_string():
> I see the point, but its not clear to me that this is necessarily
> a good thing: what if there are lots of units with a flag: then
> mentioning every one when referring to an effect will be annoying
> to the user.  I think its ok either to refer to a unit type
> (eg, "Settler type units"), or the first such unit (eg 
> get_role_unit(F_SETTLERS,0)) and "similar units".

I think it if there are not more than e.g. 4 units with
the same flag it's not annoying. If there are more than
4 units with the same flag we can still fall back (within
this function) to use other textes (e.g. your last suggestion
would be easy to implement in this function).

>  Also, if it 
> is required, this should use num_role_units() and get_role_unit() 
> instead of iterating over all units.

Yes, I didn't know about get_role_unit() and it's of course
better and easier to use them.

> - /* there should be something like astr_append() */
> Yeah, I kind of agree, although I've been a bit torn between
> keeping the astring stuff very minimal and simple, and building
> it into a fully-fledge string class :-)

Yes. But astr_append is very easy and astring would be still
simple. It just saves some lines ;-)

> - You only updated the default rulesets; civ1 and civ2 versions
> should also be kept working.

Yes, of course. But because it was not completed I didn't
do that yet.

> - I'm a bit uncertain about putting in the airbase flag until
> it actually gets implemented.  (Should be no problem to add it
> later?

No problem ;-)

> - /* game.num_tech_types not initialized here! -- sb */
> True, but actually this can be fixed elsewhere now: the
> comment in game_load() about freeing the current savefile 
> before loading rulesets is wrong, ever since strbuffermalloc 
> got replaced by sbuffer.c; I've been meaning to change this
> so the rulesets get loaded earlier when loading a savegame; 
> actually, I'll check in a change for this shortly.

Maybe, but I'm still not so familiar with freeciv.
But I'm learning ;-)

...

I saw you done this already. I will use it in the next
version of my patch.

> - Some uncessary indentation changes in some places (eg, 
> especially in ruleset.c); please try to avoid this.

Yes. I didn't know how many chars a tab spans. But
then I found out that it was 8 chars, I simply forgot
to remove this.

> Hmm, did you notice one of my faults is perfectionism? :-)

> To keep things moving on this I think I'll check in my gen_tech 
> patch I posted earlier, but with the understanding that some 
> of it might change again soon.  So feel free to make a new 
> patch which changes some things to use tech flags, and

Yes, I'll start working on this soon.

> (preferably separately) changes the number of techs to be 
> variable.  Actually, latter change should be pretty trivial.

Ok, I try this. But I cannot guarante this.

> (Incidently, I was thinking that when techs become variable,
> we should take opportunity to take definition of "None" tech
> out of ruleset files, and make it happen automatically.
> That is, since user should never need to change anything 
> about the A_NONE tech.)

Should be possible, but because I still didn't understand
everything in freeciv I try to make the chances as minimal
as posible in this areas ;-)

bye,
Sebastian Bauer