#494 Replacement for ELLIPSEARC2 wrong behavior + documentation improvements

[entuland]

The current ELLIPSEARC2 implementation leaves the turtle in the wrong position if the given angle for the spanning of the arc is not a multiple of 90 degrees.

I've also noticed that the documentation reports misleading information about the actual behavior of both ELLIPSEARC2 and ELLIPSE2 (which in turn relies on ELLIPSEARC2).

For that reason I have implemented a working version of ELLIPSEARC2 that fixes the turtle left in the wrong position and allows for fully using ELLIPSEARC2 to build ellipses out of consecutive arcs - it will also honor the start angle correctly.

Attached to this message is the test program that shows the wrong behavior of ELLIPSEARC2 and showcases the new implementation (named A.ELLIPSEARC2 in my test program)

I will add further comments here below to attach the patch for replacing the ELLIPSEARC2 file in the Logolib folder as well as the patches to improve the documentation relative to ELLIPSEARC2 and ELLIPSE2.

[please disregard both "ellipsearc2.proposal.lgo" attached here and "ellipsearc2.diff" attached to the first comment below, please refer to the fixed versions marked with V2 further down in the comments]

[entuland]

Patch to replace ELLIPSEARC2 file in the Logolib folder

[entuland]

Patch to improve the documentation relative to ELLIPSE2 - the way it is worded right now has the axis orientation wrong.

[entuland]

Patch to improve the documentation relative to ELLIPSEARC2.

The way it is worded now suggests leaving the start angle set to zero, which is not needed.

The proposed improvement provides an example of how to build an ellipse out of multiple arcs and explains in detail how FMSLogo performs the the drawing, including the calculation of the arc's angles (which could be a bit unintuitive, as the angles appear as being computed on a circle and then stretched to fit the ellipse)

[entuland]

ooops... I got the slope calculation wrong for one of the orientations of the ellipse... attached to this post is the fixed testcase, follows patch for the Library file with the fixed bug

[entuland]

Here is the hopefully correct version of the proposed ELLIPSEARC2 implementation.

I suspect the code could be simplified, I'm not sure I really need to separately target the different orientations of the ellipses, the code seems to perform fine though.

I would be more than happy if anybody more knowledgeable than me about these geometric issues could improve it further.

[David Costanzo]

