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

Hole/Thread re-edit: restore the face latch from the feature, not from the last pick

Browse Source 
m_hole_on_face and m_thread_on_face are cleared only by their tool's flyout and by
their plane combobox, so after any on-face hole or thread the flag stays true for
the rest of the session. load_feature_into_dialog restored the stored plane into
the dropdown but never touched the latch, so re-editing from the feature tree
ignored the plane it had just restored: hole_plane() returned the still-latched
face plane, which may belong to a different face, a different body, or a body
since rebuilt. Silent until snaporca-200 added the "On face" row, which then read
as a confidently wrong answer rather than as nothing.

The latch is now rebuilt from the stored feature, which is the only source that
describes THIS hole. Not from the dropdown row: index_from_plane snaps an
arbitrary face plane to the nearest XY/XZ/YZ, so driving the re-edit from the row
would MOVE a hole drilled on a slanted or offset face — that was the reason the
other candidate fix was rejected.

is_base_plane() decides which of the two a stored plane is. It compares the origin
as well as the axes (a plane parallel to XY but 12 mm up snaps to row 0 and would
come back at z=0), and adds modeling_origin before comparing, because hole_plane()
and thread_plane() add it to the dropdown plane before the feature stores it — a
document with a shifted origin would otherwise mistake every dropdown hole for a
face pick. Vector norms, not isApprox, which is relative to magnitude and useless
against the zero origin.

The face's (u,v) extent is not serialized, so m_hole_has_bounds is cleared: the
gizmo's footprint clamp goes unbounded, which is honest, where another face's
bounds are not. The label says which body the face belongs to instead of a face
number the feature does not carry; "(none — uses Hole plane)" is the one thing
that is definitely false there.

snaporca-uif9. Reviewed and compiled (RC=0), not exercised.
This commit is contained in:
Tommaso Bianchi
2026-08-13 07:35:39 +02:00
parent 6b3711fb08
commit 1a6252c88c
1 changed files with 47 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/slic3r/GUI/DesignPanel.cpp
+47
View File
@@ -216,6 +216,24 @@ static int index_from_plane(const SketchPlane& p)
return 0; // XY
}
// Does this plane survive a round trip through the Hole/Thread plane dropdown? The dropdown holds
// only XY/XZ/YZ and index_from_plane SNAPS anything else to the nearest of the three, so a plane
// that is not one of them cannot be restored from the row and has to be carried explicitly.
// Origin is compared too, not just the axes: a face plane parallel to XY but 12 mm up snaps to
// row 0 and would otherwise come back at z=0. Vector norms rather than isApprox, which compares
// relative to magnitude and so is useless against the zero origin.
// modeling_origin is not optional: hole_plane()/thread_plane() ADD it to the dropdown plane before
// the feature stores it, so on a document with a shifted origin every dropdown hole would fail a
// bare XY/XZ/YZ comparison and be mistaken for a face pick.
static bool is_base_plane(const SketchPlane& p, const Vec3d& modeling_origin)
{
SketchPlane b = plane_from_index(index_from_plane(p));
b.origin += modeling_origin;
const double e = 1e-6;
return (p.origin - b.origin).norm() < e && (p.normal - b.normal).norm() < e
&& (p.x_axis - b.x_axis).norm() < e && (p.y_axis - b.y_axis).norm() < e;
}
// #2: a sketch plane on `face` with origin at the face centroid and normal pointing INTO the
// solid, so a positioned hole drills inward and its (x,y) read as the offset from the face
// centre. A hole is rotationally symmetric, so the arbitrary in-plane basis is harmless.
@@ -7967,6 +7985,26 @@ void DesignPanel::load_feature_into_dialog(const CadFeature& f)
m_hole_through->SetValue(f.hole_through);
m_hole_x->SetValue(f.hole_x);
m_hole_y->SetValue(f.hole_y);
// Re-latch the on-face state FROM THE STORED FEATURE (snaporca-uif9). m_hole_on_face is
// only ever cleared by the Hole flyout and by the plane combo, so after any on-face hole
// it stays true — and a re-edit then drilled on whatever face was latched last, which may
// be a different face, a different body, or a body since rebuilt. f is the only source
// guaranteed to describe THIS hole. Not the dropdown row just set above: index_from_plane
// snaps an arbitrary face plane to the nearest XY/XZ/YZ, so driving the re-edit from the
// row would MOVE a hole drilled on a slanted or offset face.
m_hole_on_face = !is_base_plane(f.plane, m_doc.modeling_origin);
m_hole_face_plane = f.plane;
m_hole_face_body = m_hole_on_face ? f.target_body : -1;
// The face's (u,v) extent is not serialized, so the gizmo's footprint clamp has nothing
// to stand on. Unbounded is the honest state; the stale bounds of another face are not.
m_hole_has_bounds = false;
if (m_hole_target_label)
m_hole_target_label->SetLabel(m_hole_on_face
// No face index survives in the feature, and inventing one would be worse than
// saying so. "(none — uses Hole plane)" is the one thing that is definitely false.
? (f.target_body >= 0 ? wxString::Format(_L("On a face of Body %d"), f.target_body + 1)
: _L("On a stored face"))
: _L("(none — uses Hole plane)"));
break;
case CadFeatureType::Thread:
m_thread_plane->SetSelection(index_from_plane(f.plane));
@@ -7978,6 +8016,15 @@ void DesignPanel::load_feature_into_dialog(const CadFeature& f)
m_thread_x->SetValue(f.thread_x);
m_thread_y->SetValue(f.thread_y);
if (m_thread_std) m_thread_std->SetSelection(0); // Custom: spins reflect the stored feature
// Same latch, same failure, same fix as Hole above (snaporca-uif9).
m_thread_on_face = !is_base_plane(f.plane, m_doc.modeling_origin);
m_thread_face_plane = f.plane;
m_thread_face_body = m_thread_on_face ? f.target_body : -1;
if (m_thread_target_label)
m_thread_target_label->SetLabel(m_thread_on_face
? (f.target_body >= 0 ? wxString::Format(_L("On a face of Body %d"), f.target_body + 1)
: _L("On a stored face"))
: _L("(none — uses Thread plane)"));
break;
case CadFeatureType::Shell:
m_shell_thickness->SetValue(f.shell_thickness);