[freeciv-ai] Re: settlers.c question

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 09:59 PM 02/12/09 +0000, Per I. Mathisen wrote:
>
>On Wed, 4 Dec 2002, Ross W. Wetmore wrote:
>> This is what I worked out once upon a time for the corecleanups.
>...
>> Certainly is_non_allied_unit_tile() doesn't capture all the subtleties
>> this code has built into it. I also think you have only begun to scratch
>> the surface and need to scratch a little harder to work things out in
>> their full glory.
>
>I don't think so. I see no glory, I see only one big unholy mess.

Beauty is in the eye of the beholder ... :-)

One's first beer typically tastes horrible, but after a few it never
seems quite so bad :-) :-)

>> /* Settlers can help current operation, treat as unassigned */
>> if (same_pos(myunit->x, myunit->y, x, y)) {
>>   if (unit_flag(myunit, F_SETTLERS))
>>     return FALSE;
>> }
>
>Fair enough. Easy little check that might be added. But _only for terrain
>improvements_ and not also for city creation, as you have it.

You can only create one city, first one to try wins, and we are
already on the tile, so there is nothing to lose. 

It will be a good test of the handler code if the loser resets and 
recomputes a valid move *this* turn because it has movepoints left 
after failing. And if a settler arrives a turn or so later because it
didn't notice the new city, then having it here where improvements
are probably needed rather than where it obviously didn't have a 
better thing to do might be a good thing too.

In short the fine tuning you want to add shouldn't be needed as it
doesn't really change anything except maybe to delay the city
founding if this unit is here and the other assignee isn't. 

And actually, if F_SETTLERS is different from F_CITIES then this
probably already makes that absurd distinction (I still think it
is better to have a settler do both than split into two units -
but that is another discussion).

>> else
>> if (myunit->activity == ACTIVITY_GOTO
>>   && same_pos(myunit->goto_dest_x, myunit->goto_dest_y, x, y)) {
>>-  bool is_military = is_military_unit(myunit);
>>   bool is_settlers = unit_flag(myunit, F_SETTLERS);
>>   unit_list_iterate(map_get_tile(x, y)->units, punit)
>>     /* non-military assignments are off if currently enemy occupied
>>      *   or being actively worked by an allied settler */
>>-    if (!is_military)
>>       if (!pplayers_allied(unit_owner(punit), pplayer)
>>-*       || (is_settlers && unit_flag(punit, F_SETTLERS)) )
>>         return TRUE;
>
>What is this talk about military assignments? Aren't we evaluating settler
>assignments here?

Leftovers from experiments. But while the code is probably never called
when myunit is not a settler, it doesn't distinguish in its interface
and can legally be called by any unit type. So one can argue it should
deal with it, or say explicitly it doesn't.

The rationale for military vs non-military is typically the opposite
as the more that arrive at the rumble-fest the merrier the party :-).

It would be really neat if assignments were usable to track the hot
action points and coordinate multiple unit military actions, but I
never really made it far down that path in the experiments.

>Without the military check, which makes no sense, this achieves the same

You haven't thought enough to understand what a military check might
have meant, since your preconceived notions of what this function is
suppoed to do won't let you - clearly it did make sense at one time 
or no one would have put it there, so it only makes no sense to you 
is a fairer statement and better point to start your analysis from :-).

And now that we have sensitized you with this comment ... on to the
next analysis.

>as a call to is_non_allied_unit_tile(), only with a lot more code, and we
>don't check for the case where we contemplate building a settler to
>improve this position (why contemplate improving a position held by the
>enemy?).

You have completely missed the point of the "if" clauses. The absolute
tile state only has bearing if *myunit* unit is there or targetted to go 
there, which probably means it is not contemplating an improvement task 
but checking up on an existing assignment.

Also, presuming that an is_already_assigned() check is only called
with an intent to improve the tile, rather than check for assignments
as it says it does seems a trifle presumptuous.

Assuming the specific code that sets an initial improvement task doesn't 
also consider the necesary conditions for this in a more focussed and 
comprehensive way (since that is what it does) is probably a bad mistake
And bundling those checks into a generic assignments check is worse.

is_non_allied_unit_tile() is not near equivalent to the full logic here. 


If you remove the indicated lines by imposing conditions on myunit,
then this sub element is pretty much is_non_allied_unit_tile() without 
function overhead.

But the punit F_SETTLERS check also means that you don't compete with
allied settlers. So dropping the -* line completely is also too much
and with it you don't even come close to is_non_allied_unit_tile()
since that doesn't notice allied settlers.

>In short, is_non_allied_unit_tile() is better.

It changes the behaviour radically, which may or may not be desired,
but at least needs to be recognized and a rationale given befoe it is
done. For that you need to understand the current code a little better
since you still appear to have missed several subtle elements in your
reluctance to scratch a little deeper :-).

BTW: I did not duplicate the original logic either, but made some minor
     changes after deep thought and some play testing such as the fall
     through for non-settlers on the same_pos condition rather than 
     treating it the same as on the way :-).

This code should probably have WARNINGs posted that people should read 
it before making assumptions about what it does. That might slow down
introduction of bugs by people changing it without thinking it through.

It should also document its interface restrictions and assumptions if
in fact it makes assumptions about things like myunit that are not
immediately obvious or required.

>  - Per

Cheers,
RossW
=====