[Freeciv-Dev] Re: [Freeciv] rfc: autoattack

[Gregory Berkolaiko <Gregory.Berkolaiko@xxxxxxxxxxxx>]

On Sun, 17 Nov 2002, Per I. Mathisen wrote:

> On Sun, 17 Nov 2002, Thomas Strub wrote:
> > I use the key "k" for the autoattac"k".
> > Think only a sprite for the "k" is missing.
> 
> Wow, that was fast.

Yep.

I had a quick glance over the patch and it confirmed my earlier concerns 
about the concept.  The autoattack is triggered by movement.  To execute 
autoattack of range greater than 1 the unit needs to move, which will 
trigger another autoattack which will trigger another autoattack etc.

The result may become unpredictable and also the code will be traversed in 
strange ways.

Range 1 autoattack triggered by movement is ok, but is not that useful.

Also, another complaint is lack of comments.
Function header comments like
/**********************
 ...
***********************/
must become a thing of the past.  There is always something sensible to 
write.  At the time of writing you might think that the function is 
obvious, but it rarely is.

Best wishes,
G.

> 
> Some short comments so far (haven't tested it yet):
>  - Please remove the old autoattack code.
>  - Reuse the "A" key and sprite which the old autoattack code used. (This
> conflicts with A for autosettlers, but that doesn't matter, since by the
> time we have a modpack with military settlers, I bet Raimar has already
> moved autosettlers to the client anyway.)
>  - ACTIVITY_SERVER_AUTO_ATTACK -> ACTIVITY_AUTO_ATTACK
>  - Why are you checking in a 3 square radius? My suggestion was adjacent
> units only. Doing it over a larger distance has many pitfalls and may not
> be better (harder to know what it does, harder to trust).
>  - If you only don't do 3 square radius, then don't use do_unit_goto(),
> use handle_unit_move_request() instead, as goto is overkill when you only
> are to move one square. This should simply a lot of the other code too.
> 
>   - Per