From d1d14329d95fa9f8bd13c24efecb16d9c7eb1e1a Mon Sep 17 00:00:00 2001 From: Kris Austin Date: Thu, 1 Oct 2026 06:08:03 -0500 Subject: [PATCH] fix: exporting a sliced print again gives different G-code (#16025) extrude_infill() and extrude_support() reversed the layer's extrusion entities in place while chaining them, so each export started from the previous one's reversed toolpaths, and each copy of an object from the copy before it. The export-time region lists now hold const pointers, and chaining reverses a clone instead. --- src/libslic3r/GCode.cpp | 58 +++++++++++++++++++------------- src/libslic3r/GCode.hpp | 27 +++++++-------- src/libslic3r/ShortestPath.cpp | 42 +++++++++++++++++++---- src/libslic3r/ShortestPath.hpp | 4 +++ tests/fff_print/test_helpers.cpp | 5 +-- tests/fff_print/test_helpers.hpp | 6 ++-- tests/fff_print/test_print.cpp | 34 +++++++++++++++++++ 7 files changed, 127 insertions(+), 49 deletions(-) diff --git a/src/libslic3r/GCode.cpp b/src/libslic3r/GCode.cpp index dedd024f2b..4c02cae7b4 100644 --- a/src/libslic3r/GCode.cpp +++ b/src/libslic3r/GCode.cpp @@ -7310,12 +7310,12 @@ static std::unique_ptr calculate_layer_edge_grid(const Layer& la return out; } -std::string GCode::extrude_loop(const ExtrusionLoop& loop_ref, - const std::string& description, - double speed, - const ExtrusionEntitiesPtr& region_perimeters, - const Point* start_point, - const WipeInwardSupport* wipe_support) +std::string GCode::extrude_loop(const ExtrusionLoop& loop_ref, + const std::string& description, + double speed, + const std::vector& region_perimeters, + const Point* start_point, + const WipeInwardSupport* wipe_support) { // get a copy; don't modify the orientation of the original loop object otherwise // next copies (if any) would not detect the correct orientation @@ -7659,11 +7659,11 @@ std::string GCode::extrude_multi_path(const ExtrusionMultiPath& multipath, const return gcode; } -std::string GCode::extrude_entity(const ExtrusionEntity& entity, - const std::string& description, - double speed, - const ExtrusionEntitiesPtr& region_perimeters, - const WipeInwardSupport* wipe_support) +std::string GCode::extrude_entity(const ExtrusionEntity& entity, + const std::string& description, + double speed, + const std::vector& region_perimeters, + const WipeInwardSupport* wipe_support) { if (const ExtrusionPath* path = dynamic_cast(&entity)) return this->extrude_path(*path, description, speed); @@ -7754,23 +7754,32 @@ std::string GCode::extrude_perimeters(const Print &print, const std::vector &by_region, bool ironing) { - std::string gcode; - ExtrusionEntitiesPtr extrusions; - const char* extrusion_name = ironing ? "ironing" : "infill"; + std::string gcode; + std::vector extrusions; + std::vector> reversed; + const char* extrusion_name = ironing ? "ironing" : "infill"; for (const ObjectByExtruder::Island::Region ®ion : by_region) if (! region.infills.empty()) { extrusions.clear(); extrusions.reserve(region.infills.size()); - for (ExtrusionEntity *ee : region.infills) + for (const ExtrusionEntity *ee : region.infills) if ((ee->role() == erIroning) == ironing) extrusions.emplace_back(ee); if (! extrusions.empty()) { m_config.apply(print.get_print_region(®ion - &by_region.front()).config()); - chain_and_reorder_extrusion_entities(extrusions, m_last_pos.to_point()); + reversed.clear(); + chain_and_reorder_extrusion_entities(extrusions, m_last_pos.to_point(), reversed); + // The reversed copies are in chain order. + auto next_reversed = reversed.begin(); for (const ExtrusionEntity *fill : extrusions) { + ExtrusionEntity *own_copy = next_reversed != reversed.end() && next_reversed->get() == fill ? (next_reversed++)->get() : nullptr; auto *eec = dynamic_cast(fill); if (eec) { - for (ExtrusionEntity *ee : eec->chained_path_from(m_last_pos.to_point()).entities) + // A reversed copy is owned here and can be moved from. + ExtrusionEntityCollection chained = own_copy ? std::move(static_cast(*own_copy)) : ExtrusionEntityCollection(*eec); + if (!chained.no_sort) + chain_and_reorder_extrusion_entities(chained.entities, m_last_pos.to_point()); + for (ExtrusionEntity *ee : chained.entities) gcode += this->extrude_entity(*ee, extrusion_name); } else gcode += this->extrude_entity(*fill, extrusion_name); @@ -7808,9 +7817,9 @@ std::string GCode::extrude_support(const ExtrusionEntityCollection &support_fill std::string gcode; if (!support_fills.entities.empty()) { - ExtrusionEntitiesPtr extrusions; + std::vector extrusions; extrusions.reserve(support_fills.entities.size()); - for (ExtrusionEntity* ee : support_fills.entities) { + for (const ExtrusionEntity* ee : support_fills.entities) { const auto role = ee->role(); if ((role == support_extrusion_role) || (support_extrusion_role == erMixed && role != erIroning)) { extrusions.emplace_back(ee); @@ -7819,9 +7828,10 @@ std::string GCode::extrude_support(const ExtrusionEntityCollection &support_fill if (extrusions.empty()) return gcode; + std::vector> reversed; //ORCA: Respect no_sort to preserve support base outline->fill order. if (!support_fills.no_sort) - chain_and_reorder_extrusion_entities(extrusions, m_last_pos.to_point()); + chain_and_reorder_extrusion_entities(extrusions, m_last_pos.to_point(), reversed); for (const ExtrusionEntity *ee : extrusions) { ExtrusionRole role = ee->role(); @@ -10054,8 +10064,8 @@ const std::vector& GCode::ObjectByExtru // Now we are going to iterate through perimeters and infills and pick ones that are supposed to be printed // References are used so that we don't have to repeat the same code for (int iter = 0; iter < 2; ++iter) { - const ExtrusionEntitiesPtr& entities = (iter ? reg.infills : reg.perimeters); - ExtrusionEntitiesPtr& target_eec = (iter ? by_region_per_copy_cache.back().infills : by_region_per_copy_cache.back().perimeters); + const std::vector& entities = (iter ? reg.infills : reg.perimeters); + std::vector& target_eec = (iter ? by_region_per_copy_cache.back().infills : by_region_per_copy_cache.back().perimeters); const std::vector& overrides = (iter ? reg.infills_overrides : reg.perimeters_overrides); // Now the most important thing - which extrusion should we print. @@ -10090,7 +10100,7 @@ const std::vector& GCode::ObjectByExtru void GCode::ObjectByExtruder::Island::Region::append(const Type type, const ExtrusionEntityCollection* eec, const WipingExtrusions::ExtruderPerCopy* copies_extruder) { // We are going to manipulate either perimeters or infills, exactly in the same way. Let's create pointers to the proper structure to not repeat ourselves: - ExtrusionEntitiesPtr* perimeters_or_infills; + std::vector* perimeters_or_infills; std::vector* perimeters_or_infills_overrides; switch (type) { @@ -10114,7 +10124,7 @@ void GCode::ObjectByExtruder::Island::Region::append(const Type type, const Extr for (auto* ee : eec->entities) perimeters_or_infills->emplace_back(ee); } else - perimeters_or_infills->emplace_back(const_cast(eec)); + perimeters_or_infills->emplace_back(eec); if (copies_extruder != nullptr) { // Don't reallocate overrides if not needed. diff --git a/src/libslic3r/GCode.hpp b/src/libslic3r/GCode.hpp index 6516a3f6fd..29a8ae5a34 100644 --- a/src/libslic3r/GCode.hpp +++ b/src/libslic3r/GCode.hpp @@ -450,19 +450,19 @@ private: double &y_acceleration_limit_res, double &accumulated_mass_res); // Orca: pass the complete collection of region perimeters to the extrude loop to check whether the wipe before external loop // should be executed - std::string extrude_entity(const ExtrusionEntity& entity, - const std::string& description = "", - double speed = -1., - const ExtrusionEntitiesPtr& region_perimeters = ExtrusionEntitiesPtr(), - const WipeInwardSupport* wipe_support = nullptr); + std::string extrude_entity(const ExtrusionEntity& entity, + const std::string& description = "", + double speed = -1., + const std::vector& region_perimeters = {}, + const WipeInwardSupport* wipe_support = nullptr); // Orca: pass the complete collection of region perimeters to the extrude loop to check whether the wipe before external loop // should be executed - std::string extrude_loop(const ExtrusionLoop& loop, - const std::string& description, - double speed = -1., - const ExtrusionEntitiesPtr& region_perimeters = ExtrusionEntitiesPtr(), - const Point* start_point = nullptr, - const WipeInwardSupport* wipe_support = nullptr); + std::string extrude_loop(const ExtrusionLoop& loop, + const std::string& description, + double speed = -1., + const std::vector& region_perimeters = {}, + const Point* start_point = nullptr, + const WipeInwardSupport* wipe_support = nullptr); std::string extrude_multi_path(const ExtrusionMultiPath& multipath, const std::string& description = "", double speed = -1.); std::string extrude_path(const ExtrusionPath& path, const std::string& description = "", double speed = -1.); @@ -493,10 +493,9 @@ private: { struct Region { // Non-owned references to LayerRegion::perimeters::entities - // std::vector would be better here, but there is no way in C++ to convert from std::vector std::vector without copying. - ExtrusionEntitiesPtr perimeters; + std::vector perimeters; // Non-owned references to LayerRegion::fills::entities - ExtrusionEntitiesPtr infills; + std::vector infills; std::vector infills_overrides; std::vector perimeters_overrides; diff --git a/src/libslic3r/ShortestPath.cpp b/src/libslic3r/ShortestPath.cpp index eb3fc66927..08f973e20b 100644 --- a/src/libslic3r/ShortestPath.cpp +++ b/src/libslic3r/ShortestPath.cpp @@ -1024,13 +1024,14 @@ std::vector> chain_segments_greedy2(SegmentEndPointFunc return chain_segments_greedy_constrained_reversals2_(end_point_func, could_reverse_func, num_segments, start_near); } -std::vector> chain_extrusion_entities(std::vector &entities, const Point *start_near) +template +static std::vector> chain_extrusion_entities_impl(const std::vector &entities, const Point *start_near) { auto segment_end_point = [&entities](size_t idx, bool first_point) -> Point { return first_point ? entities[idx]->first_point() : entities[idx]->last_point(); }; auto could_reverse = [&entities](size_t idx) { const ExtrusionEntity *ee = entities[idx]; return ee->is_loop() || ee->can_reverse(); }; std::vector> out = chain_segments_greedy_constrained_reversals(segment_end_point, could_reverse, entities.size(), start_near); for (std::pair &segment : out) { - ExtrusionEntity *ee = entities[segment.first]; + const ExtrusionEntity *ee = entities[segment.first]; if (ee->is_loop()) // Ignore reversals for loops, as the start point equals the end point. segment.second = false; @@ -1040,6 +1041,20 @@ std::vector> chain_extrusion_entities(std::vector> chain_extrusion_entities(std::vector &entities, const Point *start_near) +{ + return chain_extrusion_entities_impl(entities, start_near); +} + +// Orca: Reordering queries first_point() / last_point(); drop entities that cannot provide valid endpoints. +template +static void remove_entities_without_endpoints(std::vector &entities) +{ + entities.erase(std::remove_if(entities.begin(), entities.end(), + [](const ExtrusionEntity *entity) { return !extrusion_entity_has_endpoints(entity); }), + entities.end()); +} + void reorder_extrusion_entities(std::vector &entities, const std::vector> &chain) { assert(entities.size() == chain.size()); @@ -1061,14 +1076,27 @@ void chain_and_reorder_extrusion_entities(std::vector &entitie void chain_and_reorder_extrusion_entities(std::vector &entities, const Point *start_near) { - // Orca: Reordering queries first_point() / last_point(); drop entities that cannot provide valid endpoints. - entities.erase(std::remove_if(entities.begin(), entities.end(), [](ExtrusionEntity *entity) { - return !extrusion_entity_has_endpoints(entity); - }), - entities.end()); + remove_entities_without_endpoints(entities); reorder_extrusion_entities(entities, chain_extrusion_entities(entities, start_near)); } +void chain_and_reorder_extrusion_entities(std::vector &entities, const Point &start_near, + std::vector> &reversed_clones) +{ + remove_entities_without_endpoints(entities); + std::vector out; + out.reserve(entities.size()); + for (const auto &[idx, reverse] : chain_extrusion_entities_impl(entities, &start_near)) { + if (reverse) { + ExtrusionEntity *clone = reversed_clones.emplace_back(entities[idx]->clone()).get(); + clone->reverse(); + out.emplace_back(clone); + } else + out.emplace_back(entities[idx]); + } + entities.swap(out); +} + std::vector> chain_extrusion_paths(std::vector &extrusion_paths, const Point *start_near) { auto segment_end_point = [&extrusion_paths](size_t idx, bool first_point) -> Point { return first_point ? extrusion_paths[idx].first_point() : extrusion_paths[idx].last_point(); }; diff --git a/src/libslic3r/ShortestPath.hpp b/src/libslic3r/ShortestPath.hpp index 5de7d8ef4a..c7a61d99ee 100644 --- a/src/libslic3r/ShortestPath.hpp +++ b/src/libslic3r/ShortestPath.hpp @@ -5,6 +5,7 @@ #include "ExtrusionEntity.hpp" #include "Point.hpp" +#include #include #include @@ -24,6 +25,9 @@ std::vector> chain_extrusion_entities(std::vector &entities, const std::vector> &chain); void chain_and_reorder_extrusion_entities(std::vector &entities, const Point &start_near); void chain_and_reorder_extrusion_entities(std::vector &entities, const Point *start_near = nullptr); +// Each entity the chain reverses is replaced by a reversed clone that reversed_clones owns, so the originals stay unchanged. +void chain_and_reorder_extrusion_entities(std::vector &entities, const Point &start_near, + std::vector> &reversed_clones); std::vector> chain_extrusion_paths(std::vector &extrusion_paths, const Point *start_near = nullptr); void reorder_extrusion_paths(std::vector &extrusion_paths, std::vector> &chain); diff --git a/tests/fff_print/test_helpers.cpp b/tests/fff_print/test_helpers.cpp index 64493d5f74..89f9f9fdd1 100644 --- a/tests/fff_print/test_helpers.cpp +++ b/tests/fff_print/test_helpers.cpp @@ -225,7 +225,7 @@ DynamicPrintConfig multifilament_config(unsigned int filaments, std::initializer } void init_print(std::vector &&meshes, Slic3r::Print &print, Slic3r::Model &model, const DynamicPrintConfig &config_in, - const std::vector> *per_object_overrides, bool arrange) + const std::vector> *per_object_overrides, bool arrange, size_t instances) { DynamicPrintConfig config = DynamicPrintConfig::full_print_config(); config.apply(config_in); @@ -236,7 +236,8 @@ void init_print(std::vector &&meshes, Slic3r::Print &print, Slic3r ModelObject *object = model.add_object(); object->name += "object.stl"; object->add_volume(std::move(t)); - object->add_instance(); + for (size_t i = 0; i < instances; ++i) + object->add_instance(); if (per_object_overrides && object_idx < per_object_overrides->size() && !(*per_object_overrides)[object_idx].empty()) { DynamicPrintConfig oc; diff --git a/tests/fff_print/test_helpers.hpp b/tests/fff_print/test_helpers.hpp index cb3069ed02..cbdc90c3d9 100644 --- a/tests/fff_print/test_helpers.hpp +++ b/tests/fff_print/test_helpers.hpp @@ -72,9 +72,11 @@ Slic3r::Model model(const std::string& model_name, TriangleMesh&& _mesh); DynamicPrintConfig multifilament_config(unsigned int filaments, std::initializer_list extra = {}); -// Apply `meshes` and config to `print`/`model`; optional per-object overrides, auto-arranged unless `arrange` is false. +// Apply `meshes` and config to `print`/`model`, each object with `instances` copies; optional per-object overrides, +// auto-arranged unless `arrange` is false. void init_print(std::vector &&meshes, Slic3r::Print &print, Slic3r::Model &model, const DynamicPrintConfig &config_in, - const std::vector> *per_object_overrides = nullptr, bool arrange = true); + const std::vector> *per_object_overrides = nullptr, bool arrange = true, + size_t instances = 1); void init_print(std::initializer_list meshes, Slic3r::Print &print, Slic3r::Model &model, const Slic3r::DynamicPrintConfig &config_in = Slic3r::DynamicPrintConfig::full_print_config()); void init_print(std::initializer_list meshes, Slic3r::Print &print, Slic3r::Model &model, const Slic3r::DynamicPrintConfig &config_in = Slic3r::DynamicPrintConfig::full_print_config()); void init_print(std::initializer_list meshes, Slic3r::Print &print, Slic3r::Model &model, std::initializer_list config_items); diff --git a/tests/fff_print/test_print.cpp b/tests/fff_print/test_print.cpp index a28669b8d4..4176256021 100644 --- a/tests/fff_print/test_print.cpp +++ b/tests/fff_print/test_print.cpp @@ -561,6 +561,40 @@ TEST_CASE("export_gcode writes G-code without a result pointer", "[Print][export REQUIRE_FALSE(gcode.empty()); } +TEST_CASE("Exporting a sliced print again gives the same G-code", "[Print][export_gcode][Regression]") +{ + const int instances = GENERATE(1, 3); + CAPTURE(instances); + DynamicPrintConfig config = DynamicPrintConfig::full_print_config(); + TestMesh mesh = TestMesh::ipadstand; + SECTION("infill reversed by chaining") { config.set_deserialize_strict({{"sparse_infill_pattern", "gyroid"}}); } + SECTION("support reversed by chaining") { + mesh = TestMesh::overhang; + config.set_deserialize_strict({{"enable_support", true}, {"support_interface_pattern", "concentric"}}); + } + Print print; + Model model; + Slic3r::Test::init_print({Slic3r::Test::mesh(mesh)}, print, model, config, nullptr, true, instances); + + const auto export_without_timestamp = [&print]() { + std::string gcode = Slic3r::Test::gcode(print); + const size_t line = gcode.find("; generated by "); + REQUIRE(line != std::string::npos); + gcode.erase(line, gcode.find('\n', line) - line); + return gcode; + }; + const std::string first = export_without_timestamp(); + const std::string second = export_without_timestamp(); + + // Shows the first differing line on failure. + const size_t diff = std::mismatch(first.begin(), first.end(), second.begin(), second.end()).first - first.begin(); + const size_t line_start = diff == 0 ? 0 : first.rfind('\n', diff - 1) + 1; + INFO("first export: " << first.substr(line_start, first.find('\n', diff) - line_start)); + INFO("second export: " << second.substr(line_start, second.find('\n', diff) - line_start)); + CHECK(diff == first.size()); + CHECK(first.size() == second.size()); +} + TEST_CASE("Sequential printing follows model order", "[Print]") { // Two objects of different heights, taller one added first. Orca prints