[metapost] Trying to figure out MPLib

[Shriramana Sharma]

Hello Taco and thanks for your work on this -- this is really great! I
am replying in some delay because I wanted to review the code
carefully [and yes I did look at some of the black magic too! ;-)].

On Sun, Aug 5, 2012 at 6:21 PM, Taco Hoekwater <taco at elvenkind.com> wrote:
>         <top>/metapost-1.212$ patch -p0 < ~/mppath-demo/mp.w.patch
> Then build libmplib.a, and copy libmplib.a, mplib.h, mpmp.h, and
> mplibps.h to the mppath-demo folder.
> From there, 'make' builds the simple test application 'test'.

Done. I used the following shorter Makefile:

OBJECTS = test.o fixkpse.o mppath.o
all: $(OBJECTS) mppath.h libmplib.a
	gcc -Wall -g -o test $(OBJECTS) -L . -l mplib  -lm
.c.o:
	gcc -Wall -g -c $<

>   mppath-demo/mppath.h
>         -- header for the functions in mppath.c, including documentation
>   mppath-demo/mppath.c
>         -- internals, containing quite a bit of black magic you should
>         not look at too closely

One small suggestion for change:

mppath.h: extern int mp_cycle_knotpair (MP mp, mp_knot *p, mp_knot *q);
mppath.c: int mp_cycle_knotpair (MP mp, mp_knot *p, mp_knot *q)

mppath.h: extern int mp_final_knotpair (MP mp, mp_knot *p, mp_knot *q);
mppath.c: int mp_final_knotpair (MP mp, mp_knot *q, mp_knot *first)

Could you please change the labeling as following:

mp_close_path (MP mp, mp_knot *p, mp_knot *first ) ;
mp_close_path_cycle (MP mp, mp_knot *p, mp_knot *first ) ;

I feel this would be easier to relate to the drawing commands because
you either abruptly stop at a point or you say "cycle".

> * mp_knot lists have to be circular, so if you forget to close the
>   path with either mp_final_knotpair or mp_cycle_knotpair, the solve
>   function will fail.

Um, I think this will happen only if you try to apply solve on a
single lone knot created using create_knot, because IIUC a knot *list*
is only produced if you use append, which internally uses
mp_cycle_knotpair.

> * the mp_set_knot*_* functions should be called only after two knots
>   are already connected via mp_final_knotpair or mp_cycle_knotpair,
>   otherwise the internal connections become messy.
>   This is the main reason for mp_append_knot(), which is just a chain
>   of mp_creat_knot, mp_set_knot, mp_final_knotpair.

Actually in your code it's mp_cycle_knotpair and not final_knotpair.
But I'm not comfortable with your using the path-closing functions to
link up two knots. (I realize you've called it blackmagic but I'm just
putting it on the record.)

I think the only reason you need the cycle/final_knotpair here is to
set the next pointer, left/right_type (to open) and tension (to 1). In
which case, would it not be better to have another internal function
"link_to" or something? Then you can substitute the mp_cycle_knotpair
within append by that, and use cycle_knotpair (aka close_path_cycle)
only to actually close the path. Of course, cycle_knotpair aka
close_path_cycle would also use link_to to link the last knot to the
first one.

IIUC in non-cyclic paths there is no requirement for the last knot's
next pointer to point to the first one (since there will be no join in
such cases between the final and initial knots -- even if they are
mathematically identical). But perhaps you are trying to get a common
condition "if next == first then break loop" for iterating over loops.
In which case the only thing which would be necessary would be to set
next to first and link_to (which modifies the left/right-types and
tensions) would not be appropriate.

[Obviously, I'm thinking out loud here -- you guys surely know the
implementation better than I. So ignore and if possible point out if
I'm saying something logically wrong.]

> * mp_solve_path changes the path in-situ. Right now, this involves
>   some copying and black magic, but once the code moves to the core
>   mplib, that part becomes simpler (and therefore faster).

Great!

> * struct mp_knot is currently defined in mplibps.h. This (and its
>   internals) will change in mplib 2,

Please do so -- it should go to the generic header. If I'm not using
the PS output I should not need to include mplibps.h (for a clean
API).

> Have fun,

I will! :-) Thanks a lot!

-- 
Shriramana Sharma