Hey @Noisyfox @SoftFever, this PR looks important, and it's not so much code to review.

That's clever and will remove the need to have arc commands enabled in klipper even if arc fitting is disabled.

However it will need extensive testing. Approach:

1. Disable arc fitting in orca
2. Disable arc commands in klipper
3. Run a single color print
4. Run a multi color print with prime tower
5. run a multi color print with purge tower
6. Run a print with the tower and /or object at the edge of the plate (so the arc command clips the bed plate).

thoughts?

With this PR everything remains the same, every check is preserved. The main difference is that no G2/3 commands are issued if arc fitting is disabled in Orca, the exact arc is reproduced by 24 tiny segments.
If there were issues with arc, they remain, if there were not, none introduced.

An important addition about Arc Filling is that you need to provide such a lock in the printer settings, not only on the Quality tab.
I understand that this is now an outdated property, but nevertheless it is useful.
The fact is that you can install different firmware for the same printer, for example, Marlin, with or without support for this function. And this will help initially protect beginners from accidentally enabling this feature.

The problem is that G2/3 commands are generated regardless of this setting. This PR fixes that.
I do not understand exactly what tests needs to be done... Maybe if my PR really generates a spiral Z hop or not with polylines instead of G2/3.
I do not understand what single color/multicolor, print tower on/off has to do with it...

There have been several issues with spiral moves extending beyond the bed, causing klipper to shut down. This happened on prime towers and on models close to the edge.

Having reviewed the Pr I don’t think this is as relevant, but we need to double check that no movement is done outside the bounds of the printer as a whole.

Arc moves on z hop is why in the orca main page it’s instructed to enable arc fitting in klipper. And disable it in the quality tab (as it’s a double lossy conversion for klipper)

this PR should address the need for any arc fitting to be enabled at all on the klipper side. @pi-squared-studio

There have been several issues with spiral moves extending beyond the bed, causing klipper to shut down. This happened on prime towers and on models close to the edge.

Having reviewed the Pr I don’t think this is as relevant, but we need to double check that no movement is done outside the bounds of the printer as a whole.

I agree, but that is not what this PR is about. This PR generates the exact size of the replaced spiral movement. It cannot introduce any outside of bounds movement that wouldn't be there with the regular spiral (G2/3) movement.

Reviewed the PR. Looks good.

Just one thing - I am curious whether exploring adding a final G1 XYZ emission after the arc is complete with the final, target position. Why? There is the possibility of numerical errors with the sin commands which may or may not result in slight deviation in the XYZ plane vs the intended position. Or maybe even better check the last XYZ position vs the intended position and if a delta > epsilon is present emit a final G1 XYZ correcting command.

What do you think? Is that even relevant or a possibility?

The intended final position is just the start XY and target Z, because we’re doing a full 360° spiral:
• Start: (x0, y0, z_start) = m_pos(0/1/2)
• End: (x0, y0, z) // same XY, new Z

So for the 24-segment version we can do:
• Use trig for the first 23 segments as we do now.
• For the last segment (i == segments), override x,y,z with the exact values.

for example:

for (int i = 1; i <= segments; ++i) {
    double t = double(i) / segments;
    double x, y, zz;

    if (i == segments) {
        // Force exact final position to avoid trig/rounding drift
        x  = x_start;
        y  = y_start;
        zz = z;             // exact target Z
    } else {
        double a = a0 + delta * t;   // CCW arc param
        x  = cx + radius * std::cos(a);
        y  = cy + radius * std::sin(a);
        zz = z_start + (z - z_start) * t;
    }

    oss << "G1 X" << x
        << " Y" << y
        << " Z" << zz << "\n";
}

output = oss.str();
m_pos(2) = z;

I was writing a response with the exact same solution to emit only 23 generated segments and do the last one manually to the intended end position.

However I do not find it necessary as the end position of the spiral ZHop is not critical, it will always be followed by another move command, it will never be the start of an extrusion move.

If you still think it is a vital change it can be done just like this:

for (int i = 1; i < segments; ++i) {
    double t = double(i) / segments;
    double x, y, zz;

    double a = a0 + delta * t;   // CCW arc param
    x  = cx + radius * std::cos(a);
    y  = cy + radius * std::sin(a);
    zz = z_start + (z - z_start) * t;

    oss << "G1 X" << x
        << " Y" << y
        << " Z" << zz << "\n";
}
// final segment to the intended end position
oss << "G1 X" << x_start
    << " Y" << y_start
    << " Z" << z << "\n";

output = oss.str();
m_pos(2) = z;

Yeap that works. I’m not certain of downstream impact as yes XY will move in the next travel position and the starting Z position is not critical for de retraction.

However from a code “elegance” perspective it’s nice to cater for numerical precision issues, as they can be very tricky to diagnose later on if there is an impact somewhere :)

so in short, not critical but more elegant :)

ps. Your code is better than what I proposed, please use that :)

Done.

