Tk Source Code

If the chord length is zero then the code does not handle the resulting infinities although it does not crash either.

Merged to trunk.

Thank you for your patch. I included most of it in a new commit [5d9a9cf7].

The useless bbox settings in ComputeArcParametersFromHeight() could indeed be removed from my original patch, however I definitely think that arcPtr->start = arcPtr->extent = arcPtr->height = 0; must be kept in the zero-length chord shortcut in ComputeArcParametersFromHeight(). At least arcPtr->height = 0 must necessarily be set to zero so that configure -height correctly returns zero as expected even for very small arcs. Setting the extent to zero is more a convention, and the start already is zero since it's the default but it's safer to set it here as well.

With this improved patch, the new test canvas-21.1 that I had previously added to check the computed parameters of such zero-length arcs still passes.

I will close this ticket soon, unless someone objects. Thanks for your help Simon.

Thanks. I've added my fix and the updated test file.

Thanks, I've now added the file.

There is an "Attach" button a bit at the left of the "Edit" button you used to add a comment.

Hi. I'd be happy to send my fix but there seems to be no way of attaching the file here so I'm including a unified diff below:


[simon@localhost tip415]$ diff -u ./tkCanvArc.c-safe ./tcl/tk-a613a84f/generic/tkCanvArc.c  
--- ./tkCanvArc.c-safe  2019-04-06 20:45:29.534997330 +0100
+++ ./tcl/tk-a613a84f/generic/tkCanvArc.c       2019-04-06 21:07:00.935916663 +0100
@@ -185,7 +185,7 @@
 static int             ConfigureArc(Tcl_Interp *interp,
                            Tk_Canvas canvas, Tk_Item *itemPtr, int objc,
                            Tcl_Obj *const objv[], int flags);
-static int             ComputeArcFromHeight(ArcItem *arcPtr);
+static void            ComputeArcParametersFromHeight(ArcItem *arcPtr);
 static int             CreateArc(Tcl_Interp *interp,
                            Tk_Canvas canvas, struct Tk_Item *itemPtr,
                            int objc, Tcl_Obj *const objv[]);
@@ -471,20 +471,16 @@
     }
 
     /*
-     * If either the height is provided then the start and extent will be
-     * overridden.
+     * Override the start and extent if the height is given.
      */
-    if (arcPtr->height != 0) {
-       int ret = ComputeArcFromHeight(arcPtr);
-    if (ret != TCL_OK) {
-               Tcl_SetObjResult(interp, Tcl_ObjPrintf(
-                       "coordinates too close to define a chord"));
-               Tcl_SetErrorCode(interp, "TK", "CANVAS", "COORDS", "ARC",
-                       NULL);
-        return ret;
-    }
+
+       ComputeArcParametersFromHeight(arcPtr);
+    
+    /*
+     * Calculate the arc's bounding box
+     */
+    
        ComputeArcBbox(canvas, arcPtr);
-    }
 
     i = (int) (arcPtr->start/360.0);
     arcPtr->start -= i*360.0;
@@ -598,13 +594,11 @@
 /*
  *--------------------------------------------------------------
  *
- * ComputeArcFromHeight --
+ * ComputeArcParametersFromHeight --
  *
  *     This function calculates the arc parameters given start-point,
  *     end-point and height (!= 0).
  *
- * Results:
- *     TCL_ERROR if the chord length is zero, TCL_OK otherwise.
  *
  * Side effects:
  *     The height parameter is set to 0 on exit.
@@ -612,21 +606,28 @@
  *--------------------------------------------------------------
  */
 
