Re: [PATCH] x11 terminal respects the requested aspect ratio

[Ethan A M. <sf...@us...>]

On Friday, September 28, 2012 01:35:07 am Dima Kogan wrote:
> > On Thu, 27 Sep 2012 12:00:10 -0700
> > Ethan Merritt <merritt@u.washington.edu> wrote:
 
> > > The problem seems to trigger when YMAX crosses exactly 10000, 
> > > Perhaps it's a formatted field overflow from 4 digits to
> > > 5 in the y coordinate?
> > 
> > I think that must be it.  The coordinates sent from x11.trm to
> > gnuplot_x11 use format statements like PRINT2("V%04d%04d\n", x, y);
> > 
> > It looks like either the field width has to be increased everywhere
> > to 5 digits, or else the new rescaling code needs to rethought
> > (perhaps by adjusting both xsize and ysize rather than only adjusting
> > ysize?)  Increasing the field width has the drawback that more bytes
> > will be sent over the channel regardless of the window size, which
> > can slow down the communication.
> > 
> > What to do?
> > 
> > 		Ethan
> 
> Hi Ethan.
> 
> Good you found the issue. Do you know why field widths are specified at
> all? Why don't we PRINT2("V%d %d\n") and then sscanf("%d %d", ...) on
> the other side? No efficiency is gained by specifying the widths, it
> only builds in limitations and makes the code brittle, as we have just
> observed.

The basic X11 framework was in place long before my involvement, so I do 
not know.  My best guess is that the fixed field width was chosen so as 
to reduce the amount of data sent through the gnuplot->gnuplot_x11 channel.
Adding a space in front of every coordinate pair would increase the
traffic by as much as 25%.

As I recall, when the polygon encoding was switched from formatted to
binary (2004), the advocates of binary encoding/decoding had benchmarks
showing data transfer really was a performance bottleneck, and reducing
the number of bytes sent over the channel resulted in faster plotting.
I do not know if this would still be true on modern machines.

We should benchmark before and after your latest patch.

> I'm attaching a patch that removes hardcoded field sizes from ALL %d
> and %u fields. Some extra care had to be taken in places where a
> numerical field is followed by a string (%s), since the '4' was
> hardcoded in those places too. The bug you described is now gone, and
> the code is more robust.
> 
> I noticed another, very related issue. x11.trm has
> 
>     /* badly outrange labels can overflow into text field */
>     if (x < 10000 && y < 10000) {
> 	PRINT3("T%d %d %s\n", x, y, str);
>     }
> 
> Here we're hardcoding the 10000 again. This has the effect of not
> printing the legend label when the window is skinny (as you were making
> it). This test needs to be adjusted or removed entirely; not sure
> which. CVS says this test was added by lhecking in 1998. This predates
> all of sourceforge. Newsgroup logs go back that far, but I didn't see
> any mention of this issue. Thoughts?

That does look like an early instance of the same problem.
But I wonder how that code ever triggered?

> Finally, in writing the patch, I stumbled on what looks like a bug in
> what looks like dead code. In gplt_x11.c, look at the
> 
>     else if (*buffer == X11_GR_FILLED_POLYGON) {	/* filled
> [snip]
> I suspect the whole block is dead, and this bug is thus
> never hit. If this whole block is truly dead, it should be removed.
> dima

I think it must be left over from when the choice between binary/ascii
polygon encoding was a configuration option.  Since 2009 the ascii 
option no longer exists.  So yeah, that section is dead code.

	Ethan