Menu
▾
▴
gnuplot-beta
|
From: Harald H. <h.h...@tu...> - 2005-10-16 18:57:18
|
I do not really understand what the aim of Ethans patch of 2005-10-15 is.
It introduces a new structure type which can take bounds of plots and the
canvas. This is good.
But I still do not understand what it does. Because the variable canvas is
a pure copy of { 0, term->xmax, 0, term->ymax }. Why do we now have the
term->xmax and term->ymax and a copy of them? What does it really do?
Still, the clipping of
labels and arrow does not work at all.
Try this:
set size 2,2
set term png size 320,240
set arrow from screen 0.9,0.5 rto screen 0.2,0.0
set arrow from screen 1.1,0.7 rto screen -0.2,0.0
set output 'asdf.png'
splot sin(x)*cos(y)
quit
What we get is a plot that only contains labels below screen 1,1 and
arrows that start below screen 1,1. So, what was the aim of the patch?
What we need is to set the canvas boundaries at 'set terminal' (and not
in term_start_plot()) to
canvas.xright = term->xmax * xsize;
canvas.ytop = term->ymax * ysize;
In addition, the clipping routines, e.g. on_page, get_arrow3d, have to use
canvas.xright and canvas.ytop.
I have provided a patch that has been working before this new change in
CVS, and I will not provide a new one.
We will have to fix the old system (enabling sizes > 1) before we
can even think of changing it.
Ethan, why have you put your patch to cvs without discussing it in the
mailing list?
Regards
Harald
--
Harald Harders
h.h...@tu...
http://www.harald-harders.de
|
|
From: Ethan A M. <merritt@u.washington.edu> - 2005-10-16 19:12:52
|
On Sunday 16 October 2005 12:02 pm, Harald Harders wrote:
>
> But I still do not understand what it does. Because the variable canvas
> is a pure copy of { 0, term->xmax, 0, term->ymax }. Why do we now have
> the term->xmax and term->ymax and a copy of them?
Because it abstracts the notion of a clipping area to the generic case.
The 15 Oct patch by itself does nothing. Previously the routines
clip_line() and clip_point() were hard-coded to clip against the plot
boundaries. Now they have been generalized to clip against whatever
the current clipping area is.
> What does it really do?
"Patience, grasshopper". It paves the way.
It is much cleaner to change the infrastructure first,
with *no* intentional change in program behaviour, and then
introduce actual changes in separate incremental steps.
It makes it a whole lot easier to pinpoint the source of
a bug later on.
> Still, the clipping of
> labels and arrow does not work at all.
Indeed. But today's patchset adds generic clipping of arrows,
fixing bug #1187336 and a bunch of others. Give me another
hour or so of running test scripts though it.
And following that I will re-visit the clipping of labels.
That's a more difficult problem, however, because gnuplot itself
is not very good at predicting whether a text string will
exceed the clipping limits.
> Try this:
>
> set size 2,2
I am not interested in fixing bugs that arise from
setting size > 1. I thought we had agreed to work
toward making such kludges unnecessary.
--
Ethan A Merritt
Biomolecular Structure Center
University of Washington, Seattle 98195-7742
|
|
From: Ethan M. <merritt@u.washington.edu> - 2005-10-20 18:24:17
|
On Sunday 16 October 2005 12:12 pm, Ethan A Merritt wrote:
>
> today's patchset adds generic clipping of arrows,
> fixing bug #1187336 and a bunch of others.
Clipping arrow to the visible screen is messy because
the component lines and fill areas must be individually
clipped. This is particularly nasty if the start/end
coordinates have been forced to (unsigned int), because
negative coordinates are masquerading as large positive
numbers.
Clipping the vector itself before calling term->arrow()
may not be sufficient, because the head flag needs to be
adjusted so that a head is not draw on an end that was
clipped. (Or so it seems to me; perhaps there are cases
where one wants to draw the head anyway).
Until very recently, arrows were basically not clipped.
This caused problems on many terminals.
Current state of the code in cvs, as recently modified:
- full clipping is done in do_arrow(), which is a generic
routine called by most terminals. Because this is a
terminal entry point, the not-yet-clipped coordinates have
already been forced to (unsigned int). Casting them back
to (int) works, but is an ugly hack.
- The clipping in do_arrow() does not help terminals which
have a private term->arrow() routine (e.g. metapost, TeX
variants).
- Terminals which want to do clipping themselves (e.g. post)
set a flag TERM_CAN_CLIP, in which case the generic code
does not, in fact, clip the arrow. I think that's OK, but
I mention it for completeness.
I suggest that the clipping code below become a new routine
draw_clip_arrow(), analogous to draw_clip_line(), and all
places which currently call (term->arrow)() directly be
changed to call this intermediate layer routine instead.
This has the advantage that it will apply equally to all
terminals, can operate on coordinates before they have
been stuffed into an (unsigned int), and benefits from the
fact that draw_clip_line() and clip_point() can now be set
to clip against any desired bounding box.
This would replace the clipping code in do_arrow() and
possibly code in some individual terminal drivers.
The call site could specify a bounding box to clip
against if the default is not appropriate:
BoundingBox *clip_save = clip_area;
clip_area = &(BoundingBox *){ 0, xmax, 0, ymax };
draw_clip_arrow(...)
clip_area = clip_save;
draw_clip_arrow( int sx, int sy, int ex, int ey, int head)
{
/* 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;
clip_line(&sx, &sy, &ex, &ey);
/* Call terminal routine to draw the clipped arrow */
(term->arrow)((unsigned int)sx, (unsigned int)sy,
(unsigned int)ex, (unsigned int)ey, head);
}
--
Ethan A Merritt merritt@u.washington.edu
Biomolecular Structure Center
Mailstop 357742
University of Washington, Seattle, WA 98195
|
|
From: Hans-Bernhard B. <br...@ph...> - 2005-10-21 10:35:23
|
Ethan Merritt wrote:
> Current state of the code in cvs, as recently modified:
>
> - full clipping is done in do_arrow(), which is a generic
> routine called by most terminals. Because this is a
> terminal entry point, the not-yet-clipped coordinates have
> already been forced to (unsigned int). Casting them back
> to (int) works, but is an ugly hack.
IOW, that was exactly the wrong way of doing it ;-(
> This would replace the clipping code in do_arrow() and
> possibly code in some individual terminal drivers.
>
> The call site could specify a bounding box to clip
> against if the default is not appropriate:
>
> 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. Please consider making the clip area to be used a parameter
of draw_clip_arrow.
> 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()).
> {
> /* 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.
|
|
From: Ethan M. <merritt@u.washington.edu> - 2005-10-21 16:25:51
|
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
|
|
From: Hans-Bernhard B. <br...@ph...> - 2005-10-21 19:20:24
|
Ethan Merritt wrote:
> This is actually a reduction in the number of global variables.
Only at the outermost level. There's no change to the number of
individual global status variables, nor to their total size, by packing
things in structs. It's a null-sum game.
But storing globals in special storage to replace their values by
something else, for the duration of one function call, is still
atrocious, which should be avoided by
1) passing the relevant thing in as a parameter
2) having the routine use its own global status, which you set as
needed before calling it.
I'm against 2) because it again increases the number and size of
globals.
>>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.
Well, refactoring can be done a bit more builtin intelligently than
that: one could split these up into an actual engine holding a modified
version of the current clip_line()'s code:
clip_line_actually()
and make a new clip_line like this:
clip_line(parameters...) {
clip_line_actually(&global_bounds, parameters...)
}
No interface change to the outside, but new code can use
clip_line_actually() if it doesn't want to use the global default
bounding box.
> Here again I agree in principle, but that would mean reworking
> all the existing lower-level clipping routines as well.
Not necessarily. Just duplicate them and have them take coordval's.
|
