Potential oversight in toolhead.py:calc_junction()

[bmc0]

R is calculated using only the angle between the previous and current segment. Consider a circle approximated by x segments. As x increases, R increases as well and is not bounded (if you ignore the fact that junction_cos_theta cannot be less than -0.999999). As a result, it is possible for R to exceed the actual radius of the circle if x is large enough.

One way to limit R would be to add this after line 62 in toolhead.py:

cos_theta_d2 = math.sqrt(0.5*(1.0+junction_cos_theta))
R_lim = min(self.move_d, prev_move.move_d) * sin_theta_d2 / cos_theta_d2
R = min(R, R_lim)

I've done some testing and have found lots of cases in real-world G-code where the above code reduces the value of self.max_start_v2.

[KevinOConnor]

Interesting. Can you expand on why you choose that formula for R_lim?

Thanks, -Kevin

[bmc0]

Using the diagram in this post as a reference, there is a right angle formed by the intersection of V_entry and R. If you allow R to intersect V_entry at its terminal point (opposite from θ), tan(θ/2) = R / ||V_entry||. A little rearranging gives you R = ||V_entry|| * sin(θ/2) / cos(θ/2).

Technically, this solution is not quite correct (R is still a little bigger than the radius of the circle in the scenario I gave in my previous post), but it seemed a fairly reasonable approximation and is pretty cheap computationally, so I haven't tried to work out a better solution.

[KevinOConnor]

Ah, makes sense. And I think you're right about changing it. I do think this is something to do post v0.6.0 release though - there could be side effects from the change (in particular it will place a speed limit on just about any tiny move). So, I think additional testing before putting it in a release makes sense.

BTW, unless I'm missing something, I think it would be: R_lim = 0.5 min(self.move_d, prev_move.move_d) sin_theta_d2 / cos_theta_d2 (For example, if there's a 20 sided regular polygon, then I think you'd need to halve the length of each side to find the radius of the circle confined by that polygon.)

[bmc0]

Good idea. It certainly has the potential to cause problems in certain cases (though I suppose you could claim that it's the slicer's fault for producing moves that are too small).

R_lim = 0.5 * min(self.move_d, prev_move.move_d) * sin_theta_d2 / cos_theta_d2

You're right, that's a much better approximation than what I originally posted. Not sure how I missed that. It would be slightly better to have the virtual path pass through the actual points instead of being inscribed in the polygon, but the math is not as simple and the error is very small most cases.

I tested the above code with regular 32-gons of "radius" 5, 20, and 100 and got an average R_lim value of 4.99, 19.96, and 99.82, respectively—plenty close enough, I think.

[bmc0]

I realized that in order to get the correct radius for a regular polygon (so the virtual path passes through the points), you just need to calculate δ for R_lim, then add it to R_lim. So the full R_lim calculation would be:

R_lim = 0.5 * min(self.move_d, prev_move.move_d) * sin_theta_d2 / cos_theta_d2
R_lim += R_lim * (1.0 - sin_theta_d2) / sin_theta_d2

[mabl]

Okay, so just for me to get this:

The algorithm proposed by Jeon Sonny has a corner case, where the virtual path circle intersects with the actual path outside of the path segment. This can happen, if for example a circle is approximated by very many small segments. Each therefore having a shallow angle between the segments.

The proposed solution is to calculate the radius of a circle passing trough the three points of a pair of segment lines, which is exactly R_lim as proposed by the last comment of @bmc0.

Whichever radius is smaller wins.

though I suppose you could claim that it's the slicer's fault for producing moves that are too small

I tend to disagree. The slicer directly operates on the mesh, and small lines originate from the mesh and the precision derived from that.

As a side node, I recently printed a high resolution mesh and had the feeling that the system went far to fast through the corners. This might very well be the culprit

[dragonnn]

@mabl some slicers tended to simplify the mesh if it is unnecessary high resolution. As far I know S3D and slic3r does this, in Cura this is an experimental option.

[KevinOConnor]

On Sun, Mar 25, 2018 at 12:04:58AM -0700, Michael Barbour wrote:

I realized that in order to get the correct radius for a regular polygon (so the virtual path passes through the points), you just need to calculate δ for R_lim, then add it to R_lim. So the full R_lim calculation would be:

R_lim = 0.5 * min(self.move_d, prev_move.move_d) * sin_theta_d2 / cos_theta_d2
R_lim += R_lim * (1.0 - sin_theta_d2) / sin_theta_d2

Interesting. Unless I'm missing something though, that simplifies to:

R_lim = 0.5 * min(self.move_d, prev_move.move_d) / cos_theta_d2

-Kevin

[bmc0]

