From a95e8ee70106403b290773509d3054392db4e97f Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Sun, 26 Jul 2026 08:57:54 +0200 Subject: [PATCH] Re-edit: list the bodies as of the feature's timeline slot, not the final ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by sweeping the index-space defect class deliberately rather than by hitting it: that class produced 4 of the 8 defects found by hand yesterday, so it was worth auditing every combo in the panel that maps a row selection onto a document index. Most of it came back clean — the loft sidecar vectors are consistent at all four read sites, the sheet-body pickers go through the helper everywhere, mate connectors carry client data. Six did not. A stored target_body indexes the body list AS IT WAS just before that feature ran during replay, but Transform, Mirror, Thicken, Rib, Project and DeleteFace all populated their combo from the live m_doc.bodies. Boolean and Cut already replayed to the right slot. The failure is concrete: model a body, Thicken it, then Cut something later in the tree. A Cut replaces one body with two, so every index at or after it shifts. Reopen the Thicken and the combo lists the post-cut bodies while selecting the pre-cut index — showing, and on confirm re-targeting, a different body than the feature actually used. A Boolean that consumes its tool body shifts them the other way for the same result. fill_body_choice() does the truncated replay populate_body_choices() already did, for the single-combo tools. Six call sites, and 60 lines of duplicated population loops go with them. Visible change when testing: re-editing an early feature now lists FEWER bodies, because it lists only those that existed then. That is correct — you cannot target a body that did not exist yet — and it is what Boolean and Cut have always done. The new kernel test pins the invariant the GUI now leans on: a Cut turns one body into two, and replaying to just before it yields the earlier, shorter list. If body ordering after a split ever changes, that assumption fails loudly here instead of silently in a dialog. NOT click-tested — GUI wiring, compile-verified only. Filed as snaporca-oz7 and added to snaporca-cfi. Co-Authored-By: Claude Opus 5 (1M context) --- src/slic3r/GUI/DesignPanel.cpp | 90 ++++++++++------------------ src/slic3r/GUI/DesignPanel.hpp | 4 ++ tests/libslic3r/test_caddocument.cpp | 36 +++++++++++ 3 files changed, 70 insertions(+), 60 deletions(-) diff --git a/src/slic3r/GUI/DesignPanel.cpp b/src/slic3r/GUI/DesignPanel.cpp index 2f7a49963b..85104c683e 100644 --- a/src/slic3r/GUI/DesignPanel.cpp +++ b/src/slic3r/GUI/DesignPanel.cpp @@ -4693,6 +4693,30 @@ void DesignPanel::populate_body_choices(int as_of_feature) fill(m_cut_target, 0); // Cut tool: default to the first body } +void DesignPanel::fill_body_choice(ComboBox* c, int as_of_feature, int want) +{ + if (!c) return; + // Same replay as populate_body_choices, for the single-combo tools. A stored target_body + // indexes the body list AS IT WAS just before that feature ran; listing the final bodies + // instead makes the saved index select whatever now sits at that position, which is a + // different body as soon as a later Cut splits one (indices shift up) or a later Boolean + // consumes its tool body (indices shift down). + const std::vector* src = &m_doc.bodies; + std::vector as_of; + if (as_of_feature >= 0 && as_of_feature <= int(m_doc.features.size())) { + CadDocument tmp = m_doc; + tmp.features.resize(as_of_feature); + if (tmp.recompute() && !tmp.bodies.empty()) { as_of = tmp.bodies; src = &as_of; } + } + c->Clear(); + for (size_t i = 0; i < src->size(); ++i) { + const std::string& n = (*src)[i].name; + c->Append(n.empty() ? wxString::Format(_L("Body %zu"), i + 1) : wxString::FromUTF8(n)); + } + if (want >= 0 && want < int(c->GetCount())) c->SetSelection(want); + else if (c->GetCount() > 0) c->SetSelection(0); +} + void DesignPanel::on_add_boolean() { if (m_doc.bodies.size() < 2) { @@ -7006,16 +7030,7 @@ void DesignPanel::load_feature_into_dialog(const CadFeature& f) select_sheet_choice(m_surf_thicken_body, f.target_body); break; case CadFeatureType::Transform: { - { - m_xf_body->Clear(); - for (size_t i = 0; i < m_doc.bodies.size(); ++i) { - const std::string& n = m_doc.bodies[i].name; - m_xf_body->Append(n.empty() ? wxString::Format(_L("Body %zu"), i + 1) : wxString::FromUTF8(n)); - } - if (f.target_body >= 0 && f.target_body < int(m_xf_body->GetCount())) - m_xf_body->SetSelection(f.target_body); - else if (m_xf_body->GetCount() > 0) m_xf_body->SetSelection(0); - } + fill_body_choice(m_xf_body, m_edit_index, f.target_body); m_xf_dx->SetValue(f.xf_translate.x()); m_xf_dy->SetValue(f.xf_translate.y()); m_xf_dz->SetValue(f.xf_translate.z()); @@ -7029,32 +7044,14 @@ void DesignPanel::load_feature_into_dialog(const CadFeature& f) break; } case CadFeatureType::Mirror: { - { - m_mirror_body->Clear(); - for (size_t i = 0; i < m_doc.bodies.size(); ++i) { - const std::string& n = m_doc.bodies[i].name; - m_mirror_body->Append(n.empty() ? wxString::Format(_L("Body %zu"), i + 1) : wxString::FromUTF8(n)); - } - if (f.target_body >= 0 && f.target_body < int(m_mirror_body->GetCount())) - m_mirror_body->SetSelection(f.target_body); - else if (m_mirror_body->GetCount() > 0) m_mirror_body->SetSelection(0); - } + fill_body_choice(m_mirror_body, m_edit_index, f.target_body); populate_plane_choices(m_mirror_plane); m_mirror_plane->SetSelection(index_from_plane(f.plane)); m_mirror_keep->SetValue(f.mirror_keep_original); break; } case CadFeatureType::Thicken: { - { - m_thicken_body->Clear(); - for (size_t i = 0; i < m_doc.bodies.size(); ++i) { - const std::string& n = m_doc.bodies[i].name; - m_thicken_body->Append(n.empty() ? wxString::Format(_L("Body %zu"), i + 1) : wxString::FromUTF8(n)); - } - if (f.target_body >= 0 && f.target_body < int(m_thicken_body->GetCount())) - m_thicken_body->SetSelection(f.target_body); - else if (m_thicken_body->GetCount() > 0) m_thicken_body->SetSelection(0); - } + fill_body_choice(m_thicken_body, m_edit_index, f.target_body); m_sel_solid_face = f.thicken_face; m_thicken_face_label->SetLabel(f.thicken_face >= 0 ? wxString::Format(_L("Face %d"), f.thicken_face) @@ -7064,16 +7061,7 @@ void DesignPanel::load_feature_into_dialog(const CadFeature& f) break; } case CadFeatureType::Rib: { - { - m_rib_body->Clear(); - for (size_t i = 0; i < m_doc.bodies.size(); ++i) { - const std::string& n = m_doc.bodies[i].name; - m_rib_body->Append(n.empty() ? wxString::Format(_L("Body %zu"), i + 1) : wxString::FromUTF8(n)); - } - if (f.target_body >= 0 && f.target_body < int(m_rib_body->GetCount())) - m_rib_body->SetSelection(f.target_body); - else if (m_rib_body->GetCount() > 0) m_rib_body->SetSelection(0); - } + fill_body_choice(m_rib_body, m_edit_index, f.target_body); { m_rib_sketch->Clear(); int pre_sel = wxNOT_FOUND; @@ -7093,16 +7081,7 @@ void DesignPanel::load_feature_into_dialog(const CadFeature& f) break; } case CadFeatureType::Project: { - { - m_proj_source_body->Clear(); - for (size_t i = 0; i < m_doc.bodies.size(); ++i) { - const std::string& n = m_doc.bodies[i].name; - m_proj_source_body->Append(n.empty() ? wxString::Format(_L("Body %zu"), i + 1) : wxString::FromUTF8(n)); - } - if (f.project_source_body >= 0 && f.project_source_body < int(m_proj_source_body->GetCount())) - m_proj_source_body->SetSelection(f.project_source_body); - else if (m_proj_source_body->GetCount() > 0) m_proj_source_body->SetSelection(0); - } + fill_body_choice(m_proj_source_body, m_edit_index, f.project_source_body); populate_plane_choices(m_proj_plane); m_proj_plane->SetSelection(index_from_plane(f.plane)); m_sel_solid_face = f.project_face; @@ -7112,16 +7091,7 @@ void DesignPanel::load_feature_into_dialog(const CadFeature& f) break; } case CadFeatureType::DeleteFace: { - { - m_del_face_body->Clear(); - for (size_t i = 0; i < m_doc.bodies.size(); ++i) { - const std::string& n = m_doc.bodies[i].name; - m_del_face_body->Append(n.empty() ? wxString::Format(_L("Body %zu"), i + 1) : wxString::FromUTF8(n)); - } - if (f.target_body >= 0 && f.target_body < int(m_del_face_body->GetCount())) - m_del_face_body->SetSelection(f.target_body); - else if (m_del_face_body->GetCount() > 0) m_del_face_body->SetSelection(0); - } + fill_body_choice(m_del_face_body, m_edit_index, f.target_body); m_del_faces = f.delete_faces; { wxString s; diff --git a/src/slic3r/GUI/DesignPanel.hpp b/src/slic3r/GUI/DesignPanel.hpp index c95985e048..1e78fd1403 100644 --- a/src/slic3r/GUI/DesignPanel.hpp +++ b/src/slic3r/GUI/DesignPanel.hpp @@ -122,6 +122,10 @@ private: // >= 0 = the bodies as they existed just before that feature index (Boolean re-edit, so a // consumed tool body still appears and its saved selection round-trips). void populate_body_choices(int as_of_feature = -1); + // Fill `c` with the bodies as they existed just before `as_of_feature` and select + // `want`. Re-editing any feature that stores a body index needs this: the index was + // recorded against the body list at that point in the timeline, not the final one. + void fill_body_choice(ComboBox* c, int as_of_feature, int want); void populate_sheet_body_choices(ComboBox* c) const; // bodies where is_sheet_shape() is true // Rows of a sheet-filtered picker are not body indices; go through these two, never // GetSelection()/SetSelection() directly. diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index b9b94d8041..ca2a4e3204 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -1408,6 +1408,42 @@ TEST_CASE("draft tapers a solid face about the body base", "[CadDocument]") REQUIRE_FALSE(bad.recompute()); } +TEST_CASE("a split renumbers the bodies a later feature indexes", "[CadDocument][cut]") +{ + // Pins the invariant the Design tab's re-edit path depends on (snaporca-oz7): a stored + // target_body indexes the body list AS IT WAS when that feature ran, and a Cut placed + // later in the tree changes that list. If this test ever fails, the GUI's + // fill_body_choice() replay-to-timeline-slot assumption needs revisiting with it. + using Catch::Matchers::WithinRel; + + CadDocument doc; + int sk = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 20, 20, 10, "Box"); + doc.add_extrude(sk, 20.0, false, BooleanMode::New, "Ext"); + REQUIRE(doc.recompute()); + REQUIRE(doc.bodies.size() == 1); + + // Cut the single body in half, keeping BOTH pieces — what the Cut card always does. + doc.add_cut(SketchPlane::XY(), 10.0, false, true, true, 0, "Split"); + REQUIRE(doc.recompute()); + REQUIRE(doc.error.empty()); + REQUIRE(doc.bodies.size() == 2); // one body became two + + // Body index 1 did not exist before the Cut. A feature recorded BEFORE the Cut could + // never have referred to it, which is exactly why re-edit must list the earlier set. + const double half = 20.0 * 20.0 * 10.0; + double v0 = double(SketchEngine::tessellate(doc.bodies[0].shape).volume()); + double v1 = double(SketchEngine::tessellate(doc.bodies[1].shape).volume()); + CHECK_THAT(v0 + v1, WithinRel(2.0 * half, 1e-3)); + + // Replaying to just before the Cut yields the pre-split list — the one a feature sitting + // there indexes into. This is the operation fill_body_choice() performs. + CadDocument as_of = doc; + as_of.features.resize(as_of.features.size() - 1); // drop the Cut + REQUIRE(as_of.recompute()); + REQUIRE(as_of.bodies.size() == 1); + CHECK(as_of.bodies.size() < doc.bodies.size()); +} + TEST_CASE("cut splits a body with a plane", "[cut]") { using Catch::Matchers::WithinRel;