mirror of
https://github.com/OrcaSlicer/OrcaSlicer.git
synced 2026-09-19 15:03:05 +00:00
A body carries its own name, because a body is not its first feature
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.
This commit is contained in:
@@ -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<uint32_t>(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<uint32_t, std::string> 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<uint32_t>(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<uint32_t, std::string> 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.
|
||||
|
||||
@@ -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};
|
||||
|
||||
@@ -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));
|
||||
|
||||
@@ -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()) {
|
||||
|
||||
@@ -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<SketchEntity> 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");
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user