juli/OrcaSlicer
1
1
Fork 0
mirror of https://github.com/OrcaSlicer/OrcaSlicer.git synced 2026-09-27 19:01:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

A stray click must not break a model that looks perfect on screen

Browse Source 
Revolve failed on a sketch whose profile was closed. Decoding the reported 3mf: four
entities forming a proper closed loop (joints open by 4.44e-06 mm, well inside
tolerance) plus one stray 1.82 mm Line at (-24.2, 80.3), inside the shaded region,
touching nothing.

The viewport's region_loops discards open chains ON PURPOSE — it exists to find
EXTRUDABLE regions — so the user saw one clean closed region. entities_to_wires kept
the stray as its own one-edge loop, so it returned two wires, and Revolve goes through
entities_to_wire which demands exactly one. Extrude would have failed one step later in
wires_to_face, because a one-edge open wire bounds no face. Same class as the tolerance
split fixed in 8b568b7b: the viewport and the kernel disagreeing about the sketch — this
time about what BELONGS to the profile.

entities_to_wires/entities_to_wire/build_sketch_wire take closed_only. It is not a
blanket rule: a SurfaceExtrude builds a sheet FROM an open profile and a Sweep PATH is
normally open, so all ten call sites are classified individually — true for the face
fallback, Extrude-taper, Revolve, the Sweep PROFILE and Loft profiles; false for
SurfaceExtrude/Revolve/Loft/Fill and the Sweep path.

A component counts as open when some welded node has DEGREE 1. The first attempt used
"the traversal did not return to its starting node", which regressed the bridged C
profile: a closed loop that also carries a second edge across the same two nodes has no
free endpoint, but its Eulerian walk ends elsewhere. Degree-1 is the property that
actually distinguishes a stray segment from a closed profile; the suite caught the
difference.

Behaviour change decided by Tommaso: a stray is IGNORED, not refused. The test that
required refusal dates from when ignoring meant falling through to a default rectangle —
geometry nobody drew. That fallback is gone, so ignoring now builds the circle the user
actually drew. Its assertion is updated with the reason.

The bridge round-trip test extruded an ENTIRELY open chain and "worked" only because
OCCT will make a face from an open wire. It gets a genuinely closed profile: the test is
about serialization, and deserialize_recipe recomputes, so the document has to be one
that legitimately builds.

Failures now say WHERE. sketch_open_ends reports free endpoints under the same weld
tolerance the wire build uses, and open_loop_message is shared by both throws, because
Extrude fails through build_sketch_face and Revolve through build_sketch_wire — enriching
only one would have left the commoner path the less informative one.

