juli/OrcaSlicer
1
1
Fork 0
mirror of https://github.com/OrcaSlicer/OrcaSlicer.git synced 2026-09-18 14:32:36 +00:00
Code Issues Packages Projects Releases Wiki Activity

Re-edit: list the bodies as of the feature's timeline slot, not the final ones

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Tommaso Bianchi
2026-07-26 09:32:47 +02:00
co-authored by Claude Opus 5
parent 8f06b9dfd8
commit a95e8ee701
3 changed files with 70 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/slic3r/GUI/DesignPanel.cpp
+30 -60
View File
@@ -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<CadBody>* src = &m_doc.bodies;
std::vector<CadBody> 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;
src/slic3r/GUI/DesignPanel.hpp
+4
View File
@@ -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.
tests/libslic3r/test_caddocument.cpp
+36
View File
@@ -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;