[Freeciv-Dev] Re: (PR#3928) Convert client to use PF

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Mon, Apr 07, 2003 at 08:43:28AM -0700, Gregory Berkolaiko wrote:
> 
> The resulting code is very nice and clear, but still poorly commented.  I
> added some comments where I knew the answers and left "Glip:" marked
> questions where I didn't.  Please answer these questions by comments!!

>  /********************************************************************** 
> -...
> +  PF callback to get the path with the minimal number of steps (out of 
> +  all shortest paths).
>  ***********************************************************************/
> +/* Glip: can we have a better name please? */
> +static int my_get_EC(int x, int y, enum known_type known,
> +                  struct pf_parameter *param)

You can create one. I always use "my_". If I would have anonymous
function I make give them no name at all.

> +/* Glip: dir is enum, in many other places too. */

The use of direction in the other cases is more or less buggy. You
loose for example if dir>=4 && reverse(dir)>=4. There is now code to
test for this. So some kind of "enum direction8_folded_to_0-3" is
needed. With the corresponding functions for the conversion.

> +  Glip: Can we somehow get rid of this function please?  This and 
> +  get_drawn_char confuse me.

No. But I added comments.

I have processed the remaing points. It compiles and runs simple
tests.

> I didn't change any of your code, only comments, but I added one fix to 
> prevent client crashing (also marked by "Glip:") and threw in a couple of 
> fixes of the path_finding, for good measure.  These were the bugs I 
> discovered while doing rampage, we can commit them separately or as part 
> of your patch.  The bugs are:
> -- construct_path tried to step back beyond the initial position of the 
>    path
> -- zoc-handling prevented paths ending on a military target.

It was to be expected that we find errors in PF. These should be
commited separately.

> Since goto.c was practically rewritten, I suggest we run indent on
> it (and ask it to get rid of tabs too).

Done.

> On Sat, 5 Apr 2003, Raimar Falke wrote:
> 
> > > > Open issues are:
> > > >  - the network interface doesn't support dangerous tiles
> > > >  - the path execution at the server doesn't support dangerous tiles
> > > >  - the network interface is quite weird (for example it divides the
> > > >  path into 20-positions chunks while a chunk could holds 1500/2=750
> > > >  positions)
> 
> So this means that if I attempt doing goto with a trireme I will get a 
> core?  I think it should be fixed before this is committed.

Untested: the server will just go the steps and will not wait at the
steps at which the unit should wait. So you don't get a core but a
sunken trireme.

> > > 5. It's a bit hard to tell just looking at the diff, but it seems
> > > you have committed the greatest possible travesty: recalculating map
> > > each time the mouse is moved.  If it is so, you have to repent, fast
> > > for three days and fix it by using pf_get_path on the old map.
> > 
> > You have looked correct. Fixed.
> 
> But have you fasted?

Can I do more sport in exchange? This is also a burden.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "We've all heard that a million monkeys banging on a million typewriters
  will eventually reproduce the entire works of Shakespeare.
  Now, thanks to the Internet, we know this is not true."

cc8.diff.gz
Description: cc8.diff.gz