Kernel 66115 assertions / 608 cases green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FbJKJAJxxkhDTs9XdZzKA
This commit is contained in:
Tommaso Bianchi
2026-09-02 14:58:05 +02:00
co-authored by Claude Opus 5
parent 8b568b7b9d
commit bb6a1810f6
6 changed files with 231 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/libslic3r/test_caddocument.cpp
+24 -8
View File
@@ -4703,14 +4703,23 @@ TEST_CASE("bridge round-trip serialization", "[CadDocument][bridge]")
using Catch::Matchers::WithinAbs;
CadDocument doc;
// The two collinear stubs plus the bridge span (0,0)->(30,0); the remaining three lines
// return to the origin so the profile is genuinely CLOSED. It used to be just the two
// stubs and the bridge, an entirely open chain that "extruded" only because OCCT will
// make a face out of an open wire — the silently-wrong geometry this area exists to stop.
// This test is about the bridge's serialization round-trip, and deserialize_recipe
// recomputes, so the document has to be one that legitimately builds.
std::vector<SketchEntity> ents = {
{SketchEntity::Type::Line, Vec2d(0,0), Vec2d(10,0)},
{SketchEntity::Type::Line, Vec2d(20,0), Vec2d(30,0)},
{SketchEntity::Type::Line, Vec2d(0,0), Vec2d(10,0)},
{SketchEntity::Type::Line, Vec2d(20,0), Vec2d(30,0)},
{SketchEntity::Type::Line, Vec2d(30,0), Vec2d(30,10)},
{SketchEntity::Type::Line, Vec2d(30,10), Vec2d(0,10)},
{SketchEntity::Type::Line, Vec2d(0,10), Vec2d(0,0)},
};
int sk = doc.add_sketch_entities(ents, SketchPlane::XY(), "S");
REQUIRE(sk == 0);
int bi = doc.add_bridge(sk, 0, 1, 1, 0, "Bridge");
REQUIRE(bi == 2);
REQUIRE(bi == 5);
doc.add_extrude(sk, 5.0, false, BooleanMode::New, "Extrude");
REQUIRE(doc.recompute());
@@ -4721,7 +4730,7 @@ TEST_CASE("bridge round-trip serialization", "[CadDocument][bridge]")
REQUIRE(doc2.deserialize_recipe(blob));
REQUIRE(doc2.features.size() == 2);
const auto& br = doc2.features[0].entities[2];
const auto& br = doc2.features[0].entities[5];
REQUIRE(br.type == SketchEntity::Type::BSpline);
REQUIRE(br.ctrl.size() == 4);
REQUIRE_THAT(br.ctrl.front().x(), WithinAbs(10.0, 1e-9));
@@ -7484,14 +7493,21 @@ TEST_CASE("An entity sketch that forms no wire fails instead of extruding a defa
Catch::Matchers::WithinRel(M_PI * 100.0 * 5.0, 0.01));
}
SECTION("circle + stray line is rejected, not silently turned into a box") {
// Behaviour CHANGED 2026-09-02 by Tommaso's decision. This used to require a refusal,
// because back then ignoring the stray meant falling through to a default rectangle — a
// box the user never drew. That fallback is gone: the closed profile is now built and the
// stray open chain is discarded, exactly as the viewport's region_loops already discards
// open chains when it decides what is extrudable. A stray click must not break a model
// that looks perfect on screen.
SECTION("circle + stray line builds the circle and ignores the stray") {
CadDocument doc;
int sk = doc.add_sketch_entities({ circle({0, 0}, 10.0), line({40, 40}, {60, 40}) },
SketchPlane::XY(), "Sketch");
doc.add_extrude(sk, 5.0, false, BooleanMode::New, "Extrude");
CHECK_FALSE(doc.recompute());
CHECK(doc.error.find("does not bound a face") != std::string::npos);
CHECK(doc.bodies.empty());
REQUIRE(doc.recompute());
CHECK(doc.error.empty());
CHECK_THAT(doc.body_mass_properties(0).volume,
Catch::Matchers::WithinRel(M_PI * 100.0 * 5.0, 0.01));
}
SECTION("two disjoint circles are rejected too") {
tests/libslic3r/test_sketchedit.cpp
+76
View File
@@ -1,6 +1,7 @@
#include <catch2/catch_all.hpp> // mainline OrcaSlicer ships Catch2 v3 (v2 was catch2/catch.hpp)
#include "libslic3r/CAD/SketchEngine.hpp"
#include <cmath>
#include <algorithm>
#include <TopExp_Explorer.hxx>
#include <TopAbs.hxx>
@@ -652,3 +653,78 @@ TEST_CASE("auto-close off makes the kernel demand an exact joint", "[SketchEngin
// Restore the default so test order cannot leak OFF into the other cases.
Slic3r::set_sketch_auto_close(true);
}
// A stray open segment touching nothing must not break a closed profile: the viewport
// discards open chains when it shades a region extrudable, so with closed_only the kernel
// must discard them too — otherwise Revolve/Extrude fail on a sketch that looks perfect.
TEST_CASE("a stray open segment does not break a closed profile", "[SketchEngine]")
{
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);
return e;
};
std::vector<SketchEntity> ents;
ents.push_back(line(0, 0, 20, 0)); // 20x10 quad
ents.push_back(line(20, 0, 20, 10));
ents.push_back(line(20, 10, 0, 10));
ents.push_back(line(0, 10, 0, 0));
ents.push_back(line(5, 5, 6, 5.2)); // stray, touches nothing
auto edge_count = [](const TopoDS_Wire& w) {
int n = 0;
for (TopExp_Explorer ex(w, TopAbs_EDGE); ex.More(); ex.Next()) ++n;
return n;
};
// Unchanged behaviour: the stray line is its own open wire.
auto wires_all = SketchEngine::entities_to_wires(ents, SketchPlane::XY(), /*closed_only=*/false);
REQUIRE(wires_all.size() == 2);
// closed_only drops the open chain: one closed quad survives.
auto wires_closed = SketchEngine::entities_to_wires(ents, SketchPlane::XY(), /*closed_only=*/true);
REQUIRE(wires_closed.size() == 1);
REQUIRE(edge_count(wires_closed[0]) == 4);
REQUIRE(wires_closed[0].Closed());
// The Revolve path (entities_to_wire) finds the single closed loop.
TopoDS_Wire w = SketchEngine::entities_to_wire(ents, SketchPlane::XY(), /*closed_only=*/true);
REQUIRE_FALSE(w.IsNull());
REQUIRE(edge_count(w) == 4);
}
// sketch_open_ends names the two free endpoints of an open chain, so the "does not form a
// single closed wire" failure can say WHERE the sketch is open.
TEST_CASE("sketch_open_ends names where a chain fails to close", "[SketchEngine]")
{
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);
return e;
};
// Open C shape: three lines, free endpoints at (0,0) and (0,10).
std::vector<SketchEntity> ents;
ents.push_back(line(0, 0, 10, 0));
ents.push_back(line(10, 0, 10, 10));
ents.push_back(line(10, 10, 0, 10));
auto got = sketch_open_ends(ents, SketchPlane::XY());
REQUIRE(got.size() == 2);
std::sort(got.begin(), got.end(), [](const Vec2d& a, const Vec2d& b) {
if (a.x() < b.x()) return true;
if (a.x() > b.x()) return false;
return a.y() < b.y();
});
REQUIRE_THAT(got[0].x(), WithinAbs(0.0, 1e-9));
REQUIRE_THAT(got[0].y(), WithinAbs(0.0, 1e-9));
REQUIRE_THAT(got[1].x(), WithinAbs(0.0, 1e-9));
REQUIRE_THAT(got[1].y(), WithinAbs(10.0, 1e-9));
}