mirror of
https://github.com/OrcaSlicer/OrcaSlicer.git
synced 2026-09-18 22:42:37 +00:00
the same phantom-endpoint bug in Coincident, and the two unguarded branches next to it
Port of snaporca da8d011b87; parity holds (DesignPanel.cpp still exactly 32 divergent
lines). Verified independently on this fork's own rig: full ladder 126/126 against
BuildID 96c697a3, built from this tree.
Reviewing the DistanceX/Y fix for OTHER members of its class found three more live defects
on the constrain toolbar. All four share one root: a branch assumes every picked entity has
two endpoints, and the solver's refusal to resolve a role it cannot find is silent.
COINCIDENT had the identical closest-pair walk over {P0,p0},{P1,p1}. For two Points the
phantom (0,0) pair sits at distance 0, which is the smallest distance there is, so it
ALWAYS won: ptOf(Point,P1) -> 0, ref_ok fails (SketchSolver.cpp:185), constraint dropped.
Not sometimes -- every press.
HORIZONTAL/VERTICAL hardcoded ra=P0, rb=P1 with no type check. With a Point picked the
constraint is dropped by the same mechanism but still STORED: constraints goes 0 -> 1 after
the commit and nothing moves, so the Constraints list shows a dimension that can never do
anything. Worse than refusing -- the panel claims the sketch is constrained when it is not.
ANGLE computed p1-p0 on whatever was picked. On a circle that is (0,0)-centre, so two
circles pre-filled the field with the angle between their centre POSITION VECTORS (178.83
deg for two on the x axis), and accepting it emits SLVS_C_ANGLE on two circle prims.
Both branches now refuse with a message. entity_ends()/closest_ends() are file-scope and
shared by Coincident and DistanceX/Y, so there is one implementation instead of two that
drift.
Two smaller findings from the same review: infer_auto_constraints' roles_of omitted
EllipseArc while heal_coincidences' identical copy has it; and set_point(Circle, Center)
wrote e.center and not e.p0, breaking the "p0 mirrors centre" invariant for the duration of
a live drag.
New rungs D8 and D9, both RED against the shipped binary and green here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MrMzTpAf78U4NG2M8jfvHY
This commit is contained in:
Tommaso Bianchi
co-authored by
Claude Opus 5
Claude Opus 5
parent
507b45431c
commit
cd30fb891e
@@ -1155,6 +1155,43 @@ def rung_distance_xy():
|
||||
leave_sketch()
|
||||
|
||||
|
||||
def rung_coincident_points():
|
||||
reset_document()
|
||||
print("\nD8 constrain — Coincident on two POINTS actually joins them")
|
||||
# The regression this exists for: the closest-pair search used to enumerate {P0,p0},{P1,p1}
|
||||
# on both entities, and a Point's p1 reads (0,0). For two points the phantom pair is at
|
||||
# distance 0, which ALWAYS wins, so the constraint was added against a role the solver
|
||||
# cannot resolve and dropped in silence -- constraints=0, the points never moved, no error.
|
||||
# Two points is the simplest possible use of the button and it did nothing on every press.
|
||||
enter_sketch("p")
|
||||
clickmm(-30, -20)
|
||||
key("p", 0.6)
|
||||
clickmm(25, 18)
|
||||
key("k", 1.5)
|
||||
d1 = describe()
|
||||
ps = [e for e in d1["entities"] if e["type"] == "point"]
|
||||
ia, ib = d1["entities"].index(ps[0]), d1["entities"].index(ps[1])
|
||||
gap0 = math.dist(ps[0]["p"], ps[1]["p"])
|
||||
check("VERTEX", gap0 > 1.0, f"they start apart: {gap0:.6f}")
|
||||
clickmm(*ps[0]["p"]); clickmm(*ps[1]["p"])
|
||||
click(*CON_BTN["coincident"])
|
||||
time.sleep(1.2)
|
||||
d = describe()
|
||||
gap = math.dist(d["entities"][ia]["p"], d["entities"][ib]["p"])
|
||||
check("VERTEX", gap < 1e-6, f"and are now joined: gap {gap:.9f}")
|
||||
# The count has to be read AFTER the round trip, never during the session: while
|
||||
# constraining, sketch_describe reports the LIVE tool's constraints, which the constrain
|
||||
# session never touches -- it writes the committed feature's entity_constraints. Reading it
|
||||
# here says 0 for a constraint that really did land. Same trap the D2 rung documents.
|
||||
d2 = confirm_and_reopen()
|
||||
check("CLOSED", d2["solve_ok"] and d2["constraints"] > 0,
|
||||
f"{d2['constraints']} constraints survived the commit — a dropped one leaves this at 0")
|
||||
ps2 = [e for e in d2["entities"] if e["type"] == "point"]
|
||||
check("VERTEX", math.dist(ps2[0]["p"], ps2[1]["p"]) < 1e-6,
|
||||
"and they are still joined after the round trip")
|
||||
leave_sketch()
|
||||
|
||||
|
||||
def rung_symmetric_axis():
|
||||
reset_document()
|
||||
print("\nD7 constrain — symmetric about the vertical axis, with no construction line")
|
||||
@@ -1187,6 +1224,56 @@ def rung_symmetric_axis():
|
||||
leave_sketch()
|
||||
|
||||
|
||||
def rung_type_guards():
|
||||
reset_document()
|
||||
print("\nD9 constrain — a button that cannot apply must SAY so, not store a dead constraint")
|
||||
# Both halves were live defects found by reviewing the DistanceX/Y fix for its class.
|
||||
#
|
||||
# Horizontal on a lone Point: P1 is a role the solver cannot resolve, so the constraint was
|
||||
# dropped at ref_ok -- but still STORED. Measured before the fix: constraints 0 -> 1 after the
|
||||
# commit, point unmoved. A constraint listed in the panel that can never do anything is worse
|
||||
# than a refusal, because the panel then claims the sketch is constrained when it is not.
|
||||
enter_sketch("p")
|
||||
clickmm(-30, -20)
|
||||
key("k", 1.5)
|
||||
d = describe()
|
||||
ps = [e for e in d["entities"] if e["type"] == "point"]
|
||||
ia = d["entities"].index(ps[0])
|
||||
clickmm(*ps[0]["p"])
|
||||
click(*CON_BTN["horizontal"])
|
||||
time.sleep(1.0)
|
||||
d = describe()
|
||||
check("VERTEX", near(d["entities"][ia]["p"][0], ps[0]["p"][0], 1e-9) and
|
||||
near(d["entities"][ia]["p"][1], ps[0]["p"][1], 1e-9),
|
||||
"Horizontal on a point moved nothing, as it must")
|
||||
d2 = confirm_and_reopen()
|
||||
check("CLOSED", d2["constraints"] == 0,
|
||||
f"and stored NO constraint: {d2['constraints']} (a dead stored one reads 1)")
|
||||
leave_sketch()
|
||||
|
||||
# Angle on two circles: p1-p0 on a circle is (0,0)-centre, so the field used to pre-fill with
|
||||
# the angle between the two centre POSITION VECTORS -- 178.83 deg for two circles on the x
|
||||
# axis -- and committing it fed SLVS_C_ANGLE two circle prims, which are not directions.
|
||||
reset_document()
|
||||
enter_sketch("c")
|
||||
clickmm(-35, 0); clickmm(-20, 0); key("Escape", 0.7)
|
||||
key("c", 0.6)
|
||||
clickmm(35, 0); clickmm(60, 0); key("Escape", 0.7)
|
||||
key("k", 1.5)
|
||||
d = describe()
|
||||
cs = [e for e in d["entities"] if e["type"] == "circle"]
|
||||
clickmm(*rim(cs[0])); clickmm(*rim(cs[1]))
|
||||
click(*CON_BTN["angle"])
|
||||
time.sleep(1.0)
|
||||
check("ANGLE", field_win() is None,
|
||||
"Angle on two circles opened no value field")
|
||||
key("Escape", 0.6)
|
||||
d2 = confirm_and_reopen()
|
||||
check("CLOSED", d2["constraints"] == 0,
|
||||
f"and stored no angle: {d2['constraints']}")
|
||||
leave_sketch()
|
||||
|
||||
|
||||
def rung_undo():
|
||||
print("\nE1 undo — the last entity goes, the rest do not move")
|
||||
enter_sketch("l")
|
||||
@@ -1452,6 +1539,8 @@ RUNGS = {"rect": rung_rect, "circle": rung_circle, "line": rung_line, "arc": run
|
||||
"perpendicular": rung_perpendicular,
|
||||
"equal_radius": rung_equal_radius, "collinear": rung_collinear,
|
||||
"distance_xy": rung_distance_xy, "symmetric_axis": rung_symmetric_axis,
|
||||
"coincident_points": rung_coincident_points,
|
||||
"type_guards": rung_type_guards,
|
||||
"undo": rung_undo,
|
||||
"feature_undo": rung_feature_undo, "roundtrip": rung_roundtrip,
|
||||
"scale": rung_scale}
|
||||
|
||||
@@ -7720,6 +7720,62 @@ void DesignPanel::on_begin_constrain(int sel_override)
|
||||
m_status->Refresh();
|
||||
}
|
||||
|
||||
// The point roles an entity ACTUALLY exposes, for the closest-pair searches below.
|
||||
//
|
||||
// Enumerating {P0,p0},{P1,p1} blindly is a silent-no-op generator: a Point's p1 is unused and
|
||||
// reads (0,0) (SketchEngine.hpp:32), as does a Circle's, so the search picks those two phantom
|
||||
// origins at distance 0 -- which for a pair of Points ALWAYS wins, being the smallest distance
|
||||
// there is. The constraint is then added against a role the solver cannot resolve
|
||||
// (ptOf(Point,P1) -> s.p1 -> 0), ref_ok fails, and it is dropped. Nothing errors: the button
|
||||
// just does nothing, on every press. Measured on both the DistanceX/Y and the Coincident paths.
|
||||
//
|
||||
// Returns the number of roles written, 0 for a type with no usable point. Same role set as
|
||||
// roles_of() in DesignSketchTool::heal_coincidences (the copy at 9348 -- the one inside
|
||||
// infer_auto_constraints omits EllipseArc), plus the circle centre, which is a real solver
|
||||
// handle for BOTH Circle (SketchSolver.cpp:109) and Ellipse (:126), and the only sensible thing
|
||||
// a round entity can be coincident with or measured from.
|
||||
//
|
||||
// Every one of the seven types returns at least one role today, so the callers' "no usable
|
||||
// point" branch is unreachable and their message cannot currently fire. It is kept for the
|
||||
// eighth type, not as protection against the defect above -- that one was never a missing role,
|
||||
// it was a role the solver silently refused.
|
||||
static int entity_ends(const SketchEntity& e, std::pair<SketchPointRole, Vec2d> out[2])
|
||||
{
|
||||
using ET = SketchEntity::Type;
|
||||
using R = SketchPointRole;
|
||||
switch (e.type) {
|
||||
case ET::Line: case ET::Arc: case ET::BSpline: case ET::EllipseArc:
|
||||
out[0] = {R::P0, e.p0}; out[1] = {R::P1, e.p1}; return 2;
|
||||
case ET::Point:
|
||||
out[0] = {R::P0, e.p0}; return 1;
|
||||
case ET::Circle: case ET::Ellipse:
|
||||
out[0] = {R::Center, e.center}; return 1;
|
||||
}
|
||||
return 0;
|
||||
}
|
||||
|
||||
// The closest (role, role) pair between two entities, over the roles each really exposes.
|
||||
// Returns false when either side has none, so the caller can say so instead of going quiet.
|
||||
static bool closest_ends(const SketchEntity& A, const SketchEntity& B,
|
||||
SketchPointRole& ra, SketchPointRole& rb, Vec2d& pa, Vec2d& pb)
|
||||
{
|
||||
std::pair<SketchPointRole, Vec2d> aps[2], bps[2];
|
||||
const int na = entity_ends(A, aps), nb = entity_ends(B, bps);
|
||||
if (na == 0 || nb == 0) return false;
|
||||
double best = 1e30;
|
||||
ra = aps[0].first; rb = bps[0].first; pa = aps[0].second; pb = bps[0].second;
|
||||
for (int i = 0; i < na; ++i)
|
||||
for (int j = 0; j < nb; ++j) {
|
||||
const double d = (aps[i].second - bps[j].second).squaredNorm();
|
||||
if (d < best) {
|
||||
best = d;
|
||||
ra = aps[i].first; rb = bps[j].first;
|
||||
pa = aps[i].second; pb = bps[j].second;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
void DesignPanel::apply_entity_constraint(SketchConstraintType type)
|
||||
{
|
||||
using R = SketchPointRole;
|
||||
@@ -7764,7 +7820,16 @@ void DesignPanel::apply_entity_constraint(SketchConstraintType type)
|
||||
switch (type) {
|
||||
case T::Horizontal:
|
||||
case T::Vertical:
|
||||
// One line: level/plumb its own two endpoints.
|
||||
// One line: level/plumb its own two endpoints. The type check is not pedantry -- with a
|
||||
// Point or a Circle picked, P1 is a role the solver cannot resolve, so the constraint is
|
||||
// dropped at ref_ok (SketchSolver.cpp:184) while still being STORED in the feature. It
|
||||
// then sits in the Constraints list, permanently doing nothing, which is worse than the
|
||||
// button refusing: the panel says the sketch is constrained when it is not. Measured on
|
||||
// the rig: Horizontal on a lone Point commits, constraints goes 0 -> 1, nothing moves.
|
||||
if (feat.entities[e0].type != SketchEntity::Type::Line) {
|
||||
fail(_L("Horizontal and Vertical apply to a line"));
|
||||
return;
|
||||
}
|
||||
def.ea = e0; def.ra = R::P0;
|
||||
def.eb = e0; def.rb = R::P1;
|
||||
break;
|
||||
@@ -7774,18 +7839,14 @@ void DesignPanel::apply_entity_constraint(SketchConstraintType type)
|
||||
def.ea = e0; def.eb = e1; // two whole line segments (roles unused)
|
||||
break;
|
||||
case T::Coincident: {
|
||||
// Join the closest endpoint pair of the two picked lines.
|
||||
const SketchEntity& A = feat.entities[e0];
|
||||
const SketchEntity& B = feat.entities[e1];
|
||||
const std::pair<R, Vec2d> aps[2] = {{R::P0, A.p0}, {R::P1, A.p1}};
|
||||
const std::pair<R, Vec2d> bps[2] = {{R::P0, B.p0}, {R::P1, B.p1}};
|
||||
R ra = R::P1, rb = R::P0;
|
||||
double best = 1e30;
|
||||
for (const auto& ap : aps)
|
||||
for (const auto& bp : bps) {
|
||||
const double d = (ap.second - bp.second).squaredNorm();
|
||||
if (d < best) { best = d; ra = ap.first; rb = bp.first; }
|
||||
}
|
||||
// Join the closest point pair of the two picked entities. NOT {p0,p1} on both: see
|
||||
// entity_ends above -- two Points always resolved to their phantom (0,0) p1s, so
|
||||
// Coincident on a pair of points did nothing at all, on every press.
|
||||
R ra, rb; Vec2d pa, pb;
|
||||
if (!closest_ends(feat.entities[e0], feat.entities[e1], ra, rb, pa, pb)) {
|
||||
fail(_L("This constraint needs two entities with a point to join"));
|
||||
return;
|
||||
}
|
||||
def.ea = e0; def.ra = ra; def.eb = e1; def.rb = rb;
|
||||
break;
|
||||
}
|
||||
@@ -7794,44 +7855,12 @@ void DesignPanel::apply_entity_constraint(SketchConstraintType type)
|
||||
// Axis-projected distance between the closest endpoint pair of the two picked
|
||||
// entities. Typed in-canvas pre-filled with the current projection, committed on
|
||||
// the typed value (same deferred pattern as Angle).
|
||||
const SketchEntity& A = feat.entities[e0];
|
||||
const SketchEntity& B = feat.entities[e1];
|
||||
// ONLY the roles an entity actually exposes. A Point's p1 is unused and reads (0,0),
|
||||
// and a Circle's likewise -- enumerate {P0,p0},{P1,p1} blindly and the closest-pair
|
||||
// search below picks those two phantom origins, distance 0. The field then opens
|
||||
// pre-filled 0.00 and the solver drops the whole constraint, because ptOf(Point, P1)
|
||||
// resolves to no handle and ref_ok fails. Nothing errors; the dimension just does
|
||||
// nothing. Same role set as roles_of() in DesignSketchTool::heal_coincidences.
|
||||
auto ends_of = [](const SketchEntity& e, std::pair<R, Vec2d> out[2]) -> int {
|
||||
using ET = SketchEntity::Type;
|
||||
switch (e.type) {
|
||||
case ET::Line: case ET::Arc: case ET::BSpline: case ET::EllipseArc:
|
||||
out[0] = {R::P0, e.p0}; out[1] = {R::P1, e.p1}; return 2;
|
||||
case ET::Point:
|
||||
out[0] = {R::P0, e.p0}; return 1;
|
||||
case ET::Circle: case ET::Ellipse:
|
||||
out[0] = {R::Center, e.center}; return 1;
|
||||
}
|
||||
return 0;
|
||||
};
|
||||
std::pair<R, Vec2d> aps[2], bps[2];
|
||||
const int na = ends_of(A, aps), nb = ends_of(B, bps);
|
||||
if (na == 0 || nb == 0) {
|
||||
// Only over the roles each entity really exposes -- see entity_ends above.
|
||||
R ra, rb; Vec2d pa, pb;
|
||||
if (!closest_ends(feat.entities[e0], feat.entities[e1], ra, rb, pa, pb)) {
|
||||
fail(_L("This dimension needs two entities with a point to measure between"));
|
||||
return;
|
||||
}
|
||||
R ra = aps[0].first, rb = bps[0].first;
|
||||
Vec2d pa = aps[0].second, pb = bps[0].second;
|
||||
double best = 1e30;
|
||||
for (int i = 0; i < na; ++i)
|
||||
for (int j = 0; j < nb; ++j) {
|
||||
const double d = (aps[i].second - bps[j].second).squaredNorm();
|
||||
if (d < best) {
|
||||
best = d;
|
||||
ra = aps[i].first; rb = bps[j].first;
|
||||
pa = aps[i].second; pb = bps[j].second;
|
||||
}
|
||||
}
|
||||
// The constraint is SIGNED: PROJ_PT_DISTANCE fixes (pB - pA).dot(axis), not its
|
||||
// magnitude. Showing |delta| while the current signed delta is negative would mean
|
||||
// that opening the dimension and simply accepting the number on screen flips the
|
||||
@@ -7870,6 +7899,16 @@ void DesignPanel::apply_entity_constraint(SketchConstraintType type)
|
||||
case T::Angle: {
|
||||
// Angle between two line segments; typed in-canvas at the cursor (no card),
|
||||
// pre-filled with the current angle between the picked lines.
|
||||
//
|
||||
// "Line segments" was an assumption, not a check. p1-p0 on a Circle is (0,0)-centre, so
|
||||
// picking two circles pre-filled the field with the angle between their centre POSITION
|
||||
// VECTORS -- measured on the rig: two circles on the x axis opened at 178.83 deg. Accept
|
||||
// that and SLVS_C_ANGLE is emitted on two circle prims, which are not directions.
|
||||
if (feat.entities[e0].type != SketchEntity::Type::Line ||
|
||||
feat.entities[e1].type != SketchEntity::Type::Line) {
|
||||
fail(_L("Angle applies between two lines"));
|
||||
return;
|
||||
}
|
||||
const int a = e0, b = e1;
|
||||
const Vec2d da = feat.entities[a].p1 - feat.entities[a].p0;
|
||||
const Vec2d db = feat.entities[b].p1 - feat.entities[b].p0;
|
||||
|
||||
@@ -939,7 +939,11 @@ void DesignSketchTool::set_point(int ei, SketchPointRole role, const Vec2d& v)
|
||||
e.p0 = v;
|
||||
break;
|
||||
case SketchEntity::Type::Circle:
|
||||
if (role == SketchPointRole::Center) e.center = v;
|
||||
// p0 mirrors the centre for circles, which is the convention the solver both writes
|
||||
// (SketchSolver.cpp:110) and restores after every solve (:421). Moving only e.center
|
||||
// left the two disagreeing for the whole duration of a live drag, so anything reading
|
||||
// p0 in that window saw the pre-drag position.
|
||||
if (role == SketchPointRole::Center) { e.center = v; e.p0 = v; }
|
||||
break;
|
||||
case SketchEntity::Type::Arc:
|
||||
case SketchEntity::Type::EllipseArc:
|
||||
@@ -2359,6 +2363,12 @@ void DesignSketchTool::infer_auto_constraints(int base, double ang_tol_rad, doub
|
||||
switch (e.type) {
|
||||
case SketchEntity::Type::Line: out[0] = SketchPointRole::P0; out[1] = SketchPointRole::P1; return 2;
|
||||
case SketchEntity::Type::Arc: out[0] = SketchPointRole::P0; out[1] = SketchPointRole::P1; return 2;
|
||||
// EllipseArc was missing here while the otherwise identical roles_of in
|
||||
// heal_coincidences (below) has it, so an ellipse arc's endpoints could be WELDED by the
|
||||
// healer but never auto-inferred coincident at draw time -- the same gesture behaved
|
||||
// differently depending on which path ran. Two copies of one rule is how that happens.
|
||||
case SketchEntity::Type::EllipseArc:
|
||||
out[0] = SketchPointRole::P0; out[1] = SketchPointRole::P1; return 2;
|
||||
case SketchEntity::Type::BSpline:out[0] = SketchPointRole::P0; out[1] = SketchPointRole::P1; return 2;
|
||||
case SketchEntity::Type::Point: out[0] = SketchPointRole::P0; return 1;
|
||||
default: return 0; // circle: centre coincidence handled by Concentric, not here
|
||||
|
||||
Reference in New Issue
Block a user