cf. [#94]

I haven't had time to read up on ELLIPSEARC2, but I wanted to state that backward compatibility with old programs that expect the broken behavior should be considered. I generally don't break compatibility with old programs unless the need is strong and the likelihood of impact is small.

Also, UCBLogo has a different ELLIPSEARC2, so it's worth checking if updating FMSLogo's version to the latest in UCBLogo is what's needed.

If research shows that UCBLogo 6.0 has a perfect ELLIPSEARC2, then it might be that the right fix is to add an ELLIPSEARC3 (or something similar) in FMSLogo 7.X and possible change ELLIPSEARC2 in FMSLogo 8.X. Any deprecation process should be run by the larger community.

That said, if the Italian translation of ELLIPSEARC2 has never been released, then there's no backward compatiblity to worry about. It can fixed without changing ELLIPSEARC2.

[entuland]

I've checked the UCBLogo manual from this link: https://sourceforge.net/p/ucblogo/code/HEAD/tree/docs/usermanual.pdf
as far as I can see there is not even a direct command named after circles (one should rely on the ARC command) and the same stands for ellipses (it could be eventually achieved with the SETSCRUNCH command combined with the ARC command, which by the way would achieve same the angle behavior I've explained in my patches above and which is what the C++ NODE lellipsearc(NODE arg) command does, if I'm not mistaken).

There seems to be no trace of CIRCLE, ELLIPSE, ELLIPSEARC and so forth in UCBLogo 6.0 at all

For some reason I thought that ELLIPSEARC2 was so broken that nobody could have ever relied on it, but you're right, it should stay as it is for the whole FMSLogo 7.x version at least. Both adding it as ELLIPSEARC3 (or with a different name) in the meantime or postpone it to version 8.x is fine would be fine with me. By the way, I think I could improve my version of ELLIPSEARC2 further, so if you plan to add it in any way please let me know and I'll provide the refined version of ELLIPSEARC2.

Of all the contributions I've posted in this ticket, though, one still applies conceptually: it's the comment about the ELLIPSE2 documentation

Sorry for the mess of these combined patches into a single ticket, should I open another ticket for addressing only the ELLIPSE2 documentation?

The same question applies for eventually updating the ELLIPSEARC2 documentation: if the command is going to stay as it is for version 7.x, at least the documentation could point out this misbehavior: should I open another ticket for it?

[David Costanzo]

I have only gotten up to the point of ELLIPSE2 documentation change [r4407]. I think the root cause of the problem is that the inputs were labelled incorrectly in the manual. The inputs for ELLIPSE and ELLIPSE2 are inexplicibly given in the opposite order. To me, "crosswise" means perpendicular to the direction of the turtle and this is what the term means in ELLIPSE and elsewhere. Changing its meaning in ELLIPSE2 would be confusing. It's clearer to swap the order, explicitly call out that the inputs are swapped, and keep the naming consistent.

In MSWLogo, both ELLIPSE and ELLISPE2 were documented on the same page and the inputs were labeled "Minor" and "Major". I probably changed this to "crosswise.semiaxis" and "inline.semiaxis" because "Minor" and "Major" aren't descriptive (especially since the caller can give a larger number for MINOR). I didn't notice at the time is that ELLIPSE and ELLIPSE2 take their arguments in a different order.

I also don't have any regression tests for ELLIPSE or ELLIPSE2. I suspect what happened is that I couldn't understand what it was supposed to do and opened bug [#94] as a reminder to come back to it someday.

As for the UCBLogo thing, I misread what I had written in [#94]. UCBLogo has an ARC, not an ELLIPSEARC. I suspect that they're really the same thing, but I don't know the history and haven't tried to compare the implementations or trace the implementation history back to the point where MSWLogo last forked from UCBLogo.

[entuland]

Indeed "crosswise" and "inline" are far better than "minor" and "major" in this context.

I found the log "Fixed bug in ELLIPSA2 (start angle did not work correct)" (whatever that means) in the attached changelog for MSWLogo 6.0h, which I found somewhere on the internet, but that doesn't help much about tracking where and how things went wrong with that procedure. It only confirms that the misbehaving ELLIPSA2 procedure is out since a long time.

If ELLIPSEARC2 isn't going to get fixed anytime soon, the documentation could point out the behavior: as far as I can tell, the actual behavior is that everything works fine as long as the given ANGLE and the given STARTANGLE are multiples of 90 degrees; anything else messes up the drawing by misplacing the arc's start related to the turtle start position or misplacing the turtle end position related to the arc's end.

On a side note, as for UCBLogo ARC and ARC2, they are only related to circles. The only way I can find to draw ellipses with them (which almost works in FMSLogo too) is to use the SCRUNCH, but I guess this is an unrelated bug, cause the markers for turtle position are messed up here too and the resulting ellipse isn't tangent to the turtle:

; attempts to draw an ellipse out of multiple arcs
clearscreen
pendown
setscrunch 1 2
setpensize 3
right 45
repeat 36 [
    ; mark the start position
    setpencolor "red
    circle 1

    ; draw a section of the ellipse
    setpencolor "green
    arc2 10 50
]

(the actual orientation of the ellipse isn't rotated by 45 degrees but that's normal because of the way SCRUNCH works, that's not an issue)

[David Costanzo]

I have integrated your ELLIPSEARC2 documentation improvements as [r4410]. Then I made some modifications for consistency with the overall manual in [r4411].

I liked the example you added and it was the first time I ever saw a use for a non-zero startangle. I also think that your idea of separating how the ellipse arc's shape is defined from where it is drawn improved the clarity, so I integrated your "important" subsection into the main description, instead of keeping it as an addendum. After this was done, I realized that labelling the axes as "inline" and "crosswise" made things more obscure, so I renamed them as part of how the ellipse arc was defined.

As for fixing ELLISPEARC2, I'd like to do this only once and get it exactly right, as every time a new procedure is released, it creates a backward compatibility headache. I don't think that getting it exactly right can be can be done as a library routine that moves the turtle, then calls ELLIPSEARC, as this doesn't work quite right with FENCE. For example, my expectation is that

CLEARSCREEN FENCE CIRCLE2 600

should draw the portion of the circle up to the top of the turtle's world, throw an error, and leave the turtle there.

[David Costanzo]

I have started work on FMSLogo 8.0.0, which means I will incorporate breaking changes that have a low compatibility risk to existing programs. While I still think that rewriting ELLIPSEARC2 to not use ELLISPEARC (likely as a primitive) is necessary to make it compatible with FENCE, SQUASH, and SLOWDRAW, that's more than I want to do right now. I have therefore decided to accept your patch to ELLIPSEARC2. However, since I changed the meaning of the angles in ELLISEARC, I broke your ELLIPSEARC2 (the turtle would either glide ahead or behind the line). I have updated it to be compatible with the changes in FMSLogo 8.0.0.

After my updates, the code looks correct at a glace, but it's hard to follow closely. This is because some of the signs are (intentionally) wrong, but I account for this by turning LEFT instead of RIGHT, or vice-versa.

After finishing this, I realized that I was wrong about the X and Y inputs being swapped (at least the way its documented). I will correct the documentation and may update the input names in ELLISPEARC2 to match the updated documentation.

The new ELLISPEARC2 passes all my tests, so I'm confident that it's correct. However, if you want to test it out yourself, I'll compile a pre-release build of 8.0.0 for you.

For reference, I integrated your patch as [r5397] and updated it to work with FMSLogo 8.0.0 in [r5398]. The fix will be first available in FMSLogo 8.0.0.

By the way, your patch fixed a bug I didn't even know about. If you give ELLIPSEARC2 a negative 3rd input in FMSLogo 7.X and below, the ellipsearc is drawn far away from where the turtle was. It's worth a try if you just want a laugh:

ELLIPSEARC2 90 100 -200 0