-static int
-ComputeArcFromHeight(
+static void
+ComputeArcParametersFromHeight(
     ArcItem* arcPtr)
 {
     double chordLen, chordDir[2], chordCen[2], arcCen[2], d, radToDeg, radius;
 
+    /* 
+     * Do nothing if no height has been specified.
+     */
+    
+    if (arcPtr->height == 0)
+        return;
+    
     /*
-     * The chord.
+     * Calculate the chord, do nothing if it is too small.
      */
-
+    
     chordLen = hypot(arcPtr->endPoint[1] - arcPtr->startPoint[1],
            arcPtr->startPoint[0] - arcPtr->endPoint[0]);
 
     if (chordLen < DBL_EPSILON)
-        return TCL_ERROR;
+        return;
 
     chordDir[0] = (arcPtr->endPoint[0] - arcPtr->startPoint[0]) / chordLen;
     chordDir[1] = (arcPtr->endPoint[1] - arcPtr->startPoint[1]) / chordLen;
@@ -652,7 +653,7 @@
      * The arc start and span. Angles are negated because the coordinate
      * system is left-handed.
      */
-
+    
     radToDeg = 45 / atan(1);
     arcPtr->start = atan2(arcCen[1] - arcPtr->startPoint[1],
            arcPtr->startPoint[0] - arcCen[0]) * radToDeg;
@@ -682,7 +683,7 @@
 
     arcPtr->height = 0;
     
-    return TCL_OK;
+    return;
 }
 

 /*

Will do, but it's late here (UK) now so it'll have to be tomorrow.

Is your fix different from my proposal? Is it perhaps better? I'm interested in you're sharing of it.

Regarding comments emails this is supposed to work in fossil. No idea why you didn't get them.

I wasn't ignoring this just waiting for more people to contribute. What I hadn't realised was that there is no automated email for comments so I wasn't aware others had expressed a preference. Still no need for me to send in my fix now someone else has done it.

I have now committed a proposed change that removes the error but still prevents any NaNs of Infs to sneak in the calculations.

I have also added a new test canvas-21.1 that exercises the case.

See [23d0a7f5] for details.

I would prefer that no error results. Otherwise, as already suggested, for applications which allow the end user to scale the drawing the programmer would just end up having to put catches around the arc creation code, which is messy and could hide other real programming errors.

My preference also would be not to result in an error. So, fvogel, count me in at your side of the debate ;-)

> My thinking is simply that the error would show the user that they had an bug in their code.

Not necessarily (I understand the "user" you mention actually is the programmer). Think interaction by the (end) user:
Small arcs could well show up in, say, a drawing application in which the (end) user is able to dynamically zoom in/out the canvas, or resize its (arc) elements to an arbitrary size, or input arc coords manually, or read them from a data (measurements for instance) file.
The error would then pop out at runtime depending on what the user did, or what the data file contains, all things that are out of the programmer control.
The programmer would need to take this possibility into account and catch or otherwise protect its code from that irrelevant (in this case) error.

> consensus

We should have more people dropping their opinion. So far we are only two, we can't say there is a consensus :-)

My thinking is simply that the error would show the user that they had an bug in their code. We've already detected the condition so why not pass it back to the user?

Whilst I agree that arcs with an enclosed area of < 1 pixel could be flagged to the user unless this is already done for other enclosed area items I think it is not appropriate for this tip.

If the consensus if to silently ignore them I will change the patch and test to do just that.

> potentially useful information

What would be a use case for sending this error to the Tk programmer? When is it important to know that one of the arcs in a canvas is so small that its chord length is smaller than a very small value that directly depends on how doubles are represented on the host machine?

If we're going to detect small arcs, then it would be more useful to detect cases where the arc would display in an area of less than a single pixel for instance, so that the programmer could then skip this arc in order to improve performance.

In contrast, I see more value in silently ignoring the error than reporting it:
- no need to catch the arc creation code
- code is more generic, no need to specialize for very small arcs
- arcs smaller than a single pixel anyway have no display use I can see

What am I missing here?

My preference is for the error since it provides potentially useful information to the user and since we have it we should pass it back.
On the other hand if the Tk idiom is to ignore the error and do nothing silently I'll go with that - just seems a shame to lose this information.

Simon Geard (privately) sent me a patch throwing an error when the start and stop points of the arc are such that the chord length is smaller than DBL_EPSILON.

I have committed this patch in a bugfix branch for easy review, and I'm attaching the files submitted by Simon for any future reference. Note: this patch was submitted to me before the big change None --> 0 controversially discussed in [9e31fd9449], so with respect to the current state of tkCanvArc.c I ignored the irrelevant diffs in my commit [48214056].

My first reaction regarding this proposal is that the error is perhaps not so welcome. I think it would be better to simply ignore the small arc (do nothing gracefully, in other words).

Example of a test script triggering infinite values in the code:

    package require Tk
    canvas .c -width 700 -height 700 -bg grey
    pack .c -fill both -expand 1 -side left
    .c create arc 0 100 0 100 -height 100 -outline blue -style arc