#2401 Incorrect rounding in dumb.trm

[Benson M]

Originally tested with gnuplot 5.2.8 from gentoo, but I grabbed a snapshot and confirmed the bug remains. My patch, attached as '1_bresenham.patch', is based on that snapshot.

The line-drawing algorithm in DUMB_vector() has incorrect rounding in some cases. Two places (lines 571 and 601) it casts ints to double for division, then adds 0.5 and casts back for rounding to the nearest int. But since the quantity being converted is a delta, it can be either positive, which is fine, or negative, in which case this is off by one.

This could be fixed by tracking the sign of the result and adding -0.5 if negative, by calling floor() instead of implicitly truncating, or by adding dumb_x/dumb_y first, so that the quantity being converted is always positive.
Instead, I've just changed it to Bresenham's algorithm with all integer math and avoided the floating-point conversion entirely, as this works out well for some other changes I'm playing with.

I've included files that demonstrate the problem: 'slopetest.gnuplot' is the input script, 'slopetest.out' the result from unpatched gnuplot, and 'slopetest.correct' the result with this patch applied. You can see in 'slopetest.out' the shallow slopes on the left are all biased upward from a straight line between the points, as are the NW-SE slopes along the bottom. Likewise the steep slopes in the center-right are all biased to the right.
In 'slopetest.correct' you can see the lines are where they should be.

Also included, the same script with the data order reversed, and its respective outputs as 'tsetepols.{gnuplot,out,correct}'. As expected the bug manifests on different vectors, mainly the SW-NE slopes across the bottom, and is still gone in the patched version.

By the way, I have several more patches to fix/improve DUMB_vector()'s rendering. This first one is the only one that's definitely a bug, rather than a missing feature, and I wasn't sure whether the others should go to Patches or Feature Requests. For that matter, I'm not sure if this one should've gone to Patches instead? Please advise.

[Ethan Merritt]

Are you OK with the simpler patch below?

diff --git a/term/dumb.trm b/term/dumb.trm
index 7141934a9..a6b6b2c8a 100644
--- a/term/dumb.trm
+++ b/term/dumb.trm
@@ -568,8 +568,8 @@ DUMB_vector(unsigned int arg_x, unsigned int arg_y)
        }
        dumb_set_pixel(dumb_x, dumb_y, pen1);
        for (delta = 1; delta < ABS(y - dumb_y); delta++) {
-       dumb_set_pixel(dumb_x  + (int) ((double) (x - dumb_x) *
-                                       delta / ABS(y - dumb_y) + 0.5),
+     dumb_set_pixel(dumb_x  + round((double) (x - dumb_x) *
+                                     delta / ABS(y - dumb_y)),
                           dumb_y + delta * sign(y - dumb_y), pen);
        }
        dumb_set_pixel(x, y, pen1);
@@ -597,8 +597,7 @@ DUMB_vector(unsigned int arg_x, unsigned int arg_y)
        dumb_set_pixel(dumb_x, dumb_y, pen1);
        for (delta = 1; delta < ABS(x - dumb_x); delta++)
            dumb_set_pixel(dumb_x + delta * sign(x - dumb_x),
-                      dumb_y +
-                      (int) ((double) (y - dumb_y) * delta / ABS(x - dumb_x) + 0.5),
+                    dumb_y + round((double) (y - dumb_y) * delta / ABS(x - dumb_x)),
                           pen);
        dumb_set_pixel(x, y, pen1);
     } else {

[Ethan Merritt]

By the way, I have several more patches to fix/improve DUMB_vector()'s rendering. This first one is the only one that's definitely a bug, rather than a missing feature, and I wasn't sure whether the others should go to Patches or Feature Requests. For that matter, I'm not sure if this one should've gone to Patches instead? Please advise.

Sending to Patches is fine.
Thanks

[Benson M]

Yes, that should certainly fix the bug, although I might have cast the double returned from round() to int on hygienic principles.

I got the first patchset posted [patches:#792], so you can see where I'm going with this and why I ditched floating point altogether.
Of course these rendering improvements can be implemented with doubles and fmod(), and indeed that's how I initially did it, but that way turns out fairly ugly with fuzz factors, so I scrapped it and started over with Bresenham's algorithm. The integer error makes thresholds within the character cell simpler to reason about.
But obviously the change to Bresenham's can trivially be rolled into the feature improvement patchset instead, if you want to push the simple patch and close this bug now, then discuss those other changes.