Re: Short vectors are not plotted

[sfeam <sf...@us...>]

On Saturday, 11 November 2017 17:50:18 you wrote:
> Ethan A Merritt <EAM...@gm...> writes:
> 
> > On Saturday, 04 November 2017 21:43:09 Dima Kogan wrote:
> >
> >> 1. This patch uncovered an inconsistency in much of our code: when
> >> converting a floating-point terminal coordinate to an integer one,
> >> sometimes we round (either with (int)(x+0.5) or by calling
> >> axis_map_toint()) and sometimes we floor ( (int)(x) ). Looks like it's
> >> more or less random which one we pick. We should always round, I
> >> suspect. This patch series doesn't attempt to resolve this, but a future
> >> set of patches should.
> >
> > Your patch set definitely runs aground on that point.
> > I tried to compare results before/after the patches but there are so many
> > single-digit coordinate changes throughout that it's impossible.
> > Yes it may be worth it to review the double->termcoord conversion
> > everywhere in the program, but let's disentangle that from changes to
> > handling short vectors.
> >
> > Here's how I tested:
> >     ./gnuplot-before -e 'set term post color' all.dem < /bin/yes > all_before.ps
> >     ./gnuplot-after -e 'set term post color' all.dem < /bin/yes > all_after.ps
> >
> >     diff -ur all_before.ps all_after.ps > all.diff
> >     diffstat all.diff
> >
> >     diffstat all.diff
> >      all_after.ps |234431 +++++++++++++++++++++++++++++++-----------------------------
> >      1 file changed, 121820 insertions(+), 112611 deletions(-)
> 
> That is a great way to test this! Definitely keeping that in mind for
> the future.
> 
> I have new patches (attached). This is a similar series from before,
> except with finer granularity. The bulk of the main patch lives in patch
> 0003. This is the same as before except:
> 
> - The meat of the changes in draw_clip_arrow() has been left out
> - The previous round/truncate behavior was carefully preserved
> 
> Thus your all.dem test results after applying patch 0003 should match
> those before applying any of these patches.
> 
> Then patch 0004 is a short one that removes the compatibility off-by-one
> changes and patch 0005 actually updates draw_clip_arrow() to handle the
> short vectors.

Very good.
That allows me to test before/after applying 0005 so that I can evaluate
the intended change by itself.  Here's what I see.

1) Something has gone wrong with the arrowhead direction.
    Try running arrowstyle.dem.   All the arrows at the page bottom have
    inverted arrowheads.

2) The patch has lost the distinction between "fixed" arrowhead size and
     the default variable arrowhead size.  Many of the arrows in the
     arrowstyle demo look strange because their heads are too big.

3) Arrowstyle 6 in the demo is supposed to show open unfilled heads.
    After patch 0005 the arrow shaft extends through the head so it no
    longer appears unfilled.

4) Not a result of your patch but I realized while testing - The code in
    post.trm that was messing up was intended to enforce solid lines 
    when drawing an arrowhead (dotted arrowheads look terrible).
    Since version 5 introduced dash patterns to all terminals, all terminals
    now suffer from this.  Any fix to replace the one in post.trm will have
    to catch all terminals.   But maybe it's not worth it.

I attach a revised version of your patch 0005 that addresses 1 + 2.

It adds a parameter to draw_clip_arrow that passes the fixed/variable
arrowhead size state.  I changed most the call sites to match.
This change is in patch 0006

0005-EAM is to be applied instead of your previous 0005
0006-EAM is to be applied on top of it
(these are not git patch-format.  I'm still getting up to speed with that).


I consider this an imperfect solution to be used for debugging.
A cleaner solution would be to pass a pointer
to the structure containing the arrowhead style.  E.g. instead of
      draw_clip_arrow(x1, y1, x2, y2, ap.head, ap.head_fixedsize);
it would be
      draw_clip_arrow(x1, y1, x2, y2, &ap);

I didn't change the call sites in util3d.c (draw3d_line_unconditional)
you'll have to dummy those up for testing.  I think that routine will
need additional changes eventually.

> Clearly patches 0004 and 0005 WILL produce rendering differences.
> To be clear, these patches still don't try to clean up the
> round/truncate differences, but at least make it obvious where the
> questionable spots are regarding this patch series: in all the things
> that patch 0004 touches.

I understand.  I will disregard the 1-pixel change side effects for now.

[snip fixed problem with post.trm]
> I don't see this. Could it be because I applied the postscript-terminal
> patch you sent out?
Yes.

> I can think of one piece of the new code that could be producing an
> overflow, though. In draw_clip_arrow() I have this:
> 
>     // Direction vector in (dex,dey). I need to convert this to integers
>     // with a scale that's large-enough to give me good angular resolution,
>     // but small-enough to not overflow the data type. Let's aim for the
>     // vectors to be on the order of 1e6 ~ 2^20
>     dex -= dsx;
>     dey -= dsy;
> 
>     double delta_largest = fmax( fabs(dex), fabs(dey) );
>     double scale_want    = (double)(1U << 20);
>     double scale = scale_want / delta_largest;
> 
>     dex *= scale_want;
>     dey *= scale_want;
> 
> Maybe 2^20 is too large for some terminals?

Yes.  See revised code in my version of 0005 that uses a
terminal-specific scale.

I have no clue what is going on with bug (3).  Maybe you can spot the error.
Otherwise I think this is converging rapidly on a nice improvement.

As noted above I am suspicious that the 3D code may trip other problems
but I have not looked at it very hard.

	Ethan