Re: Short vectors are not plotted

[Dima K. <gn...@di...>]

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.

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.



> That let me look for problem areas, and I found some. There are many
> cases where the coordinates passed to the terminal driver have
> overflowed. For example the "Let's smile with parametric filled
> curves" plot in the filledcurves demo ends like this:
>
>     gsave [] 0 setdash
>     3980 1811 M
>     0 -221 V
>     -218 37 V
>     -653259491 771754262 M
>     3980 1590 L
>     stroke
>     grestore
>
> The coordinates in that last move (M) command are nonsense.

I don't see this. Could it be because I applied the postscript-terminal
patch you sent out?

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?