PS: Thank you for insisting on it, the previous code could have introduced some artifacts/position inconsistency (others than discussed - m_pos(0) and m_pos(1) was not updated in case of the last segment would not end up EXACTLY on the starting XY coordinates).

Perfection! Thank you!

yeah I’ve been bitten a few times with numerical precision results hence I’m being a bit paranoid about it.

My tests showed me that the simulated spiral ends up where it starts (XY) so I didn't give more thought to that.
But you are right, better safe than sorry!

I think this should be re evaluated and I think that spiral z hop should ALWAYS use segments.

In fact, the default setting of 1.0 mm segments used in so many thousands klipper printers (which likely won't ever be updated) causes significant deviations between the commanded arc and the effective path.

For example (see Klipper3d/klipper#6984 ) only at radius of 12.5 mm the standard setting results in less than 0.01 mm deviation.

If I understand correctly the example in the initial screenshot of this pull, the I parameter specifies an offset of the center of rotation from the current position and it's set to ... 1.2 mm!! You can guess how accurate that will be with segments of 1.0 mm! It's about 7.5 segments for a circle.

The "after" screenshot shows not ~8 but 24 segments to approximate the same circle!

So here either:

• we agree that 24 segments are a waste of commands and we reduce them to 10 or so, or
• we agree that the standard klipper setting is unsuitable for this kind of small circles for spiral Z hop and we change the code to use ALWAYS segments for z spiral Z hop, leaving the normal G2/G3 only for the model gcode.

What do you think?

A 2.42mm diameter circle (360°) is made by 16 segments with Orca's standard settings

A 1.21mm radius corner (90°) is made by 4 segments with Orca's standard settings.

So yeah, 24 might be a bit excessive — actually I started with 16 than changed to 24 segments to be closer to a circle. The reasoning of 24 vs 16 is that this is a travel move, not an extrusion one (more segments -> less jerk).

Regarding the other issues you forgot that Orca is not only for Klipper users, some other firmwares do have proper arc commands implemented. (As an example a slower bedslinger Marlin printer is "happier" with G2/3 commands than with rapid tiny segments)

Orca offers tools, the user can select what and how to use based on his needs, machines and their capabilities.

Regarding the other issues you forgot that Orca is not only for Klipper users, some other firmwares do have proper arc commands implemented. (As an example a slower bedslinger Marlin printer is "happier" with G2/3 commands than with rapid tiny segments)

Marlin also splits arcs into segments and, except for recent releases, has the same rule of 1.0 mm per segment Klipper uses by default.
In recent releases they modified the rule with the default "segments from 0.1 to 1.0 mm per segment and no less than 72 segments per circle" instead of fixed 1.0 mm. I'm not sure how widespread the new approach is in actual printers around.

But we know that firmware upgrades are something very few users do, so many with arc enabled in Orca (because of the myth that interpolated and then segmented again arcs are better...) might end up with the same segments Klipper users have.

I'm just saying that for spiral Z hop, a very specific use case, users might get a better experience with explicitly generated segments, and in this case I think 24 as you did is very good for the reason you mentioned (Marlin likes shorter segments better).

Orca offers tools, the user can select what and how to use based on his needs, machines and their capabilities

In theory, but not in practice, since likely most printers around will, in fact, use very rough segments. To preserve the "integrity" of your approach ("use G2/G3 for Z hop if the user chose to do so") the end result is a likely worse user experience without, in fact, any real advantage beside the "principle".

Summary: the G2/G3 option in the print settings should affect the user models only. Spiral Z hop should always use explicit segments.
No one gets worse results, many will get better ones.

The simplest way to address your concerns is to disable "Arc fitting" in Orca.

My concern is not about my case but it's more for people who have it enabled and then find out spiral z hop won't be operating as smooth as it should.

But I think arc fitting is disabled in most profiles, I should check by making a quick search.

I'll also update the Wiki since it's incorrect.

This PR is causing some machines to give -- the print failed with “MCU error: Move queue overflow” when they have spiral zhop.
I think we should create a separate variable for arc spiral zhop.
Does anybody want to do that or should i implement that?

This PR is causing some machines to give -- the print failed with “MCU error: Move queue overflow” when they have spiral zhop. I think we should create a separate variable for arc spiral zhop. Does anybody want to do that or should i implement that?

Can you first investigate a little on Klipper discord what the cause could be? I'm very surprised klipper would be unable to handle saucy motion.

I would in any case try first by reducing from 24 segments to 16 to see if the error disappears: 24 for a circle are really unnecessary

This PR is causing some machines to give -- the print failed with “MCU error: Move queue overflow” when they have spiral zhop. I think we should create a separate variable for arc spiral zhop. Does anybody want to do that or should i implement that?

Can you first investigate a little on Klipper discord what the cause could be? I'm very surprised klipper would be unable to handle saucy motion.

I would in any case try first by reducing from 24 segments to 16 to see if the error disappears: 24 for a circle are really unnecessary

We are talking about that in slicer-settings at discord.

I presented my ideas there (Discord).
... and made a PR

Do you have arcs enabled? Toggle that setting and check if it gets better