From 6e7f6429fa7e91aaeb9fd734a3131561823a625e Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Sun, 23 Aug 2026 19:09:17 +0200 Subject: [PATCH] A body carries its own name, because a body is not its first feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User report 2026-08-23, and it is right: "you have renamed the feature extrusion, not the body. i clicked rename on the body feature tree and the feature extrude changed name. this means that you consider the extrusion = the body. this is very far from truth as a body can contain several extrusions." That is exactly what the previous commit did. It resolved a selected body to CadBody::source_feature and renamed THAT feature, on the reasoning that a body has no name of its own. The reasoning described an implementation detail — CadBody:: name is derived and restamped on every recompute — and mistook it for the user's model. An Extrude, a Cut and a Fillet all land on the same body: the maker is one operation in its history, and renaming it renames the wrong object. A body now has a name of its own. CadBody::user_name, set only by a rename, is: - carried across recompute() by body index, next to the per-body colour override and under the same index contract the GUI already relies on for visibility and Move — without which a name would survive exactly until the next feature; - written into the recipe, because bodies are recomputed and never serialised, so a name has nowhere else to live and would otherwise vanish on reopen; - shown on the Bodies row ahead of the derived maker name, as "Body N — name", with the number still leading because every status line, the interference report and the mate errors identify a body that way. The recipe block is APPENDED after the variables block rather than given a version bump. A build that predates it reads features and variables, returns, and never looks at the trailing bytes — so yesterday's projects open here and today's projects still open there. A bump would have cost every project written today its readability by the previous build, for one optional field. Renaming a FEATURE is unchanged. The feature tree renames features; the Bodies list renames bodies; neither reaches into the other. WHAT THIS COST, and why it is written down. Getting here took two wrong turns inside one fix, both mine: 1. UnselectAll() -> Unselect(). Both trees are wxTR_SINGLE, where UnselectAll() — the MULTI-selection call — does nothing. That was the one-word reason the rename had been vetoed everywhere (BEGIN_LABEL_EDIT refuses while tree_body_selection() >= 0). Fixing it turned a pair of harmless no-ops into a real loop: the two lists clear each other so "the target" is unambiguous, so clicking a body row ran apply_body_row -> m_tree->Unselect() -> the feature tree's SEL_CHANGED -> m_parts->Unselect(), which cleared the row just clicked. The handler now clears the other list only when it actually holds a selection. 2. Trusting a screenshot taken after a polluted run. A leftover Rib card had shifted the whole panel, so a click measured against it landed nowhere near the row. Relaunch, then measure. VERIFIED, on the rig and in the kernel: body renamed ('Extrude', user_name=False) -> ('Bracket', user_name=True) features untouched ['Sketch', 'Extrude'] before and after survives a recompute add a Hole to the same body: features become ['Sketch', 'Extrude', 'Hole'], body stays 'Bracket' survives the recipe serialize -> deserialize -> 'Bracket' New kernel test "a body carries its own name, through recompute and the recipe" pins all three properties; the suite is 189 cases / 2547 assertions. Gate green: ALL LADDERS HELD — offer table matches the atlas, kernel suite, engine rungs 1-8, 977-sheet corpus + the heaviest sheets, gesture ladder 98/98, offer ladder 108/108. --- src/libslic3r/CAD/CadDocument.cpp | 55 ++++++++++++++++- src/libslic3r/CAD/CadDocument.hpp | 8 +++ src/slic3r/GUI/CAD/DesignPanel.cpp | 91 ++++++++++++++++------------ src/slic3r/GUI/CAD/McpControl.cpp | 6 +- tests/libslic3r/test_caddocument.cpp | 48 +++++++++++++++ 5 files changed, 168 insertions(+), 40 deletions(-) diff --git a/src/libslic3r/CAD/CadDocument.cpp b/src/libslic3r/CAD/CadDocument.cpp index 300fbd9ee3..42c49b4d91 100644 --- a/src/libslic3r/CAD/CadDocument.cpp +++ b/src/libslic3r/CAD/CadDocument.cpp @@ -3592,6 +3592,12 @@ bool CadDocument::recompute() built[i].has_color = true; built[i].color = bodies[i].color; } + // ...and the name the user gave the body, for the same reason and by the same index + // contract. Without this a rename would live exactly until the next feature was added. + if (bodies[i].has_user_name) { + built[i].has_user_name = true; + built[i].user_name = bodies[i].user_name; + } } bodies = std::move(built); // The face and edge maps have just been rebuilt, so every global id handed out before this @@ -3729,6 +3735,31 @@ std::string CadDocument::serialize_recipe() const uint32_t vlen = static_cast(vb.size()); ar(vlen); ar(cereal::binary_data(vb.data(), vb.size())); + // BODY NAMES, appended after the variables block. Bodies are not serialised — they are + // recomputed — so a name the user gave one has nowhere else to live, and without this it + // would survive a recompute (see the carry-over in recompute()) but not a save. + // + // APPENDED RATHER THAN VERSION-BUMPED, on purpose: a build that predates this block + // reads features and variables, returns, and never looks at the trailing bytes, so its + // projects still open here AND this build's projects still open there. A version bump + // would have made every project written today unreadable by yesterday's build for the + // sake of one optional field. Written as (index, name) pairs so an unnamed body costs + // nothing. + // A map, not a vector of pairs: cereal's map support is already included here and its + // pair support is not, and one more include for one more field is not worth it. + std::map named; + for (uint32_t i = 0; i < bodies.size(); ++i) + if (bodies[i].has_user_name && !bodies[i].user_name.empty()) + named[i] = bodies[i].user_name; + std::ostringstream bos; + { + cereal::BinaryOutputArchive ba(bos); + ba(named); + } + std::string bb = bos.str(); + uint32_t blen = static_cast(bb.size()); + ar(blen); + ar(cereal::binary_data(bb.data(), bb.size())); } return oss.str(); } @@ -3789,7 +3820,29 @@ bool CadDocument::deserialize_recipe(const std::string& blob) } catch (...) { // Variables blob predates this build: keep what was read, default the rest. } - return recompute(); + // Body names, if this project carries them. A project written before the block + // simply ends here, so the read throws and there are no names — not an error. + std::map named; + try { + uint32_t blen; + ar(blen); + std::string bbuf(blen, '\0'); + if (blen > 0) + ar(cereal::binary_data(&bbuf[0], blen)); + std::istringstream bs(bbuf); + cereal::BinaryInputArchive ba(bs); + ba(named); + } catch (...) { + named.clear(); + } + // AFTER the rebuild, never before: recompute() replaces the bodies vector wholesale. + const bool ok = recompute(); + for (const auto& kv : named) + if (kv.first < bodies.size()) { + bodies[kv.first].has_user_name = true; + bodies[kv.first].user_name = kv.second; + } + return ok; } if (v == 4) { // Pre-framing flat path, unchanged: v4 projects keep opening exactly as before. diff --git a/src/libslic3r/CAD/CadDocument.hpp b/src/libslic3r/CAD/CadDocument.hpp index 0927e85b05..c97f220f9f 100644 --- a/src/libslic3r/CAD/CadDocument.hpp +++ b/src/libslic3r/CAD/CadDocument.hpp @@ -409,6 +409,14 @@ TopoDS_Shape brep_from_string(const std::string& d); struct CadBody { TopoDS_Shape shape; std::string name; + // The name the USER gave this body. A body is NOT its first feature: an Extrude, a Cut and + // a Fillet all land on the same body, so renaming `source_feature` renames one operation in + // the history, not the object — which is exactly the bug this field exists to end. `name` + // above is the DERIVED label (the maker's name, restamped every recompute) and stays that; + // this one is set only by a rename, carried across recompute() by body index, and written + // into the recipe so it survives save/load. + bool has_user_name{false}; + std::string user_name; // Per-body display colour override (Color tool). When has_color is false the GUI // falls back to the auto body-index palette. Carried across recompute() by body index. bool has_color{false}; diff --git a/src/slic3r/GUI/CAD/DesignPanel.cpp b/src/slic3r/GUI/CAD/DesignPanel.cpp index c33c4e694b..a1e17f996f 100644 --- a/src/slic3r/GUI/CAD/DesignPanel.cpp +++ b/src/slic3r/GUI/CAD/DesignPanel.cpp @@ -1032,47 +1032,25 @@ DesignPanel::DesignPanel(wxWindow* parent) // tool vocabulary — a rename that only a double-click reveals is not discoverable, and // the row IS the object, so it belongs in the menu (and on F2) as well as on the row. auto rename_feature = [this] { - int sel = tree_selection(); - // A SELECTED BODY RENAMES THE FEATURE THAT MAKES IT. A body has no name it can - // keep — it is recomputed from the recipe on every change and CadBody::name is - // derived from its source feature, which is why the label editor vetoes body rows - // outright. So the verb resolves the body to that feature instead of refusing. - // Reported as "clicking on a body row in feature tree, I cannot find rename on - // right click": the offer DID open (that is the designed gesture for a selected - // body) and simply had no Rename row, because the verb accepted sk_loop alone. - if (sel == wxNOT_FOUND) { - const int b = (m_sel_solid_body >= 0) ? m_sel_solid_body : tree_body_selection(); - if (b >= 0 && b < int(m_doc.bodies.size())) { - const int src = m_doc.bodies[b].source_feature; - if (src >= 0 && src < int(m_tree_items.size())) { - // Unselect(), NOT UnselectAll(): both trees are wxTR_SINGLE, and - // UnselectAll() is the MULTI-selection call — on a single-selection tree - // it leaves the row selected. That is why every route into the rename - // failed identically: the body row stayed selected, so - // tree_body_selection() stayed >= 0 and BEGIN_LABEL_EDIT vetoed the - // edit before it could open. Proven by discriminator: F2 on a body row - // failed exactly like the menu, which rules out the menu's event loop. - if (m_parts) m_parts->Unselect(); - m_tree->SelectItem(m_tree_items[src]); - m_status->SetForegroundColour(wxNullColour); - set_status(wxString::Format( - _L("A body takes its name from the feature that makes it — renaming '%s'"), - wxString::FromUTF8(m_doc.features[src].name))); - m_status->Refresh(); - sel = src; - } - } + // A BODY renames ITSELF. The earlier version resolved the body to + // CadBody::source_feature and renamed that feature, which is the wrong object: a + // body accumulates many features and the first one is not its name. The body row + // is editable now (CadBody::user_name), so the verb opens the editor there. + const int b = tree_body_selection(); + if (b >= 0 && b < int(m_tree_body_items.size())) { + const wxTreeItemId row = m_tree_body_items[b]; + // After the menu, not inside it: an editor opened from within PopupMenu's + // nested loop never appears. + CallAfter([this, row] { m_parts->SetFocus(); m_parts->EditLabel(row); }); + return; } + const int sel = tree_selection(); if (sel != wxNOT_FOUND && sel < int(m_tree_items.size())) { - // AFTER the menu, not inside it: the offer runs a nested event loop and an - // in-place editor opened from within it never appears (measured three times — - // the row selected, the handler ran, no editor). Same family as the - // refresh_tree-inside-END_LABEL_EDIT trap documented further down this file. const wxTreeItemId row = m_tree_items[sel]; CallAfter([this, row] { m_tree->SetFocus(); m_tree->EditLabel(row); }); } else { m_status->SetForegroundColour(wxNullColour); // "nothing selected" is not an error - set_status(_L("Select a feature first — click a sketch or feature row, then rename it")); + set_status(_L("Select a feature, or a body, first — then rename it")); m_status->Refresh(); } }; @@ -3100,7 +3078,13 @@ DesignPanel::DesignPanel(wxWindow* parent) if (!m_viewport) return; // Bodies live in the Parts list now; picking a feature here drops any body selection // so the two lists can't both claim to be "the target". - if (m_parts) m_parts->Unselect(); // wxTR_SINGLE: UnselectAll() does nothing here + // ONLY when this tree actually has a selection. These two lists clear each other's + // selection so that "the target" is never ambiguous, and that was harmless while both + // calls were UnselectAll() — a no-op on a wxTR_SINGLE tree. Now that Unselect() really + // clears, the pair became a loop: clicking a body row runs apply_body_row, which calls + // m_tree->Unselect(), which fires THIS handler, which cleared the body row the user had + // just clicked. The guard keeps the mutual-exclusion and drops the echo. + if (m_parts && tree_selection() != wxNOT_FOUND) m_parts->Unselect(); const int sel = tree_selection(); const bool body = (sel >= 0 && sel < int(m_doc.features.size()) && m_doc.features[sel].type != CadFeatureType::Sketch && @@ -3326,7 +3310,7 @@ DesignPanel::DesignPanel(wxWindow* parent) m_parts_rule->Hide(); m_parts = new wxTreeCtrl(m_parts_box, wxID_ANY, wxDefaultPosition, wxSize(-1, 48), wxTR_HIDE_ROOT | wxTR_SINGLE | wxTR_NO_LINES | - wxTR_FULL_ROW_HIGHLIGHT | wxBORDER_SIMPLE); + wxTR_FULL_ROW_HIGHLIGHT | wxBORDER_SIMPLE | wxTR_EDIT_LABELS); if (!dp_dark()) m_parts->SetBackgroundColour(dp_panel_bg()); parts_inner->Add(m_parts, 0, wxEXPAND | wxALL, 12); // Start hidden: a fresh document has no bodies, and refresh_parts() only runs on the first @@ -3363,6 +3347,32 @@ DesignPanel::DesignPanel(wxWindow* parent) // can see highlighted. That is the confirmation a face pick cannot give: pointing at a face // lights the face, never the body the verb will actually change. The status line above has // been promising this right-click since before it existed. + // Renaming a BODY names the body itself. It does NOT rename the feature that created it: + // an Extrude, a Cut and a Fillet all land on one body, so source_feature is one operation in + // its history and renaming that is renaming the wrong object — reported, correctly, as "you + // consider the extrusion = the body". CadBody::user_name is carried across recompute() by + // index and written into the recipe, so the name outlives both the rebuild and the save. + m_parts->Bind(wxEVT_TREE_BEGIN_LABEL_EDIT, [this](wxTreeEvent& e) { + if (tree_body_selection() < 0) { e.Veto(); return; } + e.Skip(); + }); + m_parts->Bind(wxEVT_TREE_END_LABEL_EDIT, [this](wxTreeEvent& e) { + if (e.IsEditCancelled()) return; + const int b = tree_body_selection(); + if (b < 0 || b >= int(m_doc.bodies.size())) { e.Veto(); return; } + wxString label = e.GetLabel(); + label.Trim(true).Trim(false); + if (label.empty()) { e.Veto(); return; } // a nameless row is worse than a bad name + m_doc.bodies[b].has_user_name = true; + m_doc.bodies[b].user_name = std::string(label.ToUTF8().data()); + sync_recipe_to_model(); // the name is part of what gets saved + e.Skip(); + // Rebuild on the NEXT event-loop turn: refresh_parts() destroys every wxTreeItemId and + // we are inside wx's own END_LABEL_EDIT dispatch for one of them. The feature tree + // learned this the hard way — doing it here took the process down. + CallAfter([this] { refresh_parts(); }); + }); + m_parts->Bind(wxEVT_TREE_ITEM_MENU, [this, apply_body_row](wxTreeEvent& e) { if (e.GetItem().IsOk()) m_parts->SelectItem(e.GetItem()); // the row under the cursor, never a stale one @@ -7074,7 +7084,12 @@ void DesignPanel::refresh_parts() // renaming through this row and seeing the label sit unchanged at "Body 1" would read as // a rename that did nothing. const bool vis = b >= m_body_visible.size() || m_body_visible[b]; - const wxString bname = wxString::FromUTF8(m_doc.bodies[b].name); + // The user's name wins over the derived one (the maker's name, restamped every + // recompute); "Body N" still leads, because every status line, the interference report + // and the mate errors identify a body by its number. + const wxString bname = m_doc.bodies[b].has_user_name + ? wxString::FromUTF8(m_doc.bodies[b].user_name) + : wxString::FromUTF8(m_doc.bodies[b].name); wxTreeItemId id = m_parts->AppendItem(proot, bname.IsEmpty() ? wxString::Format(_L("Body %zu"), b + 1) : wxString::Format(_L("Body %zu — %s"), b + 1, bname)); diff --git a/src/slic3r/GUI/CAD/McpControl.cpp b/src/slic3r/GUI/CAD/McpControl.cpp index 3bf270e49c..c759511b2a 100644 --- a/src/slic3r/GUI/CAD/McpControl.cpp +++ b/src/slic3r/GUI/CAD/McpControl.cpp @@ -407,7 +407,11 @@ json describe_scene(DesignPanel* panel) json bodies = json::array(); for (size_t i = 0; i < doc.bodies.size(); ++i) { - json b{{"index", int(i)}, {"name", doc.bodies[i].name}, + // The user's name when the body has one, the derived maker name otherwise — the same + // rule the Bodies row shows, so a driver and a person read the same thing. + json b{{"index", int(i)}, + {"name", doc.bodies[i].has_user_name ? doc.bodies[i].user_name : doc.bodies[i].name}, + {"user_name", doc.bodies[i].has_user_name}, {"has_color", doc.bodies[i].has_color}}; // Per-body bbox/centre from the already-tessellated display meshes. if (i < doc.display_body_meshes.size() && !doc.display_body_meshes[i].empty()) { diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index 1bb12efa68..a9e4ccbcaa 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -8097,3 +8097,51 @@ TEST_CASE("A sketch-only document recomputes and round-trips", "[CadDocument]") REQUIRE_FALSE(bad.recompute()); REQUIRE_FALSE(bad.error.empty()); } + +// A body is NOT the feature that created it. Reported 2026-08-23, in these words: "you have +// renamed the feature extrusion, not the body ... this means that you consider the extrusion = +// the body, which is very far from truth as a body can contain several extrusions." The rename +// used to resolve CadBody::source_feature and rename THAT, so naming a body edited one operation +// in its history. A body now carries its own name, and this pins the three properties that make +// it a name rather than a label: it does not touch the features, it survives a recompute that +// adds more features to the same body, and it survives the recipe round trip. +TEST_CASE("a body carries its own name, through recompute and the recipe", "[CadDocument]") +{ + CadDocument doc; + std::vector ents; + auto line = [&](double x0, double y0, double x1, double y1) { + SketchEntity e; e.type = SketchEntity::Type::Line; + e.p0 = Vec2d(x0, y0); e.p1 = Vec2d(x1, y1); ents.push_back(e); }; + line(0, 0, 40, 0); line(40, 0, 40, 30); line(40, 30, 0, 30); line(0, 30, 0, 0); + + const int sk = doc.add_sketch_entities(ents, SketchPlane::XY(), "Profile"); + doc.add_extrude(sk, 10.0, false, BooleanMode::New, "Extrude"); + REQUIRE(doc.recompute()); + REQUIRE(doc.bodies.size() == 1); + + doc.bodies[0].has_user_name = true; + doc.bodies[0].user_name = "Bracket"; + + // A SECOND feature lands on the same body — the case the report is about. + doc.add_hole(6.0, 5.0, true, 20.0, 15.0, SketchPlane::XY(), "Hole"); + REQUIRE(doc.recompute()); + REQUIRE(doc.bodies.size() == 1); + CHECK(doc.bodies[0].has_user_name); + CHECK(doc.bodies[0].user_name == "Bracket"); + + // The features keep their own names: renaming the body renamed nothing else. + REQUIRE(doc.features.size() == 3); + CHECK(doc.features[0].name == "Profile"); + CHECK(doc.features[1].name == "Extrude"); + CHECK(doc.features[2].name == "Hole"); + + // ...and the name is part of what gets saved. Bodies are recomputed, never serialised, so + // without the recipe block a body name would live exactly until the project was reopened. + const std::string blob = doc.serialize_recipe(); + REQUIRE_FALSE(blob.empty()); + CadDocument fresh; + REQUIRE(fresh.deserialize_recipe(blob)); + REQUIRE(fresh.bodies.size() == 1); + CHECK(fresh.bodies[0].has_user_name); + CHECK(fresh.bodies[0].user_name == "Bracket"); +}