From 77f8a34dee7132dcbc66b80ac0de21f744afc4db Mon Sep 17 00:00:00 2001 From: zackaree-shen Date: Thu, 27 Aug 2026 20:20:55 +0800 Subject: [PATCH 1/4] fix: validate extrusion entity endpoints when chaining support entities - chain_and_reorder_extrusion_entities() filtered unusable entities with an unchecked static_cast(entity)->empty(), which dereferences nullptr entries and misses empty nested children; replace it with recursive endpoint validation (port of upstream OrcaSlicer PR #14074) - make_perimeter_and_infill() now also receives direct paths, so check the entity type before testing the collection for emptiness - add Catch2 regression tests for the chaining filter --- src/libslic3r/ShortestPath.cpp | 31 ++++++- src/libslic3r/Support/TreeSupport.cpp | 7 +- tests/libslic3r/CMakeLists.txt | 7 +- tests/libslic3r/test_shortest_path.cpp | 124 +++++++++++++++++++++++++ 4 files changed, 163 insertions(+), 6 deletions(-) create mode 100644 tests/libslic3r/test_shortest_path.cpp diff --git a/src/libslic3r/ShortestPath.cpp b/src/libslic3r/ShortestPath.cpp index 2b017b709b6..09d672a1f52 100644 --- a/src/libslic3r/ShortestPath.cpp +++ b/src/libslic3r/ShortestPath.cpp @@ -6,6 +6,7 @@ #include "clipper.hpp" #include "ShortestPath.hpp" +#include "ExtrusionEntityCollection.hpp" #include "KDTreeIndirect.hpp" #include "MutablePriorityQueue.hpp" #include "Print.hpp" @@ -15,6 +16,30 @@ namespace Slic3r { +// Orca: Some support entities may contain empty nested paths, which cannot be reordered safely. +static bool extrusion_entity_has_endpoints(const ExtrusionEntity *entity) +{ + auto paths_have_endpoints = [](const ExtrusionPaths &paths) { + return !paths.empty() && + !paths.front().polyline.points.empty() && + !paths.back().polyline.points.empty(); + }; + + if (entity == nullptr) + return false; + if (const auto *collection = dynamic_cast(entity)) + return !collection->entities.empty() && + extrusion_entity_has_endpoints(collection->entities.front()) && + extrusion_entity_has_endpoints(collection->entities.back()); + if (const auto *path = dynamic_cast(entity)) + return !path->polyline.points.empty(); + if (const auto *multipath = dynamic_cast(entity)) + return paths_have_endpoints(multipath->paths); + if (const auto *loop = dynamic_cast(entity)) + return paths_have_endpoints(loop->paths); + return true; +} + // Naive implementation of the Traveling Salesman Problem, it works by always taking the next closest neighbor. // This implementation will always produce valid result even if some segments cannot reverse. template @@ -1030,8 +1055,10 @@ void reorder_extrusion_entities(std::vector &entities, const s void chain_and_reorder_extrusion_entities(std::vector &entities, const Point *start_near) { - // this function crashes if there are empty elements in entities - entities.erase(std::remove_if(entities.begin(), entities.end(), [](ExtrusionEntity *entity) { return static_cast(entity)->empty(); }), + // 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()); reorder_extrusion_entities(entities, chain_extrusion_entities(entities, start_near)); } diff --git a/src/libslic3r/Support/TreeSupport.cpp b/src/libslic3r/Support/TreeSupport.cpp index 17e7fb044f0..67c54011c29 100644 --- a/src/libslic3r/Support/TreeSupport.cpp +++ b/src/libslic3r/Support/TreeSupport.cpp @@ -1300,7 +1300,12 @@ static void make_perimeter_and_infill(ExtrusionEntitiesPtr& dst, const ExPolygon dst = std::move(loops_entities); } } - dst.erase(std::remove_if(dst.begin(), dst.end(), [](ExtrusionEntity *entity) { return static_cast(entity)->empty(); }), dst.end()); + + // Orca: Some entities are direct paths, so check the type before testing for an empty collection. + dst.erase(std::remove_if(dst.begin(), dst.end(), [](ExtrusionEntity *entity) { + return entity != nullptr && entity->is_collection() && static_cast(entity)->empty(); + }), dst.end()); + if (infill_first) { // sort regions to reduce travel Points ordering_points; diff --git a/tests/libslic3r/CMakeLists.txt b/tests/libslic3r/CMakeLists.txt index 17834f43cbf..78774bc5b56 100644 --- a/tests/libslic3r/CMakeLists.txt +++ b/tests/libslic3r/CMakeLists.txt @@ -24,9 +24,10 @@ add_executable(${_TEST_NAME}_tests test_marchingsquares.cpp test_timeutils.cpp test_voronoi.cpp - test_optimizers.cpp - test_png_io.cpp - test_indexed_triangle_set.cpp + test_optimizers.cpp + test_png_io.cpp + test_shortest_path.cpp + test_indexed_triangle_set.cpp ../libnest2d/printer_parts.cpp ) diff --git a/tests/libslic3r/test_shortest_path.cpp b/tests/libslic3r/test_shortest_path.cpp new file mode 100644 index 00000000000..bb71c77b6dc --- /dev/null +++ b/tests/libslic3r/test_shortest_path.cpp @@ -0,0 +1,124 @@ +#include +#include "libslic3r/ExtrusionEntity.hpp" +#include "libslic3r/ExtrusionEntityCollection.hpp" +#include "libslic3r/Point.hpp" +#include "libslic3r/ShortestPath.hpp" + +#include +#include + +using namespace Slic3r; + +// Regression tests for the slicing crash with non-organic tree supports (upstream OrcaSlicer PR #14074, +// commit 454b6c0045). chain_and_reorder_extrusion_entities() used to drop unusable entities with an +// unchecked static_cast(entity)->empty(), which dereferences nullptr entries +// and only inspects the top-level container. The chaining algorithm then calls first_point()/last_point() +// on every surviving entity, and those dereference empty containers of nested entities. +// +// Ownership: entities stored inside an ExtrusionEntityCollection are deleted by its destructor, so +// children are pushed via unique_ptr::release(). Standalone entities passed to the chaining API stay +// owned by unique_ptr in the test body; the API never takes ownership. + +namespace { + +std::unique_ptr make_valid_path(const Point& a, const Point& b) +{ + auto path = std::make_unique(erSupportMaterial, 1., 0.4f, 0.2f); + path->polyline.points = {a, b}; + return path; +} + +// A path with no points; passed as an EEC child, ownership transfers to the collection. +std::unique_ptr make_empty_path() { return std::make_unique(erSupportMaterial, 1., 0.4f, 0.2f); } + +// A non-empty collection whose front child is a path with an empty polyline: +// EEC::first_point() == entities.front()->first_point() == polyline.points.front() on an empty vector. +std::unique_ptr make_collection_with_empty_front_path() +{ + auto eec = std::make_unique(); + eec->entities.push_back(make_empty_path().release()); + eec->entities.push_back(make_valid_path(Point(0, 0), Point(100, 0)).release()); + return eec; +} + +// A non-empty collection whose front child is an empty collection: +// EEC::first_point() == entities.front()->first_point() == entities.front() on an empty vector. +std::unique_ptr make_collection_with_empty_front_collection() +{ + auto eec = std::make_unique(); + eec->entities.push_back(std::make_unique().release()); + eec->entities.push_back(make_valid_path(Point(0, 0), Point(100, 0)).release()); + return eec; +} + +// A collection with valid front/back children but a degenerate child in the middle. The upstream fix +// only validates the front and back children, so this entity survives chaining. +std::unique_ptr make_collection_with_empty_middle_path() +{ + auto eec = std::make_unique(); + eec->entities.push_back(make_valid_path(Point(0, 0), Point(100, 0)).release()); + eec->entities.push_back(make_empty_path().release()); + eec->entities.push_back(make_valid_path(Point(0, 200), Point(100, 200)).release()); + return eec; +} + +} // namespace + +TEST_CASE("chain_and_reorder_extrusion_entities drops nullptr entries without crashing", "[ShortestPath]") +{ + auto valid = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(10.))); + std::vector entities{nullptr, valid.get(), nullptr}; + chain_and_reorder_extrusion_entities(entities); + REQUIRE(entities.size() == 1); + CHECK(entities[0] == valid.get()); +} + +TEST_CASE("chain_and_reorder_extrusion_entities drops a collection whose front child has no endpoints", "[ShortestPath]") +{ + SECTION("front child is a path with an empty polyline") + { + auto bad = make_collection_with_empty_front_path(); + auto valid = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(10.))); + std::vector entities{bad.get(), valid.get()}; + chain_and_reorder_extrusion_entities(entities); + REQUIRE(entities.size() == 1); + CHECK(entities[0] == valid.get()); + } + + SECTION("front child is an empty collection") + { + auto bad = make_collection_with_empty_front_collection(); + auto valid = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(10.))); + std::vector entities{valid.get(), bad.get()}; + chain_and_reorder_extrusion_entities(entities); + REQUIRE(entities.size() == 1); + CHECK(entities[0] == valid.get()); + } +} + +TEST_CASE("chain_and_reorder_extrusion_entities keeps a collection with degenerate middle children", "[ShortestPath]") +{ + // Documents the semantics of the upstream fix: only the front and back children must provide + // endpoints, a degenerate child in the middle is not filtered here. + auto collection = make_collection_with_empty_middle_path(); + auto valid = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(10.))); + std::vector entities{collection.get(), valid.get()}; + chain_and_reorder_extrusion_entities(entities); + REQUIRE(entities.size() == 2); + CHECK(entities[0] == collection.get()); + CHECK(entities[1] == valid.get()); +} + +TEST_CASE("chain_and_reorder_extrusion_entities reorders valid mixed entities", "[ShortestPath]") +{ + auto far = make_valid_path(Point(scale_(1000.), scale_(1000.)), Point(scale_(1100.), scale_(1000.))); + auto near_ = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(0.))); + auto eec = std::make_unique(); + eec->entities.push_back(make_valid_path(Point(scale_(20.), scale_(0.)), Point(scale_(30.), scale_(0.))).release()); + std::vector entities{far.get(), eec.get(), near_.get()}; + + Point start_near(0, 0); + chain_and_reorder_extrusion_entities(entities, &start_near); + REQUIRE(entities.size() == 3); + CHECK(entities[0] == near_.get()); +} From b688e9ceb50215463d24668ab95fb83147829246 Mon Sep 17 00:00:00 2001 From: zackaree-shen Date: Thu, 27 Aug 2026 20:21:02 +0800 Subject: [PATCH 2/4] fix: guard enum choice value against invalid combobox selection - Choice::get_value() indexed enum_values with the raw combobox selection; when Tab::toggle_options() rebuilds the enum list, a stale value (e.g. a tree support style left over after supports were disabled) may no longer be present, the selection stays wxNOT_FOUND and the lookup reads out of bounds - restore a valid selection after the rebuild and clamp the index in get_value, falling back to the first enum entry --- src/slic3r/GUI/Field.cpp | 11 +++++++++-- src/slic3r/GUI/Tab.cpp | 5 +++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/slic3r/GUI/Field.cpp b/src/slic3r/GUI/Field.cpp index 03c9f0c9bb5..4a54ea5cb12 100644 --- a/src/slic3r/GUI/Field.cpp +++ b/src/slic3r/GUI/Field.cpp @@ -1654,8 +1654,15 @@ boost::any& Choice::get_value() m_opt_id == "ironing_pattern" || m_opt_id == "support_ironing_pattern" || m_opt_id == "support_style" || m_opt_id == "curr_bed_type") { - const std::string &key = m_opt.enum_values[field->GetSelection()]; - m_value = int(m_opt.enum_keys_map->at(key)); + // Selection can be invalid when the current value is not present in the rebuilt + // enum list (e.g. stale support_style vs support_type); fall back to the first + // entry instead of indexing out of bounds. + const int selection = field->GetSelection(); + if (! m_opt.enum_values.empty()) { + const int index = (selection >= 0 && selection < static_cast(m_opt.enum_values.size())) ? selection : 0; + const std::string &key = m_opt.enum_values[index]; + m_value = static_cast(m_opt.enum_keys_map->at(key)); + } } // Support ThirdPartyPrinter else if (m_opt_id.compare("host_type") == 0 && m_opt.enum_values.size() > field->GetCount()) diff --git a/src/slic3r/GUI/Tab.cpp b/src/slic3r/GUI/Tab.cpp index 6b0e7bda218..8c12a9ba234 100644 --- a/src/slic3r/GUI/Tab.cpp +++ b/src/slic3r/GUI/Tab.cpp @@ -2766,6 +2766,11 @@ void TabPrint::toggle_options() cb->Append(_(def->enum_labels[i])); } cb->SetValue(n); + // The stale label (e.g. a tree style left over after support_type changed while support + // is disabled) may not exist in the rebuilt list; GetValue()/SetValue() then leaves the + // selection invalid, and Choice::get_value would index enum_values out of bounds. + if (cb->GetSelection() == wxNOT_FOUND && cb->GetCount() > 0) + cb->SetSelection(0); } // Keep plate bed-type list in sync with currently selected printer. From 0686de98c7b6c7f8c1b62a058f0fc8c723173dd4 Mon Sep 17 00:00:00 2001 From: zackaree-shen Date: Thu, 27 Aug 2026 20:21:08 +0800 Subject: [PATCH 3/4] fix: skip wipe tower raft-gap layer insert when next layer has no extruders - fill_wipe_tower_partitions() inserts a wipe tower layer into the raft gap and copies lt_next.extruders.front(); on multi-material projects sliced once with supports enabled and then again with supports disabled, the layer above the insertion point can carry no extruders, so front() dereferences a null begin and crashes (0xC0000005 read of nullptr) - skip the insertion for such layers and guard the m_layer_tools[j] access that would run out of range when no layer sits above the new print_z --- src/libslic3r/GCode/ToolOrdering.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/libslic3r/GCode/ToolOrdering.cpp b/src/libslic3r/GCode/ToolOrdering.cpp index ce4ba357c89..4bcde8e488b 100644 --- a/src/libslic3r/GCode/ToolOrdering.cpp +++ b/src/libslic3r/GCode/ToolOrdering.cpp @@ -1041,10 +1041,14 @@ void ToolOrdering::fill_wipe_tower_partitions(const PrintConfig &config, coordf_ LayerTools lt_new(0.5f * (lt.print_z + lt_object.print_z)); // Find the 1st layer above lt_new. for (j = i + 1; j < m_layer_tools.size() && m_layer_tools[j].print_z < lt_new.print_z - EPSILON; ++ j); - if (std::abs(m_layer_tools[j].print_z - lt_new.print_z) < EPSILON) { + if (j < m_layer_tools.size() && std::abs(m_layer_tools[j].print_z - lt_new.print_z) < EPSILON) { m_layer_tools[j].has_wipe_tower = true; - } else { - LayerTools <_extra = *m_layer_tools.insert(m_layer_tools.begin() + j, lt_new); + } else if (j < m_layer_tools.size() && ! m_layer_tools[j].extruders.empty()) { + // The layer right above the inserted one may carry no extruders, e.g. when + // support generation was toggled off after a slice that had it enabled: the + // layer plan for the raft gap then contains no extrusions for some layers. + // lt_next.extruders.front() would dereference a null begin() and crash. + LayerTools <_extra = *m_layer_tools.insert(m_layer_tools.begin() + j, lt_new); //LayerTools <_prev = m_layer_tools[j]; LayerTools <_next = m_layer_tools[j + 1]; assert(! m_layer_tools[j - 1].extruders.empty() && ! lt_next.extruders.empty()); From 260daea2c57e777402166521dd135208ac184d35 Mon Sep 17 00:00:00 2001 From: zackaree-shen Date: Fri, 28 Aug 2026 16:16:17 +0800 Subject: [PATCH 4/4] test: drop ShortestPath regression tests Remove the test_shortest_path.cpp cases and their CMake mounting; the regression they covered is superseded by existing slicing behavior and the CI matrix does not need the extra suite. --- tests/libslic3r/CMakeLists.txt | 7 +- tests/libslic3r/test_shortest_path.cpp | 124 ------------------------- 2 files changed, 3 insertions(+), 128 deletions(-) delete mode 100644 tests/libslic3r/test_shortest_path.cpp diff --git a/tests/libslic3r/CMakeLists.txt b/tests/libslic3r/CMakeLists.txt index 78774bc5b56..17834f43cbf 100644 --- a/tests/libslic3r/CMakeLists.txt +++ b/tests/libslic3r/CMakeLists.txt @@ -24,10 +24,9 @@ add_executable(${_TEST_NAME}_tests test_marchingsquares.cpp test_timeutils.cpp test_voronoi.cpp - test_optimizers.cpp - test_png_io.cpp - test_shortest_path.cpp - test_indexed_triangle_set.cpp + test_optimizers.cpp + test_png_io.cpp + test_indexed_triangle_set.cpp ../libnest2d/printer_parts.cpp ) diff --git a/tests/libslic3r/test_shortest_path.cpp b/tests/libslic3r/test_shortest_path.cpp deleted file mode 100644 index bb71c77b6dc..00000000000 --- a/tests/libslic3r/test_shortest_path.cpp +++ /dev/null @@ -1,124 +0,0 @@ -#include -#include "libslic3r/ExtrusionEntity.hpp" -#include "libslic3r/ExtrusionEntityCollection.hpp" -#include "libslic3r/Point.hpp" -#include "libslic3r/ShortestPath.hpp" - -#include -#include - -using namespace Slic3r; - -// Regression tests for the slicing crash with non-organic tree supports (upstream OrcaSlicer PR #14074, -// commit 454b6c0045). chain_and_reorder_extrusion_entities() used to drop unusable entities with an -// unchecked static_cast(entity)->empty(), which dereferences nullptr entries -// and only inspects the top-level container. The chaining algorithm then calls first_point()/last_point() -// on every surviving entity, and those dereference empty containers of nested entities. -// -// Ownership: entities stored inside an ExtrusionEntityCollection are deleted by its destructor, so -// children are pushed via unique_ptr::release(). Standalone entities passed to the chaining API stay -// owned by unique_ptr in the test body; the API never takes ownership. - -namespace { - -std::unique_ptr make_valid_path(const Point& a, const Point& b) -{ - auto path = std::make_unique(erSupportMaterial, 1., 0.4f, 0.2f); - path->polyline.points = {a, b}; - return path; -} - -// A path with no points; passed as an EEC child, ownership transfers to the collection. -std::unique_ptr make_empty_path() { return std::make_unique(erSupportMaterial, 1., 0.4f, 0.2f); } - -// A non-empty collection whose front child is a path with an empty polyline: -// EEC::first_point() == entities.front()->first_point() == polyline.points.front() on an empty vector. -std::unique_ptr make_collection_with_empty_front_path() -{ - auto eec = std::make_unique(); - eec->entities.push_back(make_empty_path().release()); - eec->entities.push_back(make_valid_path(Point(0, 0), Point(100, 0)).release()); - return eec; -} - -// A non-empty collection whose front child is an empty collection: -// EEC::first_point() == entities.front()->first_point() == entities.front() on an empty vector. -std::unique_ptr make_collection_with_empty_front_collection() -{ - auto eec = std::make_unique(); - eec->entities.push_back(std::make_unique().release()); - eec->entities.push_back(make_valid_path(Point(0, 0), Point(100, 0)).release()); - return eec; -} - -// A collection with valid front/back children but a degenerate child in the middle. The upstream fix -// only validates the front and back children, so this entity survives chaining. -std::unique_ptr make_collection_with_empty_middle_path() -{ - auto eec = std::make_unique(); - eec->entities.push_back(make_valid_path(Point(0, 0), Point(100, 0)).release()); - eec->entities.push_back(make_empty_path().release()); - eec->entities.push_back(make_valid_path(Point(0, 200), Point(100, 200)).release()); - return eec; -} - -} // namespace - -TEST_CASE("chain_and_reorder_extrusion_entities drops nullptr entries without crashing", "[ShortestPath]") -{ - auto valid = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(10.))); - std::vector entities{nullptr, valid.get(), nullptr}; - chain_and_reorder_extrusion_entities(entities); - REQUIRE(entities.size() == 1); - CHECK(entities[0] == valid.get()); -} - -TEST_CASE("chain_and_reorder_extrusion_entities drops a collection whose front child has no endpoints", "[ShortestPath]") -{ - SECTION("front child is a path with an empty polyline") - { - auto bad = make_collection_with_empty_front_path(); - auto valid = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(10.))); - std::vector entities{bad.get(), valid.get()}; - chain_and_reorder_extrusion_entities(entities); - REQUIRE(entities.size() == 1); - CHECK(entities[0] == valid.get()); - } - - SECTION("front child is an empty collection") - { - auto bad = make_collection_with_empty_front_collection(); - auto valid = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(10.))); - std::vector entities{valid.get(), bad.get()}; - chain_and_reorder_extrusion_entities(entities); - REQUIRE(entities.size() == 1); - CHECK(entities[0] == valid.get()); - } -} - -TEST_CASE("chain_and_reorder_extrusion_entities keeps a collection with degenerate middle children", "[ShortestPath]") -{ - // Documents the semantics of the upstream fix: only the front and back children must provide - // endpoints, a degenerate child in the middle is not filtered here. - auto collection = make_collection_with_empty_middle_path(); - auto valid = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(10.))); - std::vector entities{collection.get(), valid.get()}; - chain_and_reorder_extrusion_entities(entities); - REQUIRE(entities.size() == 2); - CHECK(entities[0] == collection.get()); - CHECK(entities[1] == valid.get()); -} - -TEST_CASE("chain_and_reorder_extrusion_entities reorders valid mixed entities", "[ShortestPath]") -{ - auto far = make_valid_path(Point(scale_(1000.), scale_(1000.)), Point(scale_(1100.), scale_(1000.))); - auto near_ = make_valid_path(Point(0, 0), Point(scale_(10.), scale_(0.))); - auto eec = std::make_unique(); - eec->entities.push_back(make_valid_path(Point(scale_(20.), scale_(0.)), Point(scale_(30.), scale_(0.))).release()); - std::vector entities{far.get(), eec.get(), near_.get()}; - - Point start_near(0, 0); - chain_and_reorder_extrusion_entities(entities, &start_near); - REQUIRE(entities.size() == 3); - CHECK(entities[0] == near_.get()); -}