@KevinOConnor: You're correct. I hadn't tried to simplify it, but I decided to do the math after seeing your comment and got the same answer. Then I drew a diagram with a regular polygon inscribed in a circle and... felt a bit silly for not realizing earlier that the answer was that simple.

@mabl: Your understanding of the problem is accurate.

The proposed solution is to calculate the radius of a circle passing trough the three points of a pair of segment lines

Not quite. It only passes through all three points if the entry and exit segment lengths are the same. I don't think that finding a circle that passes through all three points in all cases is what we want because the calculated radius would be huge if one of the segments is long.

The slicer directly operates on the mesh, and small lines originate from the mesh and the precision derived from that.

Cura and Slic3r both use a line simplification algorithm to avoid unreasonably small segments.

[KevinOConnor]

Okay, thanks. I think we can also handle the (very rare) case where the two moves have different acceleration. I uploaded this on a new work-centripetal-20180325 branch for those interested in testing it (commit 8e560d2e). Again, I think this is for a post v0.6.0 release.

-Kevin

[bmc0]

Looks good, except you forgot to halve the move distances. I hadn't considered the case where the two moves have different accelerations.

Also, after thinking about this a bit more, I've concluded that it actually makes more sense to use sin(θ/2) / cos(θ/2) from the original formula instead of 1 / cos(θ/2). The maximum allowable junction velocity should go down as θ decreases because the corners in the path that the machine actually follows are sharper. With 1 / cos(θ/2), it stays the same. Thoughts?

[KevinOConnor]

On Sun, Mar 25, 2018 at 04:47:15PM -0700, Michael Barbour wrote:

Looks good, except you forgot to halve the move distances. I hadn't considered the case where the two moves have different accelerations.

Oops! I've corrected the work-centripetal-20180325 test branch (now commit 28f2deb2). It's not worth worrying about two different accelerations - it just seemed like we could handle it without an issue.

Also, after thinking about this a bit more, I've concluded that it actually makes more sense to use sin(θ/2) / cos(θ/2) from the original formula instead of 1 / cos(θ/2). The maximum allowable junction velocity should go down as θ decreases because the corners in the path that the machine actually follows are sharper. With 1 / cos(θ/2), it stays the same. Thoughts?

I'll have to give that some thought.

-Kevin

[KevinOConnor]

On Sun, Mar 25, 2018 at 04:47:15PM -0700, Michael Barbour wrote:

Also, after thinking about this a bit more, I've concluded that it actually makes more sense to use sin(θ/2) / cos(θ/2) from the original formula instead of 1 / cos(θ/2). The maximum allowable junction velocity should go down as θ decreases because the corners in the path that the machine actually follows are sharper. With 1 / cos(θ/2), it stays the same. Thoughts?

I think you are right. I updated the work-centripetal-20180325 branch accordingly. I doubt it makes much of a difference in practice though, as a small θ is likely to be limited more by the existing junction_deviation check.

Also, thinking about it further, I suspect that this change in general will have little impact on actual circles. With a typical acceleration value (1000-3000) the circle would have to be really small (5mm or smaller radius) before one would likely see a limit on velocity. That said, it may be useful to catch some extreme corner cases - for example, if the moves: G1 X0 Y0 ; G1 X100 Y0 ; G1 X0 Y10 were instead generated as: G1 X0 Y0 ; G1 X99.999 Y0 ; G1 X99.999 Y.001 ; G1 X0 Y10 then the current code would likely try to turn the corner at too high a speed.

-Kevin

[bmc0]

I suspect that this change in general will have little impact on actual circles.

I personally couldn't get it to make a difference with circles. I could make move_centripetal_v2 smaller than R * accel, but it didn't make a difference because the speed was ultimately limited by something else (I didn't actually check what though). A square with rounded corners, however, did trigger the limit.

I've also tested a few common STLs sliced by a couple different slicers (Slic3r and my own, shiv) and found lots of cases where the new code limits the junction speed. For example, the g-code generated by Slic3r for the 3D Hubs Marvin keychain has 3561 instances where the junction speed is reduced (out of 31455 total moves). In most cases there's not a huge reduction, but sometimes the junction speed is reduced to less than half. I have max_accel set to 900 and junction_deviation set to 0.04.

[hg42]

I have checked this with my high speed, high acceleration test on lpc1768 with these parameters:

max_velocity = 2000
max_accel = 5000
max_accel_to_decel = 5000

The test is with a real model but only steppers. Lots of short moves and small circles.

To make the test easier, I added a possibility to switch this on while printing.

I found that it makes a big difference. I didn't test with a real print yet, but I am sure it's an important improvement.

[KevinOConnor]

FYI, I've committed this change to the master branch (commit a4439b93).

[bmc0]

Cool. I guess I'll close this then.