[Freeciv-Dev] Re: Tech cost patch v14 (PR#1082)

[Juha Litola <slave@xxxxxxxxxxxx>]

On Sun, Dec 09, 2001 at 02:39:19PM +0100, Reinier Post wrote:
> On Sun, Dec 09, 2001 at 01:19:04PM +0200, Juha Litola wrote:
> > 
> > find_unknown_req_techs searches all requirements for technology
> >   that are not yet researched. 
> 
> But it also marks them in the array, except the last one, which is
> the return value.  This isn't clear from the name.
No, it doesn't do that. It finds all requirement techs including
itself and stores findings in array. It returns number of requirements
found. 

> I'd prefer the name
>   find_and_mark_unknown_prerequisites()
Would find_unknown_prerequisites be enough.

> I'd also prefer it to just fill the array and return void.
> That way it's much clearer what happens where.
I return value from it so we don't need duplicate functionality in
num_unknown_req_techs.

> Now when I look at the reason, it's because ai/aitech.c uses its own
> marked array of requirement techs, the CACHE, which memorizes which
> tech is a requirement for which, like my code did, except that it
> happens every time ai_select_tech() is called.  This is inefficient.
> And each time it will call your find...() function, which marks its own
> array which makes it doubly inefficient.  It would really be better
> to do this whole tech tree traversal once, hide it in your own code,
> together with any caches you use to avoid recomputation, and
> only expose a function to display results for use by ai/aitech.c.
This change would be sensible. It would also be big and would need
lots of testing. If it becomes any larger, I don't think it will
ever get into cvs. And if I'm not sure that it'll end in cvs, 
I won't be writing it. 

> > tech_cost gives cost of given technology
> > 
> > goal_tech_cost gives cost of reaching given goal
> 
> A goal tech is a tech, so why two functions?  I thin kthe names should
> prevent this question.
tech_cost_for_reaching_goal is pretty long name. If those are preferred,
I can change it.

> But req[tech] doesn't collect technologies, it numbers them, it would
> be logical to update req[tech] with the return value before returning.
req[tech] just marks that this technology was traversed. We could
update it with return value, but currently need only zero/non-zero
knowledge for given tech.

> The most efficient approach simply stores all values (both req[] and
> CACHE) for all players and only updates them when the tech tree is set
> or a technology is researched.
I agree. But I'm not sure that adding rewrite of aitech.c to this patch
would be accepted in any time frame involving near future. 

I think basic functionality (as in this patch) should first be 
integrated to cvs and only after that we should think about cleaning
existing cruft out.

-- 
// Juha Litola

pgp5ix9lPmUjV.pgp
Description: PGP signature