[BUG] G2/G3 Arc Math Error for min_segments variable

[ghost]

Bug Description

Following from a discussion in the discord chat, I am unsure if this is a bug or not, so the community would have to first confirm if this is an actual marlin bug or I made a math error myself somewhere, but thought I'd just post it here just in case it is a bug. Basically, it appears as though equivalent arc moves in opposite directions use different number of minimum segments. i.e. 60 degrees G2 clockwise uses more segments than 60 degrees G3 counterclockwise.

Current Version:

  // CCW angle of rotation between position and target from the circle center. Only one atan2() trig computation required.
  float angular_travel = ATAN2(rvec.a * rt_Y - rvec.b * rt_X, rvec.a * rt_X + rvec.b * rt_Y);
  if (angular_travel < 0) angular_travel += RADIANS(360);
  #ifdef MIN_ARC_SEGMENTS
    uint16_t min_segments = CEIL((MIN_ARC_SEGMENTS) * (angular_travel / RADIANS(360)));
    NOLESS(min_segments, 1U);
  #else
    constexpr uint16_t min_segments = 1;
  #endif
  if (clockwise) angular_travel -= RADIANS(360);

Suggested Fix:

  // CCW angle of rotation between position and target from the circle center. Only one atan2() trig computation required.
  float angular_travel = ATAN2(rvec.a * rt_Y - rvec.b * rt_X, rvec.a * rt_X + rvec.b * rt_Y);
  if (angular_travel < 0) angular_travel += RADIANS(360);
  if (clockwise) angular_travel -= RADIANS(360);
  #ifdef MIN_ARC_SEGMENTS
    uint16_t min_segments = CEIL((MIN_ARC_SEGMENTS) * (ABS(angular_travel) / RADIANS(360)));
    NOLESS(min_segments, 1U);
  #else
    constexpr uint16_t min_segments = 1;
  #endif

My math/problem visually described:
https://cdn.discordapp.com/attachments/496524752211410952/779457197926645780/image.png

[ghost]

Discord link: https://discord.com/channels/461605380783472640/496524752211410952/778821453721501780

[sjasonsmith]

I threw together a constexpr function to verify that a negative angular_tavel receives a very different number of segments.

// Original Code Extracted to constexpr function
constexpr float get_min_segments(float angular_travel) {
  if (angular_travel < 0) angular_travel += RADIANS(360);
  float min_segments = (MIN_ARC_SEGMENTS) * (angular_travel / RADIANS(360));
  NOLESS(min_segments, 1U);
  return min_segments;
}

// Result == 1.90985918F
constexpr auto cw_minseg = get_min_segments(0.5);
// Result == 22.0901413F
constexpr auto ccw_minseg = get_min_segments(-0.5);

I also verified that the proposed method inverts the issue, rather than correcting it.

// Proposed Code Extracted to constexpr function
constexpr float get_min_segments_proposed(float angular_travel) {
  const clockwise = true;
  if (angular_travel < 0) angular_travel += RADIANS(360);
  if (clockwise) angular_travel -= RADIANS(360);
  float min_segments = (MIN_ARC_SEGMENTS) * (ABS(angular_travel) / RADIANS(360));
  NOLESS(min_segments, 1U);
  return min_segments;
}

// Result == 22.0901413F
constexpr auto cw_minseg_p = get_min_segments_proposed(0.5);
// Result == 1.90985918F
constexpr auto ccw_minseg_p = get_min_segments_proposed(-0.5);

I'm not entirely sure how this fits into the larger arc code, but the solution seems far simpler. angular_travel is already used as an absolute value. If angular_travel reflects the radians travelled, then the actual direction should not be important at all.

constexpr float get_min_segments_proposed2(float angular_travel) {
  float min_segments = (MIN_ARC_SEGMENTS) * (ABS(angular_travel) / RADIANS(360));
  NOLESS(min_segments, 1U);
  return min_segments;
}

// Result == 1.90985918F
constexpr auto cw_minseg_p2 = get_min_segments_proposed2(0.5);
// Result == 1.90985918F
constexpr auto ccw_minseg_p2 = get_min_segments_proposed2(-0.5);

I don't really intend to do further work with this, at least for now. Hopefully this helps someone else evaluate whether something needs to be repaired here.

[sjasonsmith]

@yysh12 did you have actual print problems that caused you to investigate this?

[ghost]

Not sure. I was having problems where gaps between some parts of the circle appeared different than others parts of the same circle and I was trying to determine if the points generated by marlin significantly differed from what Cura generates using linear movements and that is when I noticed this in the code. So possibly, but it could also possibly be that my tolerances in the post processing arcwelder plugin are too wide. I am still working out all the math in both projects and it will probably take me a while to get through it all. Also, I am away from my printer for possibly 4-6 months, so I won't be able to test any of it myself until then.

[sjasonsmith]

I flagged it as help wanted. Maybe someone else interested in arc movements will pick this up.

[ghost]

Sounds good. Thanks again for everything Jason!

[ghost]

Fix has been merged. Closing issue.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.