Add new join methods for when polyline angles are too sharp.#168
Add new join methods for when polyline angles are too sharp.#168Kevinpgalligan wants to merge 3 commits intovydd:masterfrom
Conversation
|
Testing. Miter join is used on the right, bevel is used to the left of that, and the "simple" join is used (at least) on the far left. Here's what it was like before. Note that for small enough angles, the line width isn't consistent and the edge is so pointy that it goes off the screen. |
|
This does indeed work and make |
|
The ideal solution would be to make everything configurable, since it's possible that users will have different preferences. But yes, it sounds like the threshold I picked may not be ideal! Happy to change it when I get time to work on Sketch again. |
I looked at how other vector graphics (e.g. nanovg) does it, and I now think the 20deg threshold is reasonable enough (whose purpose is solely to avoid graphic glitch for miter join). Instead, there should be another |
|
That makes sense to me! I'd love to implement a |
|
Finally getting around to this! It does look bad when animated... line-join.mp4First I'll get the default behaviour to look less crap, then extend the pen API to allow specifying the join. (Edit: the flickering is only noticeable when the stroke weight is chonky). |
|
Some useful code for debugging, press space to nudge the line: (defsketch polyline-test ((deg 3))
(translate (floor width 2) (floor height 2))
(let ((coords (list -100 0 0 0 (* 150 (cos deg)) (* 150 (sin deg)))))
(with-pen (:stroke +white+ :weight 10)
(apply #'polyline coords))
(let ((lines (edges (group coords) nil)))
(multiple-value-bind (lefts rights)
(join-lines lines 5 -5)
(format t "=========~%")
(format t "LEFTS: ~a~%" lefts)
(format t "RIGHTS: ~a~%" rights)
(let ((lines (sketch::edges (sketch::group coords) nil)))
(format t "ANGLE: ~a~%" (sketch::interior-angle-between-lines
(first lines) (second lines))))
(with-pen (:stroke +red+)
(loop for (x y) in lefts
for i from 0
do (circle x y 5)
do (with-font (make-font :color +red+)
(text (format nil "~a" i) (- x 5) (- y 5)))))
(with-pen (:stroke +blue+)
(loop for (x y) in rights
for i from 0
do (circle x y 5)
do (with-font (make-font :color +blue+)
(text (format nil "~a" i) (- x 5) (- y 5))))))))
(incf deg 0.001)
(stop-loop))
(defmethod on-key ((inst polyline-test) key state)
(when (and (eq key :space) (eq state :up))
(start-loop))) |
I think this is ok if we introduce line-join option. If user use The artifact protruding to the left of the screen in your video (opposite side of the join) does worry me though... |
|
Yes, gotta fix the artifact! I've updated the debug sketch above so that it shows the angle between the lines. It seems that both bevel join and the "simple" join are buggy. My suspicion is that one of the problems is that, when a join has an angle of <90 degrees, the right and left sides switch. So the left side of the first line should be joined up with the right side of the second line. Currently, I think we're joining the left side with the left side. |
|
I think I've identified two separate bugs, one in bevel join and one in the "simple" join. It's a bit hard to explain, and I'm not sure what the solutions are just yet.
Anyway, these bugs shouldn't prevent the user being able to override the join type, so maybe I'll add that relatively simple feature first. |
|
Done:
Testing appreciated! Now just to fix the buggy line joins... |
|
Example of the tricky bevel case, where the thickened line segments are overlapping. I confirmed that the HTML5 canvas API does handle this case gracefully. It LOOKS like we could just draw the two line segments separately and the result would be indistinguishable. <!DOCTYPE html>
<html lang="en" xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta charset="utf-8" />
<title>LineJoin Example</title>
<script type="text/javascript">
function draw() {
var canvas = document.getElementById("MyCanvas");
if (canvas.getContext) {
// Draw some lines with different line joins.
var ctx = canvas.getContext("2d");
var lStart = 50;
var lEnd = 200;
var yStart = 50;
ctx.beginPath();
ctx.lineWidth = "50";
// Use a bevel corner.
ctx.lineJoin = "bevel";
ctx.moveTo(lStart, yStart);
ctx.lineTo(lEnd, yStart);
ctx.lineTo(lStart-40, yStart + 5);
ctx.stroke();
// Use a round corner.
ctx.beginPath();
ctx.lineJoin = "round";
ctx.moveTo(lStart + 200, yStart);
ctx.lineTo(lEnd + 200, yStart);
ctx.lineTo(lEnd + 200, yStart + 200);
ctx.stroke();
// Use a miter.
ctx.beginPath();
ctx.lineJoin = "miter";
ctx.moveTo(lStart + 400, yStart);
ctx.lineTo(lEnd + 400, yStart);
ctx.lineTo(lEnd + 400, yStart + 200);
ctx.stroke();
// Annotate each corner.
addText("bevel", lStart + 50, yStart + 50, "blue");
addText("round", lStart + 250, yStart + 50, "blue");
addText("miter", lStart + 450, yStart + 50, "blue");
}
function addText(text, x, y, color) {
ctx.save(); // Save state of lines and joins
ctx.font = "400 16px/2 Unknown Font, sans-serif";
ctx.fillStyle = color;
ctx.fillText(text, x, y);
ctx.restore(); // restore state of lines and joins
}
}
</script>
</head>
<body onload="draw();">
<canvas id="MyCanvas" width="800" height="300"></canvas>
</body>
</html> Also tracked down what might be the place where line joins are handled in the Cairo graphics framework, although I don't have a clue what the code is doing: |
|
FINALLY got the bugs ironed out, as far as I can tell. Commit here: Kevinpgalligan@734bba6 Here's a sample sketch showing that bevel join appears to be free of artifacts/flicker. The polyline.mp4This was a major pain in the ass and I'm not feeling motivated to work on it any further right now, but possible future additions would be:
Also, regarding this pull request, it will need to be updated with the commits I've linked from my |
There was a problem hiding this comment.
AI Review Summary
Status: Incomplete - needs update
The approach is excellent - implementing proper line join methods (miter, bevel, simple) is the right solution for the polyline artifacts issue. However, per your own comments, this PR needs to be updated with the additional commits from your dev branch:
- Line join property for pen (
53fd07a) - README update (
7c6c9eb) - Refactoring (
7c97af8) - Critical: Bug fixes for bevel/simple joins (
734bba6) - None join type (
0e0032f)
Current code observations:
-
geometry.lisp additions ✅
angle-between-lineswith proper acos clamping to [-1,1]- Vector utilities are clean
-
shapes.lisp rewrite
⚠️ - Good structure with
join-lines,miter-join,bevel-join,simple-join - But per your comments, bevel and simple joins have bugs that cause artifacts during animation - fixed in
734bba6which isn't in this PR
- Good structure with
-
Thresholds
*bevel-join-min-angle*(2°) and*miter-join-min-angle*(20°) are reasonable defaults- Good that these are configurable
Note on conflict
This PR removes translated-intersects entirely, which is fine - it replaces it with better code. There's also a simpler fix in intersect-lines that handles nearly-parallel lines, but your comprehensive join method approach is the more robust long-term solution.
Verdict: Please update the PR with your dev branch commits, then it should be ready to merge.



Attempting to fix the polyline bug, see: #53
Visual artifacts were appearing for polylines with sharp corners. This is a known problem when using the miter method of joining 2 lines. The join points are calculated by extending the left and right sides of the lines until they intersect. If there's a small angle between the lines, however, the intersection points go to infinity and beyond (they're almost parallel), creating visual artifacts.
The solution implemented here is to switch to a different join method when there's a small angle between the lines. I arbitrarily chose a cutoff of 20 degrees to switch from meter join to bevel join. I find that the transition from miter to bevel is too jarring if the cutoff angle is smaller than that, but I'm open to changing it. I also added a cutoff of 2 degrees to switch from the bevel method to a "simple" join, because bevel also gets buggy when the lines are parallel-ish.
In the future, we could make the cutoff angles configurable. It would also be possible to allow the user the choose the join they want, and add more joins like
rounded.Will share some of the experimenting I've done to confirm the fix.