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

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

sebauer@xxxxxxxxxxx (Sebastian Bauer) wrote:

> > - player_num_techs_with_flag() is going to be rather slow because 
> > have to go though all the techs every time.

> 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.

You mean in the player struct, have an array of number of
techs known for each tech flag, and update on load and
when player gains a new tech?  Should be doable.  Not 
sure if need to worry about what happens on the client...

> 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.

> No I think it's not good to implement the partisan as tech
> flags.

Yeah, actually I think I agree in both cases.

> 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)

I think this is what it should be.  

Actually I've been meaning to introduce a macro eg, BOOL() 
which converts to 0,1 (or TRUE,FALSE), to prettify some
things, especially in code I added in tilespec changes.

But we could have a flameware/competition about how BOOL() 
should be defined ;-)
Eg,  
#define BOOL(x) ((x) ? 1 : 0)
#define BOOL(x) ((x) ? TRUE : FALSE)
#define BOOL(x) (!!(x))
#define BOOL(x) (1-!(x))

> > - 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).

Yep, sounds good.

> > - /* 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 ;-)

Yep, ok.

-- David