Re: [RFC] introduce new routine draw_clip_arrow()

[Ethan M. <merritt@u.washington.edu>]

On Friday 21 October 2005 03:38 am, Hans-Bernhard Broeker wrote:
> > BoundingBox *clip_save = clip_area;
> > clip_area = &(BoundingBox *){ 0, xmax, 0, ymax };
> > draw_clip_arrow(...)
> > clip_area = clip_save;> 
> Objection, your honour --- we should absolutely not add more global 
> variables.

This is actually a reduction in the number of global variables.
Before this we had xright, xleft, ybot, ytop all as globals.
Now all 4 of these are contained in a single (struct BoundingBox)clip_area.

> Please consider making the clip area to be used a parameter  
> of draw_clip_arrow.

This would not reduce the requirement for a global pointer, 
unless the lower level clipping routines clip_point() and clip_line()
were also modified to take an extra parameter.  I have no objection
to that as a design goal, but it would require changing the current
code everywhere the routines are called. 
[greps a bit.... that's about 45 places, each of which may in turn
need to be modified to work with coordvals]

 
> > draw_clip_arrow( int sx, int sy, int ex, int ey, int head)
> 
> I'm not at all sure that doing this in ints is a good plan.  These
> should probably be coordvals (or even a complete struct arrow_def, by
> joining draw_clip_arrow() with get_arrow()).

Here again I agree in principle, but that would mean reworking
all the existing lower-level clipping routines as well.
If that were done in one large sweep over the code, draw_clip_arrow()
would be just one caller out of 40+. So I don't think that a 
long-term goal of revamping all the clipping to use coordvals is a
reason not to introduce arrow clipping first.

 
> > {
> >     /* Don't draw head if the arrow itself is clipped */
> >     if (head == BOTH_HEADS && clip_point(sx,sy))
> >         head = END_HEAD;
> >     if (head == BOTH_HEADS && clip_point(ex,ey))
> >         head = BACKHEAD;
> >     if (head == BACKHEAD && clip_point(sx,sy))
> >         head = NOHEAD;
> >     if (head == END_HEAD && clip_point(ex,ey))
> >         head = NOHEAD;
> 
> This sequence hints at a design flaw.  We should have individual flag
> bits for each head, not a single combined enum name for both.  Put in 
> another way, BOTH_HEADS should be killed.

There I definitely agree.

Harald -  Do you want to have a look at that?
On a related note, your patch on SourceForge only adds the
backwards head code to do_arrow(); it doesn't add it for the
drivers that have private code (tgif, metapost, texdraw,
probably others as well).



-- 
Ethan A Merritt       merritt@u.washington.edu
Biomolecular Structure Center
Mailstop 357742
University of Washington, Seattle, WA 98195