From 4d2c3a0af4bcf9a3a813876b1280068a5f318af8 Mon Sep 17 00:00:00 2001 From: harrierpigeon Date: Tue, 8 Sep 2026 23:58:19 -0500 Subject: [PATCH] GCodeWriter: fix two machine-mapping bugs the extraction preserved Both change emitted G-code, which is why they were kept out of the extraction commit. Both are wrong only where the machine mapping is non-identity, which is the definition of each bug. 1. Suppress lifts commanded through an unknown position. _travel_to_z() emits full XYZ whenever the mapping must emit every axis, because the mapping can make machine Z depend on logical X/Y, and it builds that point from m_pos. At print start, and after any custom G-code that invalidates position, m_pos.xy is the uninitialised origin; mapping (0, 0, z) through a non-identity remap produces a real but wrong machine point -- for a reverse mapping, build_vol_max, i.e. the far corner of the bed. The subsequent full-XYZ move corrects the position, but the lift has already commanded a rapid across the whole bed at travel speed. Belt kinematics already guarded this; the Cartesian path did not. The guard is now applied at all three lift sites through must_skip_lift_now(), not just the one the extraction covered: travel_to_xyz()'s pending-lift branch, lazy_lift(spiral_vase=true), and eager_lift(). The latter two also needed the state fix -- both recorded m_lifted = target_lift regardless, so suppressing only the emission would leave a later unlift() descending from a height that was never commanded. 2. Never emit a G2/G3 arc a mapping cannot represent. extrude_arc_to_xy() emitted G2/G3 with logical X/Y and I/J and never consulted the mapping. There is no general fix by transforming the arc: a permutation moves it out of the XY plane that I/J describes, a negation reverses handedness, and the belt shear maps a circle to an ellipse that G2/G3 cannot express at all. So supports_arc_moves() gates generation through the existing GCode::should_disable_arc_fitting() hook, and BeltGCode's special-case override is deleted -- belt now gets the same behaviour from the general rule instead of its own exception. supports_arc_moves() is m_remap_x == 0 && m_remap_y == 1, not !has_axis_remap(): an arc emits only X/Y/I/J, so a mapping that merely negates or reverses Z leaves every emitted word untouched and keeps its arcs. The fallback for an unrepresentable arc tessellates it into linear segments at a 0.005mm chord tolerance rather than substituting a single chord, and splits dE proportionally across the segments. The capability check is hoisted above every extrusion mutation: an earlier form ran it after filament()->extrude(dE) and so extruded 2*dE on the fallback path. Known limits of that fallback, since it is worth stating rather than discovering: emitted relative E is conserved only to per-segment rounding (a radius-5 semicircle with dE=1.5 emits 1.50012 across 36 segments); the 0.005mm bound is a logical-frame bound, about 0.00855mm in machine space under a 45-degree belt shear; unequal endpoint radii and non-finite inputs are unchecked. Ordinary export takes the original polyline when the mapping rejects arcs, so this path is a fallback rather than the normal route. Known gap, not claimed fixed: classic wipe towers have their own enable_arc_fitting and their own G2/G3 emitter in GCode/WipeTower.cpp, which should_disable_arc_fitting() does not govern. Belt printers are barred from classic wipe towers; a remapped Cartesian printer is not. Tests in tests/fff_print/test_gcodewriter.cpp: reverse-X remap with unknown and with known position plus an identity control; eager_lift emitting nothing and recording nothing; the arc-capability matrix including the Z-only cases; and the tessellated fallback. E accounting is asserted through used_filament() rather than E(), which resets per line in relative-E mode. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011jgzj1sf53KMLPweZ8yeUQ --- src/libslic3r/BeltGCode.hpp | 4 +- src/libslic3r/GCode.hpp | 6 +- src/libslic3r/GCode/BeltKinematics.hpp | 3 + src/libslic3r/GCode/MachineKinematics.hpp | 29 ++- src/libslic3r/GCodeWriter.cpp | 75 +++++- src/libslic3r/GCodeWriter.hpp | 8 + tests/fff_print/test_gcodewriter.cpp | 295 ++++++++++++++++++++++ 7 files changed, 404 insertions(+), 16 deletions(-) diff --git a/src/libslic3r/BeltGCode.hpp b/src/libslic3r/BeltGCode.hpp index f52c62c28f..9598e9ebd5 100644 --- a/src/libslic3r/BeltGCode.hpp +++ b/src/libslic3r/BeltGCode.hpp @@ -10,14 +10,14 @@ namespace Slic3r { // - Install a BeltKinematics on the GCodeWriter // - Write belt configuration to the G-code header // - Adjust the origin for global pre-slice transforms when switching instances -// - Disable arc fitting (G2/G3 not supported on belt printers) +// (Arc fitting is disabled for belt printers by BeltKinematics::supports_arc_moves(), +// which the base GCode::should_disable_arc_fitting() consults -- no override needed.) class BeltGCode : public GCode { protected: void init_belt_writer(Print &print, bool is_bbl_printers) override; void write_belt_header(GCodeOutputStream &file, const Print &print) override; void on_set_origin(const PrintObject *obj, const Point &inst_shift) override; - bool should_disable_arc_fitting() const override { return true; } }; } // namespace Slic3r diff --git a/src/libslic3r/GCode.hpp b/src/libslic3r/GCode.hpp index 56496f1b4e..26b8bc78a5 100644 --- a/src/libslic3r/GCode.hpp +++ b/src/libslic3r/GCode.hpp @@ -379,7 +379,11 @@ protected: virtual void init_belt_writer(Print &print, bool is_bbl_printers) {} virtual void write_belt_header(GCodeOutputStream &file, const Print &print) {} virtual void on_set_origin(const PrintObject *obj, const Point &inst_shift) {} - virtual bool should_disable_arc_fitting() const { return false; } + // Arc fitting is suppressed whenever the writer's machine mapping cannot + // represent a G2/G3 arc. Belt printers get this through BeltKinematics + // rather than through an override of their own. + virtual bool should_disable_arc_fitting() const + { return ! m_writer->kinematics().supports_arc_moves(); } void _do_export(Print &print, GCodeOutputStream &file, ThumbnailsGeneratorCallback thumbnail_cb); diff --git a/src/libslic3r/GCode/BeltKinematics.hpp b/src/libslic3r/GCode/BeltKinematics.hpp index a57f18dcdd..f145ecf3e9 100644 --- a/src/libslic3r/GCode/BeltKinematics.hpp +++ b/src/libslic3r/GCode/BeltKinematics.hpp @@ -42,6 +42,9 @@ public: // emitted G-code for an identity-transform belt configuration. bool must_emit_all_axes() const override { return true; } bool suppress_lift_at_unknown_position() const override { return true; } + // The machine frame shears and scales, so a circle is an ellipse in machine + // coordinates and G2/G3 cannot describe it. + bool supports_arc_moves() const override { return false; } bool world_coordinates() const { return m_world_coordinates; } diff --git a/src/libslic3r/GCode/MachineKinematics.hpp b/src/libslic3r/GCode/MachineKinematics.hpp index c3e381d851..e046dd9476 100644 --- a/src/libslic3r/GCode/MachineKinematics.hpp +++ b/src/libslic3r/GCode/MachineKinematics.hpp @@ -40,12 +40,21 @@ public: // axis permutation forces full emission without physically coupling axes. virtual bool must_emit_all_axes() const = 0; - // True when a separate in-place lift must be suppressed while the current - // position is unknown, because _travel_to_z() re-emits the logical X/Y - // through this mapping and an uninitialised position would map to a bogus - // machine point. + // True when a lift must be suppressed while the current position is unknown, + // because _travel_to_z() re-emits the logical X/Y through this mapping and an + // uninitialised position would map to a bogus machine point -- for a reverse + // mapping, the far corner of the bed. virtual bool suppress_lift_at_unknown_position() const = 0; + // True when a G2/G3 arc in the logical XY plane is still the same arc in the + // machine frame. Arc moves emit only X, Y, I and J, so this asks a narrower + // question than must_emit_all_axes(): whether logical X and Y reach the + // machine unchanged. A mapping that only negates or reverses Z keeps its + // arcs; one that permutes X or Y moves the arc out of the plane that I/J + // describes, and a shear turns the circle into an ellipse G2/G3 cannot + // express at all. + virtual bool supports_arc_moves() const = 0; + // Configuration. GCodeWriter forwards its setters here so that the state // lives with the strategy and a strategy installed before the setters run // still receives it. @@ -67,11 +76,13 @@ public: Vec3d to_build_volume(const Vec3d &machine) const override { return machine; } bool must_emit_all_axes() const override { return this->has_axis_remap(); } - // The base writer has never suppressed the lift, not even under a remap that - // makes _travel_to_z re-emit X/Y. That is arguably a latent bug, but fixing - // it here would change emitted G-code, so today's behaviour is preserved and - // the divergence from BeltKinematics is deliberate. - bool suppress_lift_at_unknown_position() const override { return false; } + bool suppress_lift_at_unknown_position() const override { return this->has_axis_remap(); } + + // X and Y must reach the machine untouched. Because the remap is a + // permutation, pinning those two also pins Z to Z, so a mapping that only + // negates or reverses Z still supports arcs -- every word a G2/G3 emits is + // unchanged by it. + bool supports_arc_moves() const override { return m_remap_x == 0 && m_remap_y == 1; } void set_axis_remap(int rx, int ry, int rz) override { m_remap_x = rx; m_remap_y = ry; m_remap_z = rz; } diff --git a/src/libslic3r/GCodeWriter.cpp b/src/libslic3r/GCodeWriter.cpp index be8f6607de..72c720cac6 100644 --- a/src/libslic3r/GCodeWriter.cpp +++ b/src/libslic3r/GCodeWriter.cpp @@ -25,6 +25,15 @@ namespace Slic3r { bool GCodeWriter::full_gcode_comment = true; +// A lift emitted through _travel_to_z() re-emits the stored logical X/Y under a +// mapping that must emit every axis. While the position is unknown that X/Y is +// the uninitialised origin, which maps to a real but wrong machine point, so the +// lift has to be skipped rather than commanded. +bool GCodeWriter::must_skip_lift_now() const +{ + return m_kinematics->suppress_lift_at_unknown_position() && ! this->is_current_position_clear(); +} + bool GCodeWriter::point_on_first_layer(const Vec3d &point_logical) const { if (m_first_layer_plane && m_first_layer_plane->is_active()) @@ -837,6 +846,10 @@ std::string GCodeWriter::lazy_lift(LiftType lift_type, bool spiral_vase) // BBS if (m_lifted == 0 && m_to_lift == 0 && target_lift > 0) { if (spiral_vase) { + if (this->must_skip_lift_now()) + // Record no lift, so a later unlift() does not descend from a + // height that was never commanded. + return ""; m_lifted = target_lift; return this->_travel_to_z(m_pos(2) + target_lift, "lift Z"); } @@ -882,7 +895,12 @@ std::string GCodeWriter::eager_lift(const LiftType type) { } //BBS: if position is unknown use normal lift else if (target_lift > 0) { - lift_move = _travel_to_z(m_pos(2) + target_lift, "normal lift Z"); + if (this->must_skip_lift_now()) + // Skipped, not deferred: leave m_lifted at zero below so unlift() + // does not descend from a height that was never commanded. + target_lift = 0.; + else + lift_move = _travel_to_z(m_pos(2) + target_lift, "normal lift Z"); } m_lifted = target_lift; m_to_lift = 0; @@ -966,9 +984,7 @@ std::string GCodeWriter::travel_to_xyz(const Vec3d &point, const std::string &co w0.emit_comment(GCodeWriter::full_gcode_comment, comment); slop_move = w0.string(); } - else if (m_to_lift_type == LiftType::NormalLift && - (! m_kinematics->suppress_lift_at_unknown_position() || - this->is_current_position_clear())) { + else if (m_to_lift_type == LiftType::NormalLift && ! this->must_skip_lift_now()) { // Only lift in place when the current position is known, for a mapping // that makes _travel_to_z re-emit logical X/Y: at print start (and after // custom gcode) m_pos.xy is still the uninitialised origin, which would @@ -1220,11 +1236,62 @@ std::string GCodeWriter::extrude_to_xy(const Vec2d &point, double dE, const std: return w.string(); } +// Approximate an arc with linear extrusions, for machine mappings that cannot +// express a G2/G3 (see extrude_arc_to_xy). center_offset is I/J: the centre +// relative to the CURRENT position, which is why this must run before m_pos is +// updated. +std::string GCodeWriter::extrude_arc_as_polyline(const Vec2d &point, const Vec2d ¢er_offset, + double dE, const bool is_ccw, + const std::string &comment, bool force_no_extrusion) +{ + const Vec2d start = Vec2d(m_pos.x(), m_pos.y()); + const Vec2d centre = start + center_offset; + const double r = (start - centre).norm(); + if (r < EPSILON) + // Degenerate: no arc to speak of, so a single move is exact. + return this->extrude_to_xy(point, dE, comment, force_no_extrusion); + + double a0 = std::atan2(start.y() - centre.y(), start.x() - centre.x()); + double a1 = std::atan2(point.y() - centre.y(), point.x() - centre.x()); + double sweep = a1 - a0; + if (is_ccw) { while (sweep <= 0.) sweep += 2. * PI; } + else { while (sweep >= 0.) sweep -= 2. * PI; } + + // Segment count from a chord-deviation bound: r*(1-cos(dtheta/2)) <= tol. + const double tol = 0.005; // mm + const double dmax = (tol >= r) ? PI : 2. * std::acos(1. - tol / r); + const int n = std::max(2, int(std::ceil(std::abs(sweep) / std::max(dmax, EPSILON)))); + + std::string out; + for (int i = 1; i <= n; ++ i) { + const double a = a0 + sweep * (double(i) / double(n)); + const Vec2d p = (i == n) ? point + : Vec2d(centre.x() + r * std::cos(a), centre.y() + r * std::sin(a)); + out += this->extrude_to_xy(p, dE / double(n), i == n ? comment : std::string(), force_no_extrusion); + } + return out; +} + //BBS: generate G2 or G3 extrude which moves by arc //point is end point which means X and Y axis //center_offset is I and J axis std::string GCodeWriter::extrude_arc_to_xy(const Vec2d& point, const Vec2d& center_offset, double dE, const bool is_ccw, const std::string& comment, bool force_no_extrusion) { + // Arcs emit only X/Y/I/J, so a mapping that moves logical X or Y cannot be + // expressed as a G2/G3. GCode::should_disable_arc_fitting() normally stops + // arcs being generated at all for such a mapping, but this is public API, so + // define the behaviour rather than asserting. + // + // This check MUST precede every state mutation below: falling through to + // extrude_to_xy() after filament()->extrude(dE) would advance E twice. + // + // A single chord is not a safe substitute either -- a semicircle would become + // its diameter and a full circle a stationary blob -- so approximate the arc + // with linear segments bounded by a chord tolerance, splitting dE between + // them in proportion to arc length. + if (! m_kinematics->supports_arc_moves()) + return this->extrude_arc_as_polyline(point, center_offset, dE, is_ccw, comment, force_no_extrusion); + m_pos(0) = point(0); m_pos(1) = point(1); if (!force_no_extrusion) diff --git a/src/libslic3r/GCodeWriter.hpp b/src/libslic3r/GCodeWriter.hpp index f5e037d685..a382f4abae 100644 --- a/src/libslic3r/GCodeWriter.hpp +++ b/src/libslic3r/GCodeWriter.hpp @@ -90,6 +90,10 @@ public: virtual std::string extrude_to_xy(const Vec2d &point, double dE, const std::string &comment = std::string(), bool force_no_extrusion = false); //BBS: generate G2 or G3 extrude which moves by arc std::string extrude_arc_to_xy(const Vec2d &point, const Vec2d ¢er_offset, double dE, const bool is_ccw, const std::string &comment = std::string(), bool force_no_extrusion = false); + // Linear approximation of an arc, used when the machine mapping cannot + // express a G2/G3. Must be called before m_pos is updated: center_offset is + // relative to the current position. + std::string extrude_arc_as_polyline(const Vec2d &point, const Vec2d ¢er_offset, double dE, const bool is_ccw, const std::string &comment = std::string(), bool force_no_extrusion = false); virtual std::string extrude_to_xyz(const Vec3d &point, double dE, const std::string &comment = std::string(), bool force_no_extrusion = false); std::string retract(bool before_wipe = false, double retract_length = 0); std::string retract_for_toolchange(bool before_wipe = false, double retract_length = 0); @@ -186,6 +190,10 @@ protected: // m_is_first_layer flag does. bool point_on_first_layer(const Vec3d &point_logical) const; + // True when a lift must be skipped because this mapping would emit the + // stored logical X/Y and that position is not yet known. + bool must_skip_lift_now() const; + // True when travel speed is selected per destination point rather than per // layer. Set for writers that install a first-layer plane. The historical // path emits the raw configured travel speed in the final branch of diff --git a/tests/fff_print/test_gcodewriter.cpp b/tests/fff_print/test_gcodewriter.cpp index 522d945948..18a720aaff 100644 --- a/tests/fff_print/test_gcodewriter.cpp +++ b/tests/fff_print/test_gcodewriter.cpp @@ -1032,3 +1032,298 @@ SCENARIO("Belt: start-gcode prepare-stage moves keep their real Z", "[GCode][bel } } } + +// --------------------------------------------------------------------------- +// Regression tests for the two latent bugs the MachineKinematics refactor +// preserved deliberately (see 09b-latent-bug-fix-plan.md). +// --------------------------------------------------------------------------- + +// Bug 1. _travel_to_z() emits full XYZ whenever the mapping must emit every +// axis, and it builds that point from m_pos. While the position is unknown, +// m_pos.xy is the uninitialised origin, which a reverse remap maps to the far +// corner of the bed. Belt kinematics guarded this; a Cartesian writer with an +// axis remap did not, and would command a rapid across the whole bed. +static void configure_lift_writer(GCodeWriter &writer) +{ + std::vector extruder_ids { 0 }; + writer.set_extruders(extruder_ids); + writer.set_extruder(0); + writer.config.travel_speed.values = { 100.0 }; + writer.config.travel_speed_z.values = { 100.0 }; + writer.config.z_hop.values = { 0.4 }; + writer.config.retract_lift_above.values = { 0.0 }; + writer.config.retract_lift_below.values = { 0.0 }; +} + +// Largest X word in a chunk of emitted G-code, or lowest() if none. +static double max_emitted_x(const std::string &gcode) +{ + double max_x = std::numeric_limits::lowest(); + GCodeReader reader; + reader.parse_buffer(gcode, [&max_x](GCodeReader &, const GCodeReader::GCodeLine &line) { + if (line.cmd_is("G1") && line.has(X)) + max_x = std::max(max_x, double(line.x())); + }); + return max_x; +} + +static size_t count_g1(const std::string &gcode) +{ + size_t n = 0; + GCodeReader reader; + reader.parse_buffer(gcode, [&n](GCodeReader &, const GCodeReader::GCodeLine &line) { + if (line.cmd_is("G1")) ++n; + }); + return n; +} + +SCENARIO("Axis remap: no lift is commanded through the uninitialised origin", "[GCodeWriter][remap]") +{ + // Reverse X: machine X = build_vol_max.x - logical X, so the uninitialised + // origin maps to the far edge of the bed and is unmistakable in the output. + const double bed_x = 250.0; + + GIVEN("a writer with a reverse-X remap and an unknown current position") { + GCodeWriter writer; + configure_lift_writer(writer); + writer.set_axis_remap(6, 1, 2); + writer.set_build_volume_max(Vec3d(bed_x, 250.0, 250.0)); + REQUIRE(writer.kinematics().must_emit_all_axes()); + REQUIRE_FALSE(writer.is_current_position_clear()); + + WHEN("a z-hop is pending and we travel to the first point") { + writer.lazy_lift(LiftType::NormalLift); + const std::string gcode = writer.travel_to_xyz(Vec3d(10.0, 10.0, 5.0)); + + THEN("nothing is commanded at the image of the origin") { + // The destination maps to machine X = 250 - 10 = 240; the bogus + // origin lift would have mapped to machine X = 250. + REQUIRE(max_emitted_x(gcode) < bed_x - 1.0); + } + THEN("only the destination move is emitted") { + REQUIRE(count_g1(gcode) == 1); + } + } + } + + GIVEN("the same writer once its position is known") { + GCodeWriter writer; + configure_lift_writer(writer); + writer.set_axis_remap(6, 1, 2); + writer.set_build_volume_max(Vec3d(bed_x, 250.0, 250.0)); + writer.travel_to_xyz(Vec3d(20.0, 20.0, 5.0)); + REQUIRE(writer.is_current_position_clear()); + + WHEN("a z-hop is pending and we travel again") { + writer.lazy_lift(LiftType::NormalLift); + const std::string gcode = writer.travel_to_xyz(Vec3d(30.0, 30.0, 5.0)); + + THEN("the separate lift move is still emitted") { + // Suppression must be pinned to the unknown position, not to the + // presence of a remap. + REQUIRE(count_g1(gcode) == 2); + } + } + } + + GIVEN("an identity-mapping writer with an unknown position") { + GCodeWriter writer; + configure_lift_writer(writer); + REQUIRE_FALSE(writer.kinematics().must_emit_all_axes()); + REQUIRE_FALSE(writer.is_current_position_clear()); + + WHEN("a z-hop is pending and we travel to the first point") { + writer.lazy_lift(LiftType::NormalLift); + const std::string gcode = writer.travel_to_xyz(Vec3d(10.0, 10.0, 5.0)); + + THEN("behaviour is unchanged: the lift is still emitted") { + // Three moves, not two: with no remap and an unknown position the + // destination is emitted as a separate XY move followed by its own + // Z move, on top of the lift. That split is the pre-existing + // identity-mapping path and must not change. + REQUIRE(count_g1(gcode) == 3); + } + } + } +} + +SCENARIO("Axis remap: eager_lift does not lift, or record a lift, at an unknown position", + "[GCodeWriter][remap]") +{ + GIVEN("a writer with a reverse-X remap and an unknown current position") { + GCodeWriter writer; + configure_lift_writer(writer); + writer.set_axis_remap(6, 1, 2); + writer.set_build_volume_max(Vec3d(250.0, 250.0, 250.0)); + REQUIRE_FALSE(writer.is_current_position_clear()); + + WHEN("an eager lift is requested") { + const std::string lift = writer.eager_lift(LiftType::NormalLift); + + THEN("no move is emitted") { + REQUIRE(lift.empty()); + } + THEN("no lift is recorded, so unlift does not descend from it") { + // If m_lifted had been set while nothing was commanded, unlift() + // would emit a descent from a height the machine never reached. + REQUIRE(writer.unlift().empty()); + } + } + } + + GIVEN("an identity-mapping writer with an unknown position") { + GCodeWriter writer; + configure_lift_writer(writer); + + WHEN("an eager lift is requested") { + const std::string lift = writer.eager_lift(LiftType::NormalLift); + + THEN("behaviour is unchanged: the lift is emitted and can be undone") { + REQUIRE_FALSE(lift.empty()); + REQUIRE_FALSE(writer.unlift().empty()); + } + } + } +} + +// Bug 2. extrude_arc_to_xy() emits G2/G3 with logical X/Y and I/J and never +// consulted the mapping. An arc is only representable when logical X and Y reach +// the machine unchanged -- which is a narrower question than "is the remap the +// identity", because a mapping that only touches Z leaves every emitted word alone. +SCENARIO("Arc support is decided by whether the mapping leaves X and Y alone", "[GCodeWriter][remap]") +{ + GIVEN("a Cartesian writer") { + GCodeWriter writer; + + THEN("the identity mapping supports arcs") { + REQUIRE(writer.kinematics().supports_arc_moves()); + } + THEN("a Z-only negation still supports arcs") { + // (+X, +Y, -Z): non-identity, but X, Y, I and J are all untouched. + writer.set_axis_remap(0, 1, 5); + REQUIRE(writer.kinematics().must_emit_all_axes()); + REQUIRE(writer.kinematics().supports_arc_moves()); + } + THEN("a Z-only reversal still supports arcs") { + writer.set_axis_remap(0, 1, 8); + REQUIRE(writer.kinematics().supports_arc_moves()); + } + THEN("swapping X and Y does not support arcs") { + writer.set_axis_remap(1, 0, 2); + REQUIRE_FALSE(writer.kinematics().supports_arc_moves()); + } + THEN("the X-tilt style (x, z, y) remap does not support arcs") { + writer.set_axis_remap(0, 2, 1); + REQUIRE_FALSE(writer.kinematics().supports_arc_moves()); + } + } + + GIVEN("a belt writer") { + PrintConfig belt_config; + belt_config.belt_printer.value = true; + belt_config.belt_slice_rotation.value = BeltRotationAxis::X; + belt_config.belt_slice_rotation_angle.value = 45.0; + + GCodeWriter writer; + install_belt_kinematics(writer, belt_config); + + THEN("arcs are never supported, because the frame shears") { + REQUIRE_FALSE(writer.kinematics().supports_arc_moves()); + } + } +} + +SCENARIO("An unrepresentable arc degrades to its chord rather than emitting a wrong G2/G3", + "[GCodeWriter][remap]") +{ + auto emitted_commands = [](const std::string &gcode) { + std::vector cmds; + GCodeReader reader; + reader.parse_buffer(gcode, [&cmds](GCodeReader &, const GCodeReader::GCodeLine &line) { + if (! line.cmd().empty()) cmds.emplace_back(line.cmd()); + }); + return cmds; + }; + + GIVEN("an identity-mapping writer") { + GCodeWriter writer; + configure_lift_writer(writer); + + WHEN("an arc is extruded") { + const std::string gcode = writer.extrude_arc_to_xy( + Vec2d(10.0, 0.0), Vec2d(5.0, 0.0), 0.0, /*is_ccw=*/true, "", /*force_no_extrusion=*/true); + + THEN("it is still a G3") { + const auto cmds = emitted_commands(gcode); + REQUIRE(cmds.size() == 1); + REQUIRE(cmds.front() == "G3"); + } + } + } + + GIVEN("a writer whose mapping swaps X and Y") { + GCodeWriter writer; + configure_lift_writer(writer); + writer.set_axis_remap(1, 0, 2); + + WHEN("an arc is extruded") { + const std::string gcode = writer.extrude_arc_to_xy( + Vec2d(10.0, 0.0), Vec2d(5.0, 0.0), 0.0, /*is_ccw=*/true, "", /*force_no_extrusion=*/true); + + THEN("no arc is emitted; it is approximated with linear moves") { + const auto cmds = emitted_commands(gcode); + REQUIRE(! cmds.empty()); + for (const auto &c : cmds) + REQUIRE(c == "G1"); + } + } + } + + // The first version of this test used dE = 0 with force_no_extrusion, which + // hid a real bug: the capability check sat AFTER filament()->extrude(dE), so + // the fallback into extrude_to_xy() advanced E twice. Extrusion accounting has + // to be asserted with a positive dE. + GIVEN("a writer whose mapping cannot express arcs, extruding a real amount") { + GCodeWriter writer; + configure_lift_writer(writer); + writer.set_axis_remap(1, 0, 2); + const double dE = 1.5; + // used_filament() accumulates across moves; E() is reset per line in + // relative-E mode, so it would only show the last segment. + const double used_before = writer.filament()->used_filament(); + + WHEN("an arc carrying that extrusion is emitted") { + const std::string gcode = writer.extrude_arc_to_xy( + Vec2d(10.0, 0.0), Vec2d(5.0, 0.0), dE, /*is_ccw=*/true, "", /*force_no_extrusion=*/false); + + THEN("exactly dE is accounted for, not twice dE") { + REQUIRE_THAT(writer.filament()->used_filament() - used_before, + Catch::Matchers::WithinAbs(dE, 1e-6)); + } + THEN("no G2/G3 survives") { + REQUIRE(gcode.find("G2") == std::string::npos); + REQUIRE(gcode.find("G3") == std::string::npos); + } + } + } + + GIVEN("a writer whose mapping CAN express arcs, extruding a real amount") { + GCodeWriter writer; + configure_lift_writer(writer); + const double dE = 1.5; + // used_filament() accumulates across moves; E() is reset per line in + // relative-E mode, so it would only show the last segment. + const double used_before = writer.filament()->used_filament(); + + WHEN("an arc carrying that extrusion is emitted") { + const std::string gcode = writer.extrude_arc_to_xy( + Vec2d(10.0, 0.0), Vec2d(5.0, 0.0), dE, /*is_ccw=*/true, "", /*force_no_extrusion=*/false); + + THEN("it is still a single arc and accounts for dE once") { + REQUIRE(emitted_commands(gcode).size() == 1); + REQUIRE_THAT(writer.filament()->used_filament() - used_before, + Catch::Matchers::WithinAbs(dE, 1e-6)); + } + } + } +}