From 5889f6640f529fcf1571486647babaf99cc3f5d3 Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Thu, 13 Aug 2026 23:43:36 +0200 Subject: [PATCH] CAD: extrude a sketch region with its holes, and stop crashing at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rectangle with a circle inside it, drawn in ONE sketch, could not be extruded to a plate with a bore from the GUI. Five defects were in the way. Each was found by driving the app on a headless rig and measuring the result — the code reads correctly at every one of these points, which is why they survived. 1. Wire orientation (kernel). SketchEngine::wires_to_face added every hole as wires[i].Reversed(), which is only right when the sketch happens to wind both loops the same way. A circle drawn clockwise inside a counter-clockwise rectangle came out matching the outer boundary, OCCT swept it as a SECOND contour, and the prism was the plate with its bore filled and the disc's volume counted twice. Measured: bbox 67.17 x 219.67 x 10 with volume 152088 mm3 against a solid box of 147542 — a body larger than its own bounding box, which is the signature. Holes are now added as-is and ShapeFix_Face::FixOrientation() classifies them; that is winding-independent and is the idiom make_extrude_regions already used for imported glyphs, which is why holed TEXT always extruded correctly while a holed SKETCH never did. After the fix: 142996 mm3, implied bore radius 12.03 mm against the circle drawn. 2. The live-sketch click threw the picked region away. region_at() served only as a yes/no gate and on_face_selected() carried no argument, so Extrude fell back to whichever loop the resolver found first — clicking the material of a plate-with-a-hole extruded the disc. The region is now carried through, and DesignPanel also hands it to the tool with set_loop_pick(), AFTER open_tool() because that re-derives selection state, since extrude_uses_loop() reads selected_loop_entities() and that lives on the tool. 3. region_at() had no hole awareness and no innermost preference: it returned the first polygon containing the point. It now skips a region when the point lies inside one of that region's holes, and picks the smallest containing loop, so a click in the bore selects the disc and a click on the material selects the plate. 4. Startup segfault. DesignCanvas::request_repaint probed the GL backend via OpenGLManager::get_gl_info().get_renderer() before the canvas had initialised GL — glGetString with no context current and, before init_opengl(), no loaded function pointers. Anything that asked for a repaint while the panel was still being built landed there, with no window and nothing in the log. It now bails at the top on !is_initialized() and asks for a Refresh instead. Note the crash was in the PROBE, not in render(), which already guards itself. 5. A holed sketch on a plane whose normal points -Z came out as the full box PLUS a disc — 220274 mm3 where 163726 was due (192000 + 28274). wires_to_face took a SketchPlane parameter it never used and let OCCT infer a surface from the outer wire; when the inferred normal disagreed with the sketch's, the hole classification produced no hole. Every face is now built on the sketch's own gp_Pln. Also in this change, from the same rig session: - A right-click that only clears the sketch selection no longer reports itself as consumed, so it stops suppressing the offer menu. With any geometry in a live sketch there was no menu route left to add a second entity. - Escape no longer discards a live sketch that holds drawn geometry; it says so and keeps the work (live_sketch_has_work()). - The holed-region fill is an even-odd scanline instead of a keyhole bridge, so no corridor triangle leaks from the bore to the nearest corner. - Cyan is reserved for the selection: an unselected region no longer wears a shade one step off the selected one. - The origin planes follow the mode, so pressing Sketch on a document that already has a body offers them again instead of naming a plane you cannot see. Tests: three [holes] cases over add_extrude_entities asserting the plate-with-bore volume, solid and face counts on both a +Z and a -Z sketch plane, and the by-name refusal of two disjoint regions. Full [CadDocument] suite green. --- src/libslic3r/SketchEngine.cpp | 33 ++++- src/slic3r/GUI/DesignCanvas.cpp | 30 +++- src/slic3r/GUI/DesignCanvas.hpp | 4 +- src/slic3r/GUI/DesignPanel.cpp | 41 +++++- src/slic3r/GUI/DesignSketchTool.cpp | 194 +++++++++++++++++++++++--- src/slic3r/GUI/DesignSketchTool.hpp | 34 ++++- src/slic3r/GUI/SketchInlineEditor.cpp | 7 + src/slic3r/GUI/SketchInlineEditor.hpp | 1 + tests/libslic3r/test_caddocument.cpp | 106 ++++++++++++++ 9 files changed, 418 insertions(+), 32 deletions(-) diff --git a/src/libslic3r/SketchEngine.cpp b/src/libslic3r/SketchEngine.cpp index 76a822de23..11d1907ecf 100644 --- a/src/libslic3r/SketchEngine.cpp +++ b/src/libslic3r/SketchEngine.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -727,10 +728,21 @@ TopoDS_Wire SketchEngine::entities_to_wire(const std::vector& enti } TopoDS_Face SketchEngine::wires_to_face(const std::vector& wires, - const SketchPlane& /*plane*/) + const SketchPlane& plane) { if (wires.empty()) throw std::runtime_error("sketch has no closed loop"); + // The ASSEMBLED face below is built on the SKETCH's own plane rather than on a surface OCCT + // infers from the outer wire. The inferred plane has no reason to share the sketch's normal, + // and when they disagree the hole classification produces no hole: a plate sketched on a + // plane whose normal points -Z came out as the full box PLUS a disc (measured 220274 mm3 + // where 163726 was due — 192000 box + 28274 disc). `plane` was a parameter this function + // never used. Only the assembly is named: the single-wire and per-wire-area builds keep the + // inferred surface, because naming a plane also makes MakeFace accept a wire that does not + // bound a face, and that failure is the check an open stray line is caught by. + const gp_Pln pln(gp_Pnt(plane.origin.x(), plane.origin.y(), plane.origin.z()), + gp_Dir(plane.normal.x(), plane.normal.y(), plane.normal.z())); + if (wires.size() == 1) { BRepBuilderAPI_MakeFace fm(wires[0]); if (!fm.IsDone()) throw std::runtime_error("sketch loop does not bound a face"); @@ -759,7 +771,7 @@ TopoDS_Face SketchEngine::wires_to_face(const std::vector& wires, // Note: NOT MakeFace(faces[outer], wires[outer]) — that constructor copies the outer face // (including its existing boundary wire) and then adds the wire again, doubling the outer // boundary. The wire-only constructor starts clean and the reversed holes follow. - BRepBuilderAPI_MakeFace fm(wires[outer]); + BRepBuilderAPI_MakeFace fm(pln, wires[outer]); for (size_t i = 0; i < wires.size(); ++i) { if (i == outer) continue; // Containment is checked, not assumed: a vertex of the inner wire must lie strictly @@ -775,11 +787,22 @@ TopoDS_Face SketchEngine::wires_to_face(const std::vector& wires, BRepClass_FaceClassifier fc(faces[outer], p, 1e-7); if (fc.State() != TopAbs_IN) throw std::runtime_error("sketch has two disjoint regions; put each in its own sketch"); - // A reversed wire tells OCCT this loop is a hole, not a second boundary. - fm.Add(TopoDS::Wire(wires[i].Reversed())); + // Add the hole loop AS-IS and let ShapeFix_Face sort the orientations out below. + // Reversing it here only works when the sketch happened to wind both loops the same + // way: a circle drawn clockwise inside a counter-clockwise rectangle comes out matching + // the outer boundary, OCCT sweeps it as a second contour, and the prism is the plate + // with the bore FILLED and the disc's volume counted twice. Measured on the rig: + // bbox 67.17 x 219.67 x 10 (the whole plate) with volume 152088 mm3 against a solid-box + // 147520 — a body larger than its own bounding box, which is the signature of it. + fm.Add(wires[i]); } if (!fm.IsDone()) throw std::runtime_error("sketch loop does not bound a face"); - return fm.Face(); + // Winding-independent classification of outer vs holes — the same idiom make_extrude_regions + // already uses for imported glyphs, which is why holed TEXT extruded correctly all along + // while a holed SKETCH did not. + ShapeFix_Face sff(fm.Face()); + sff.FixOrientation(); + return sff.Face(); } std::vector SketchEngine::mirror_entities( diff --git a/src/slic3r/GUI/DesignCanvas.cpp b/src/slic3r/GUI/DesignCanvas.cpp index 500dba8ad3..05de5abac2 100644 --- a/src/slic3r/GUI/DesignCanvas.cpp +++ b/src/slic3r/GUI/DesignCanvas.cpp @@ -112,6 +112,9 @@ DesignCanvas::DesignCanvas(wxWindow* parent) m_sketch_tool.on_inline_dismiss = [this]() { if (m_inline_editor) m_inline_editor->cancel(); }; + m_sketch_tool.on_inline_commit = [this]() { + if (m_inline_editor) m_inline_editor->commit(); + }; // Bottom-right viewport HUD: a borderless, non-focusable float label showing the active // tool's current values. Top-level (a child widget is hidden by the GL surface, same as @@ -236,6 +239,18 @@ void DesignCanvas::request_repaint() return; m_canvas->set_as_dirty(); + // NOTHING may touch GL before the canvas has initialised it. The backend probe below calls + // OpenGLManager::get_gl_info().get_renderer(), which runs GLInfo::detect() -> glGetString + // with no context current and, before init_opengl(), no loaded function pointers — a + // segfault at startup with no window and nothing in the log. Anything that asks for a + // repaint while the panel is still being built lands here, so the guard belongs at the top + // rather than around the render() call: the crash was in the PROBE, not in the paint. + if (!m_canvas->is_initialized()) { + if (m_canvas_widget) + m_canvas_widget->Refresh(); // the first real paint draws the current state anyway + return; + } + if (m_sw_gl < 0) { // Cache the backend once it's known; the renderer string is empty until // GL is initialised, so stay "unknown" and take the safe direct path till then. @@ -274,7 +289,7 @@ void DesignCanvas::reload(bool keep_view) for (int i = 0; i < (int)m_model.objects.size(); ++i) m_canvas->load_object(m_model, i); - const ColorRGBA sel_gold(0.40f, 0.82f, 1.0f, 1.0f); // cyan tint = solid selected + const ColorRGBA sel_gold = design_selection_color(); // same colour as every other selection const ColorRGBA ghost(0.26f, 0.66f, 1.0f, 0.45f); const auto& volumes = m_canvas->get_volumes().volumes; @@ -553,7 +568,7 @@ void DesignCanvas::set_on_sketch_selection_changed(std::function cb) m_sketch_tool.on_selection_changed = std::move(cb); } -void DesignCanvas::set_on_sketch_face_selected(std::function cb) +void DesignCanvas::set_on_sketch_face_selected(std::function cb) { m_sketch_tool.on_face_selected = std::move(cb); } @@ -583,6 +598,12 @@ void DesignCanvas::clear_loop_pick() m_sketch_tool.clear_display_pick(); } +void DesignCanvas::set_loop_pick(int feature, int region) +{ + m_sketch_tool.set_display_pick(feature, region); + request_repaint(); +} + void DesignCanvas::set_solid_pick(const std::vector* bodies, const TriangleMesh* mesh, const std::vector* tri_face, const std::vector* tri_body, const std::vector* visible, @@ -1146,6 +1167,11 @@ bool DesignCanvas::inline_busy() const return m_sketch_tool.inline_busy(); } +bool DesignCanvas::live_sketch_has_work() const +{ + return m_sketch_tool.live_sketch_has_work(); +} + bool DesignCanvas::undo_last_sketch_entity() { const bool did = m_sketch_tool.undo_last_entity(); diff --git a/src/slic3r/GUI/DesignCanvas.hpp b/src/slic3r/GUI/DesignCanvas.hpp index 657cddd924..74c238c365 100644 --- a/src/slic3r/GUI/DesignCanvas.hpp +++ b/src/slic3r/GUI/DesignCanvas.hpp @@ -78,12 +78,13 @@ public: // Sketch selection (Mode::Select). void set_on_sketch_selection_changed(std::function cb); - void set_on_sketch_face_selected(std::function cb); // closed loop clicked + void set_on_sketch_face_selected(std::function cb); // closed loop clicked: region index passed void set_on_display_sketch_selected(std::function cb); // committed loop clicked: (feature, region) void set_on_display_sketch_activated(std::function cb); // committed sketch DOUBLE-clicked: edit it std::vector selected_loop_entities() const; // entities of the click-selected loop std::vector> region_entity_indices(const std::vector& ents) const; void clear_loop_pick(); // drop the click-selected loop highlight (e.g. after extrude) + void set_loop_pick(int feature, int region); // adopt a loop pick made before the commit // Solid whole/face/edge selection: point the tool at the bodies + concatenated // tessellation (with per-triangle face & body ids), and a callback fired on each // whole->face->edge cycle (level, body index, face id, edge id). @@ -211,6 +212,7 @@ public: void set_on_context_menu(std::function cb); void delete_selected_sketch_entities(); bool inline_busy() const; // a sketch value field is open (guard keys) + bool live_sketch_has_work() const; // the live sketch holds entities a cancel would destroy bool undo_last_sketch_entity(); // Ctrl+Z in a sketch: drop the last entity bool delete_selected_or_last_sketch_entity(); // Delete in a sketch: selected, else last void clear_sketch_selection(); diff --git a/src/slic3r/GUI/DesignPanel.cpp b/src/slic3r/GUI/DesignPanel.cpp index 1a48dc6581..e62d2b270c 100644 --- a/src/slic3r/GUI/DesignPanel.cpp +++ b/src/slic3r/GUI/DesignPanel.cpp @@ -3321,7 +3321,7 @@ DesignPanel::DesignPanel(wxWindow* parent) // Onshape flow: clicking inside a closed-loop face commits the sketch and opens // the Extrude dialog (with a ghost preview) targeting that sketch. - m_viewport->set_on_sketch_face_selected([this]() { + m_viewport->set_on_sketch_face_selected([this](int region) { if (!m_viewport) return; m_viewport->finish_sketch(); // commit live sketch (synchronous) m_extrude_sketch_ref = resolve_extrude_sketch(); @@ -3333,6 +3333,13 @@ DesignPanel::DesignPanel(wxWindow* parent) } set_ui_mode(UiMode::Feature); open_tool(Tool::Extrude); + // AFTER open_tool, not before: opening the tool re-derives the selection state, so a + // region recorded ahead of it is wiped before Extrude ever reads it. + m_sel_sketch_feat = m_extrude_sketch_ref; + m_sel_sketch_region = region; + m_sel_solid_face = m_sel_solid_edge = -1; + m_pick_face = m_pick_face_body = -1; + m_viewport->set_loop_pick(m_extrude_sketch_ref, region); m_status->SetForegroundColour(wxNullColour); set_status(_L("Face selected — set the depth and Confirm")); m_status->Refresh(); @@ -3820,6 +3827,22 @@ DesignPanel::DesignPanel(wxWindow* parent) // Tool shortcuts (Onshape-style). While a sketch is open, single letters drive sketch // tools; otherwise Shift+letter drives feature tools and single letters drive view // toggles / section. Ctrl-combos and focused text fields are never intercepted. + // A SKETCH SHORTCUT MAY LEAVE AN OPEN VALUE FIELD. in_text is true while an inline + // dimension editor is up, and drawing a rectangle opens one automatically for its + // Width/Height — so after a rectangle every single-letter tool key was swallowed and the + // sketch could not be continued at all. A value field holds a NUMBER; a letter is never + // meant for it, so a letter that names a sketch tool is unambiguously a tool switch. + // set_tool() commits the field on the way through, so the typed value is kept. + // Deliberately narrow: sketch mode only, only keys that are actually bound, and only the + // in-canvas field — a focused wxTextCtrl elsewhere (the variables table, a card spin) + // still keeps every key. + const bool inline_field_only = (dynamic_cast(wxWindow::FindFocus()) == nullptr) + && m_viewport && m_viewport->inline_busy(); + if (in_text && inline_field_only && sketch_mode && !ctrl) { + const int up2 = (key >= 'a' && key <= 'z') ? key - 'a' + 'A' : key; + auto it2 = m_keys_sketch.find(up2); + if (it2 != m_keys_sketch.end()) { it2->second(); return; } + } if (!in_text && !ctrl) { const int up = (key >= 'a' && key <= 'z') ? key - 'a' + 'A' : key; // normalise case if (sketch_mode) { @@ -4525,6 +4548,10 @@ void DesignPanel::on_add_extrude() } else if (extrude_uses_loop()) { // Extrude just the selected loop (its entity subset), leaving the source sketch's // other loops intact and still selectable. + if (::getenv("SNAPORCA_PICK_TRACE")) + std::fprintf(stderr, "[pick] on_add_extrude: feat=%d reg=%d ents=%zu\n", + m_extrude_sketch_ref, m_sel_sketch_region, + m_viewport->selected_loop_entities().size()); idx = m_doc.add_extrude_entities(m_viewport->selected_loop_entities(), m_doc.features[m_extrude_sketch_ref].plane, m_distance->GetValue(), false, mode, name); @@ -10244,6 +10271,18 @@ void DesignPanel::tool_cancel() if (m_active == Tool::Insert) { cancel_insert(); return; } if (m_active != Tool::None) { cancel_tool(); return; } if (m_ui_mode == UiMode::Sketch) { + // Escape is how anyone dismisses the inline dimension field, and it used to cascade + // straight through to here: first press disarmed the tool, second press dropped the + // whole live session — a drawn rectangle gone, silently, with no undo prompt. That is + // the "I cannot add the circle after the rectangle" report: the sketch was already + // destroyed. Discarding real work needs the explicit Cancel button, not a key people + // press to close a text field. + if (m_viewport && m_viewport->live_sketch_has_work()) { + m_status->SetForegroundColour(wxNullColour); + set_status(_L("Sketch kept — use Confirm to keep it, Cancel to discard")); + m_status->Refresh(); + return; + } if (m_viewport) m_viewport->cancel_sketch(); // drop the live session (committed art stays) m_edit_index = -1; set_ui_mode(UiMode::Feature); diff --git a/src/slic3r/GUI/DesignSketchTool.cpp b/src/slic3r/GUI/DesignSketchTool.cpp index 17493c57e7..b696703c06 100644 --- a/src/slic3r/GUI/DesignSketchTool.cpp +++ b/src/slic3r/GUI/DesignSketchTool.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -216,6 +217,15 @@ void DesignSketchTool::set_tool(Mode mode) // op_ready()==0 with a=0 b=3 val=28.205 sitting right there — picked, valued, and dropped. if (op_ready()) confirm_op(); + // An OPEN inline value field freezes the canvas (on_mouse_impl returns early while + // m_awaiting_length) and blocks every keyboard shortcut (in_text includes inline_busy()). + // Drawing a rectangle opens one automatically for its Width/Height, so after a rectangle the + // sketch was STUCK: pressing C did nothing because the key never reached this function, and + // clicking on the canvas did nothing because the canvas was frozen. Committing here accepts + // the typed value and closes the field, which is the same rule the ready-edit-op above + // follows — leaving a tool must not silently discard what the user entered. + if (on_inline_commit) on_inline_commit(); + // Switch the active drawing tool without dropping accumulated entities. m_mode = mode; m_points.clear(); @@ -2779,11 +2789,22 @@ DesignSketchTool::region_entity_indices(const std::vector& ents) c // so there was no way to select it and therefore none to edit or delete it. Whether a stroke // bounds a region has nothing to do with whether the user can point at it. Region membership // decides what a hit REPORTS, not whether the hit can happen. +static void dp_pick_trace(const char* fmt, ...); // defined below; used by the diagnostics here void DesignSketchTool::hit_display_sketch(const DisplaySketch& d, const Vec2d& p, double tol, int& edge_feat, int& edge_reg, int& edge_ent, double& edge_d, int& face_feat, int& face_reg) const { const std::vector loops = region_loops(d.entities); + { // TEMPORARY DIAGNOSTIC: what did the sketch decompose into, and what is under the click? + std::string h; + for (size_t r = 0; r < loops.size(); ++r) { + h += " loop" + std::to_string(r) + "(ents=" + std::to_string(loops[r].ents.size()) + + ",poly=" + std::to_string(loops[r].poly.size()) + + ",holes=" + std::to_string(loops[r].holes.size()) + ")"; + } + dp_pick_trace("sketch feat=%d entities=%zu loops=%zu:%s", d.feature, d.entities.size(), + loops.size(), h.c_str()); + } std::vector ent_region(d.entities.size(), -1); for (int r = 0; r < int(loops.size()); ++r) for (int ei : loops[r].ents) @@ -2808,6 +2829,8 @@ void DesignSketchTool::hit_display_sketch(const DisplaySketch& d, const Vec2d& p if (h >= 0 && h < int(loops.size()) && point_in_poly(p, loops[h].poly)) { in_hole = true; break; } if (!in_hole) { face_feat = d.feature; face_reg = r; } } + dp_pick_trace("region hit -> feat=%d reg=%d (edge_feat=%d edge_reg=%d)", + face_feat, face_reg, edge_feat, edge_reg); } std::vector DesignSketchTool::selected_loop_entities() const @@ -3270,7 +3293,7 @@ void DesignSketchTool::render_solid_sel(SolidSel kind, int body, int face, // Called from render() while no sketch session is active. void DesignSketchTool::render_solid_highlight() { - const ColorRGBA sel_cyan(0.20f, 0.85f, 1.0f, 1.0f); + const ColorRGBA sel_cyan = design_selection_color(); // The pre-highlight goes FIRST so the committed selection paints over it where the two // overlap — what you HAVE outranks what you would get. Suppressed entirely when they are the @@ -3285,7 +3308,7 @@ void DesignSketchTool::render_solid_highlight() // Desaturated toward white rather than a second hue: a distinct colour would read as a // distinct KIND of selection, when it is the same selection one moment earlier. render_solid_sel(m_pre.kind, m_pre.body, m_pre.face, m_pre.edge_pts, m_pre.vertex_pt, - ColorRGBA(0.62f, 0.84f, 0.92f, 1.0f), 0.45f); + design_selection_color(), 0.45f); // hover = the same colour, quieter render_solid_sel(m_solid_sel, m_sel_body, m_sel_face, m_sel_edge_pts, m_sel_vertex_pt, sel_cyan, 1.0f); @@ -5837,7 +5860,10 @@ DesignSketchTool::region_loops(const std::vector& ents) const int DesignSketchTool::region_at(const Vec2d& p) const { - const std::vector> regions = closed_regions(); + // Walk region_loops (which already knows about holes) rather than the raw polygons: the + // returned index must stay meaningful after the sketch is committed, so it has to use the + // SAME numbering selected_loop_entities() and hit_display_sketch consume. + const std::vector regions = region_loops(m_entities); auto inside = [](const Vec2d& q, const std::vector& poly) { bool in = false; for (size_t i = 0, j = poly.size() - 1; i < poly.size(); j = i++) { @@ -5849,9 +5875,30 @@ int DesignSketchTool::region_at(const Vec2d& p) const } return in; }; - for (size_t i = 0; i < regions.size(); ++i) - if (inside(p, regions[i])) return int(i); - return -1; + // Shoelace area (absolute): the SMALLEST loop containing p is the innermost one, which is + // the region that actually owns the point — a bore inside a plate resolves to the disc, and + // a click on the plate material resolves to the plate even though the disc is also inside it. + auto poly_area = [](const std::vector& q) { + double a2 = 0.0; + for (size_t i = 0, j = q.size() - 1; i < q.size(); j = i++) + a2 += (q[j].x() + q[i].x()) * (q[j].y() - q[i].y()); + return std::abs(a2) * 0.5; + }; + int best = -1; + double best_area = 0.0; + for (size_t i = 0; i < regions.size(); ++i) { + if (regions[i].poly.size() < 3) continue; + if (!inside(p, regions[i].poly)) continue; + bool in_hole = false; // p inside one of this region's holes → that hole owns it, not us + for (int h : regions[i].holes) { + if (h < 0 || h >= int(regions.size()) || regions[h].poly.size() < 3) continue; + if (inside(p, regions[h].poly)) { in_hole = true; break; } + } + if (in_hole) continue; + const double a = poly_area(regions[i].poly); + if (best < 0 || a < best_area) { best = int(i); best_area = a; } + } + return best; } // ---- rendering -------------------------------------------------------------- @@ -5953,7 +6000,78 @@ std::vector> ear_clip(const std::vector& poly) } } // namespace -// Triangulate a closed boundary polygon and render it as a (blended) filled face. +// Fill a region that has holes with an EVEN-ODD SCANLINE fill: each horizontal band is scanned +// across every contour, the crossings sorted, and the spans between alternate crossings emitted +// as quads. A hole is just two more crossings, so any number of holes and any concavity fall out +// of the parity rule — no bridge and no triangulation to fail. +void DesignSketchTool::draw_fill_holed(GLModel& model, const std::vector& outer, + const std::vector>& holes, + const ColorRGBA& color) +{ + if (outer.size() < 3) return; + if (holes.empty()) { draw_fill(model, outer, color); return; } + + // EVEN-ODD SCANLINE, not a triangulation. The previous version spliced each hole into the + // outer contour through a keyhole corridor and ear-clipped the result; the corridor did not + // collapse to zero width and was drawn as a visible triangle running from the bore to the + // nearest rectangle corner. Rather than tune a bridge that can always find a new shape to + // fail on, this fills the region the way a rasteriser would: for each horizontal band, cross + // EVERY contour, sort the crossings, and fill between alternate pairs. A hole is simply two + // more crossings, so any number of holes and any concavity fall out of the same rule and no + // corridor exists to leak. + std::vector*> contours; + contours.push_back(&outer); + for (const auto& h : holes) if (h.size() >= 3) contours.push_back(&h); + + double ymin = outer[0].y(), ymax = ymin, xmin = outer[0].x(), xmax = xmin; + for (const auto* c : contours) + for (const Vec2d& v : *c) { + ymin = std::min(ymin, v.y()); ymax = std::max(ymax, v.y()); + xmin = std::min(xmin, v.x()); xmax = std::max(xmax, v.x()); + } + if (ymax - ymin < 1e-9) return; + + // Enough bands that the step is far below a pixel at any sane zoom, cheap enough to rebuild + // every frame: this is a translucent highlight, not geometry. + const int ROWS = 256; + const double dy = (ymax - ymin) / ROWS; + + GLModel::Geometry g; + g.format = { GLModel::Geometry::EPrimitiveType::Triangles, GLModel::Geometry::EVertexLayout::P3 }; + unsigned base = 0; + std::vector xs; + for (int row = 0; row < ROWS; ++row) { + const double y0 = ymin + dy * row, y1 = y0 + dy, ym = 0.5 * (y0 + y1); + xs.clear(); + for (const auto* c : contours) { + const std::vector& q = *c; + for (size_t i = 0, j = q.size() - 1; i < q.size(); j = i++) { + const Vec2d& A = q[i]; const Vec2d& B = q[j]; + if ((A.y() > ym) == (B.y() > ym)) continue; // edge does not cross this row + xs.push_back(A.x() + (ym - A.y()) * (B.x() - A.x()) / (B.y() - A.y())); + } + } + if (xs.size() < 2) continue; + std::sort(xs.begin(), xs.end()); + for (size_t k = 0; k + 1 < xs.size(); k += 2) { // even-odd: fill alternate spans + const double xa = xs[k], xb = xs[k + 1]; + if (xb - xa < 1e-9) continue; + g.add_vertex((Vec3f)m_plane.to_world(Vec2d(xa, y0)).cast()); + g.add_vertex((Vec3f)m_plane.to_world(Vec2d(xb, y0)).cast()); + g.add_vertex((Vec3f)m_plane.to_world(Vec2d(xb, y1)).cast()); + g.add_vertex((Vec3f)m_plane.to_world(Vec2d(xa, y1)).cast()); + g.add_triangle(base, base + 1, base + 2); + g.add_triangle(base, base + 2, base + 3); + base += 4; + } + } + if (base == 0) return; + model.reset(); + model.init_from(std::move(g)); + model.set_color(color); + model.render(); +} + void DesignSketchTool::draw_fill(GLModel& model, const std::vector& poly, const ColorRGBA& color) { if (poly.size() < 3) return; @@ -7576,10 +7694,16 @@ void DesignSketchTool::render(GLCanvas3D& canvas) // extrude is removed): faces translucent, outlines orange. Each uses its own plane. if (!m_display_sketches.empty()) { const SketchPlane saved_plane = m_plane; - const ColorRGBA dface(0.30f, 0.60f, 1.0f, 0.16f); // normal translucent face - const ColorRGBA sface(0.30f, 0.80f, 1.0f, 0.34f); // click-selected loop: brighter cyan + // CYAN/BLUE MEANS SELECTED — nothing else may wear it. The unselected fill used to be + // (0.30,0.60,1.0), one shade off the selected (0.30,0.80,1.0), so an ordinary region + // read as picked and a picked one added nothing. The outlines already got this right: + // orange for a sketch, cyan for the selection. The fill now follows the same logic, so + // an unselected region is faint amber — the sketch's own colour — and every blue thing + // on screen is something you selected. + const ColorRGBA dface = design_idle_face_color(); // unselected: neutral grey, never the selection colour + const ColorRGBA sface = design_selection_color(0.34f); // selected region const ColorRGBA dwire(1.0f, 0.55f, 0.1f, 1.0f); // normal orange outline - const ColorRGBA swire(0.30f, 0.85f, 1.0f, 1.0f); // click-selected loop: cyan outline + const ColorRGBA swire = design_selection_color(); // selected outline glsafe(::glEnable(GL_BLEND)); glsafe(::glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA)); for (const DisplaySketch& ds : m_display_sketches) { @@ -7590,7 +7714,10 @@ void DesignSketchTool::render(GLCanvas3D& canvas) const ColorRGBA* hlc = sketch_hl_color(ds.feature); ColorRGBA fc = sel ? sface : dface; if (hlc && !sel) { fc = *hlc; fc.a(0.20f); } - draw_fill(m_fill_model, loops[r].poly, fc); + std::vector> hp; + for (int h : loops[r].holes) + if (h >= 0 && h < int(loops.size())) hp.push_back(loops[h].poly); + draw_fill_holed(m_fill_model, loops[r].poly, hp, fc); } } glsafe(::glDisable(GL_BLEND)); @@ -7718,13 +7845,28 @@ void DesignSketchTool::render(GLCanvas3D& canvas) // Closed loops fill as translucent faces (the "closed loop = selectable face" // affordance). Drawn first so the entity outlines paint over the fill. { - const std::vector> regions = closed_regions(); - if (!regions.empty()) { + // region_loops(), NOT closed_regions(): the latter returns raw polygons with no notion + // of nesting, so a circle drawn inside a rectangle was filled as its own solid disc on + // top of a solid rectangle. That is why a live sketch still showed a filled blue circle + // however often the COMMITTED renderer below was corrected — these are two separate + // renderers and only one of them had been taught about holes. + const std::vector loops = region_loops(m_entities); + if (!loops.empty()) { glsafe(::glEnable(GL_BLEND)); glsafe(::glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA)); - const ColorRGBA face(0.30f, 0.60f, 1.0f, 0.22f); - for (const std::vector& r : regions) - draw_fill(m_fill_model, r, face); + // A bore is not a face. Skip loops that are somebody's hole, and cut those holes out + // of the region that owns them, so a plate with a hole LOOKS like one while drawing. + std::vector is_hole(loops.size(), 0); + for (const RegionLoop& L : loops) + for (int h : L.holes) + if (h >= 0 && h < int(is_hole.size())) is_hole[h] = 1; + for (size_t r = 0; r < loops.size(); ++r) { + if (is_hole[r]) continue; + std::vector> hp; + for (int h : loops[r].holes) + if (h >= 0 && h < int(loops.size())) hp.push_back(loops[h].poly); + draw_fill_holed(m_fill_model, loops[r].poly, hp, design_idle_face_color()); + } glsafe(::glDisable(GL_BLEND)); } } @@ -8723,6 +8865,7 @@ bool DesignSketchTool::on_mouse_impl(wxMouseEvent& evt, GLCanvas3D& canvas) const Linef3 ray8 = canvas.mouse_ray(Point(evt.GetX() + 8, evt.GetY())); int edge_feat = -1, edge_reg = -1, edge_ent = -1; double edge_d = 1e30; // nearest stroke int face_feat = -1, face_reg = -1; // interior (fallback) + dp_pick_trace("display sketches available: %zu", m_display_sketches.size()); for (const DisplaySketch& d : m_display_sketches) { const Vec2d p = d.plane.project(ray.a, ray.vector()); const Vec2d p8 = d.plane.project(ray8.a, ray8.vector()); @@ -9239,11 +9382,14 @@ bool DesignSketchTool::on_mouse_impl(wxMouseEvent& evt, GLCanvas3D& canvas) } else if (!extend) { // Inside a closed loop (not on an edge/point) → select it as a face // and hand off to the panel, which commits the sketch and extrudes. - if (evt.LeftDown() && on_face_selected && region_at(p) >= 0) { - m_selection.clear(); - m_point_sel.clear(); - on_face_selected(); - return true; + if (evt.LeftDown() && on_face_selected) { + const int reg = region_at(p); + if (reg >= 0) { + m_selection.clear(); + m_point_sel.clear(); + on_face_selected(reg); + return true; + } } m_selection.clear(); // clicked empty space m_point_sel.clear(); @@ -9253,8 +9399,12 @@ bool DesignSketchTool::on_mouse_impl(wxMouseEvent& evt, GLCanvas3D& canvas) return true; } if (evt.RightDown()) { + // Merely dropping a selection is not a gesture terminator: the m_right_consumed flag + // this return value feeds means "the tool USED this right-click", and suppressing the + // offer menu on a plain deselection would leave no way to reach the right-click menu + // again once any geometry exists. So clear, but hand the click back. clear_selection(); - return true; + return false; } return false; // let drag orbit the camera } diff --git a/src/slic3r/GUI/DesignSketchTool.hpp b/src/slic3r/GUI/DesignSketchTool.hpp index a43f9360fa..4c19937199 100644 --- a/src/slic3r/GUI/DesignSketchTool.hpp +++ b/src/slic3r/GUI/DesignSketchTool.hpp @@ -30,6 +30,23 @@ class Camera; // fwd — move_gizmo_arm() sizes the gizmo from the current z // accumulate. `finish` commits the whole entity list as one sketch feature; // `cancel` aborts. Constrain is a separate legacy mode that operates on a // committed profile's points (entity constraints land in a later chunk). + + +// ONE colour means SELECTED — a face, an edge, a vertex, a whole body, a 2D sketch region. +// Nothing else on screen may wear it. Before this there were four near-identical cyans plus a +// constant still named sel_gold that had long since become cyan, and the UNSELECTED region fill +// was blue (0.30,0.60,1.0) — one shade from the selected one — so an ordinary region read as +// picked. Selection is a state, not a decoration: it gets its own colour and keeps it. +inline ColorRGBA design_selection_color(float alpha = 1.0f) +{ + return ColorRGBA(0.20f, 0.85f, 1.00f, alpha); +} +// Unselected geometry — 2D regions and faces — is neutral translucent grey, so the only +// coloured thing in the viewport is the thing you picked. +inline ColorRGBA design_idle_face_color() +{ + return ColorRGBA(0.72f, 0.76f, 0.80f, 0.14f); +} class DesignSketchTool { public: enum class Mode { Select, Dimension, Polyline, Line, CornerRect, CenterRect, ObliqueRect, @@ -70,6 +87,9 @@ public: // (Line's existing freeze flag) as the single "inline editor open" gate. void set_inline_busy(bool b) { m_awaiting_length = b; } bool inline_busy() const { return m_awaiting_length; } // true while a value field is open + // Does the live session hold anything a cancel would throw away? Escape must not silently + // destroy drawn geometry; the panel asks this before treating Escape as "discard sketch". + bool live_sketch_has_work() const { return !m_entities.empty(); } bool constrain_value_anchor(wxPoint& out) const; // screen anchor over the picked constrain geometry void begin(const SketchPlane& plane, Mode mode = Mode::Polyline); @@ -179,6 +199,11 @@ public: // loops from the committed-sketch overlay). std::vector> region_entity_indices(const std::vector& ents) const; void clear_display_pick() { m_display_pick = -1; m_display_pick_region = -1; } + // Adopt a loop pick the tool did not make itself. The live-sketch path resolves the region + // BEFORE the sketch is committed, so once finish_sketch() has turned it into a display + // sketch there is nothing left that would set this — and selected_loop_entities(), which is + // what Extrude consumes, reads exactly these two fields. + void set_display_pick(int feature, int region) { m_display_pick = feature; m_display_pick_region = region; } // Visual Extrude gizmo (C5b). The Extrude tool is a DesignPanel docked card, so the // sketch tool is NOT active during it; the panel feeds the profile plane + a 2D centroid @@ -465,6 +490,9 @@ public: // Force-close any open inline field (runs its cancel = keep-as-drawn). Used by the polyline // terminators (right-click / double-click) to end the chain even mid per-segment edit. std::function on_inline_dismiss; + // Accept and close an open inline value field. dismiss() CANCELS; this one keeps the value, + // which is what leaving a tool should do — see set_tool(). + std::function on_inline_commit; // Bottom-right viewport readout: emitted each frame with the active tool's current // values (live segment length/angle while drawing a line, or the selected entity's @@ -485,7 +513,7 @@ public: std::function on_commit; // Emitted when a closed-loop face is clicked in Select mode (Onshape: a region // becomes a selectable face → extrude). The panel commits the sketch + extrudes. - std::function on_face_selected; + std::function on_face_selected; // region index into region_loops(m_entities) // Esc pressed while the tool is active: exit/cancel the session (the panel restores // Feature mode). Layered: an in-progress entity or a non-Select draw tool is dropped // first; a second Esc exits the session. @@ -797,6 +825,10 @@ private: void draw_vertices(GLModel& model, const std::vector& pts, const ColorRGBA& color, double half_size = 1.3); void draw_fill(GLModel& model, const std::vector& poly, const ColorRGBA& color); + // Same, with the region's holes cut out, so a selected plate-with-a-hole is drawn as an + // ANNULUS instead of a filled rectangle painted straight across its own bore. + void draw_fill_holed(GLModel& model, const std::vector& outer, + const std::vector>& holes, const ColorRGBA& color); const ColorRGBA* sketch_hl_color(int feature) const; bool m_active{false}; diff --git a/src/slic3r/GUI/SketchInlineEditor.cpp b/src/slic3r/GUI/SketchInlineEditor.cpp index 4981c9643c..ce4cda616d 100644 --- a/src/slic3r/GUI/SketchInlineEditor.cpp +++ b/src/slic3r/GUI/SketchInlineEditor.cpp @@ -137,6 +137,13 @@ void SketchInlineEditor::cancel() if (m_open) do_cancel(); } +// Accept what is typed and close. Leaving a tool must not silently discard the value the user +// just entered — the same rule set_tool already follows for a ready edit-op. +void SketchInlineEditor::commit() +{ + if (m_open) do_commit(); +} + void SketchInlineEditor::do_cancel() { if (!m_open) return; diff --git a/src/slic3r/GUI/SketchInlineEditor.hpp b/src/slic3r/GUI/SketchInlineEditor.hpp index 0ddec212d2..e3f7670e5e 100644 --- a/src/slic3r/GUI/SketchInlineEditor.hpp +++ b/src/slic3r/GUI/SketchInlineEditor.hpp @@ -31,6 +31,7 @@ public: std::function on_cancel); void close(); void cancel(); // if open, run the registered cancel (keep-as-drawn) + void commit(); // if open, run the registered commit (accept the typed value) bool is_open() const { return m_open; } private: diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index 19d79992a0..2fced4bbf2 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -7802,3 +7802,109 @@ TEST_CASE("an invalid pair names which side is wrong", "[CadDocument][mate]") for (const auto& o : doc.mate_options(-5, a)) REQUIRE_CONTAINS(o.reason, "connector A"); } + +// A rectangular plate 120 x 160 x 10 centred on the origin with a radius-30 bore through +// the middle: one solid whose volume is the box minus the cylinder, and whose topology is a +// plate-with-a-bore (1 solid, 7 faces) rather than a plate plus a plug. +TEST_CASE("add_extrude_entities builds a plate with a bore", "[CadDocument][holes]") +{ + using Catch::Matchers::WithinAbs; + + CadDocument doc; + + const Vec2d A(-60, -80), B(60, -80), C(60, 80), D(-60, 80); + std::vector entities = { + {SketchEntity::Type::Line, A, B}, + {SketchEntity::Type::Line, B, C}, + {SketchEntity::Type::Line, C, D}, + {SketchEntity::Type::Line, D, A}, + }; + SketchEntity bore; + bore.type = SketchEntity::Type::Circle; + bore.center = Vec2d(0, 0); + bore.radius = 30.0; + entities.push_back(bore); + + doc.add_extrude_entities(entities, SketchPlane::XY(), 10.0, false, BooleanMode::New, "Extrude1"); + REQUIRE(doc.recompute()); + REQUIRE(doc.error.empty()); + REQUIRE(doc.display_mesh.facets_count() > 0); + + auto mp = doc.body_mass_properties(0); + REQUIRE(mp.valid); + const double expected_vol = 120.0 * 160.0 * 10.0 - M_PI * 30.0 * 30.0 * 10.0; + REQUIRE_THAT(mp.volume, WithinAbs(expected_vol, 1.0)); + + // One solid, seven faces: top + bottom (each a planar face with a hole), four side + // walls, and the single cylindrical bore wall. + int face_count = 0, solid_count = 0; + for (TopExp_Explorer fe(doc.bodies.back().shape, TopAbs_FACE); fe.More(); fe.Next()) ++face_count; + for (TopExp_Explorer se(doc.bodies.back().shape, TopAbs_SOLID); se.More(); se.Next()) ++solid_count; + INFO("volume mm^3 = " << mp.volume << ", faces = " << face_count << ", solids = " << solid_count); + REQUIRE(solid_count == 1); + REQUIRE(face_count == 7); +} + +TEST_CASE("two disjoint circles are refused by name", "[CadDocument][holes]") +{ + CadDocument doc; + + SketchEntity a; + a.type = SketchEntity::Type::Circle; a.center = Vec2d(-50, 0); a.radius = 20.0; + SketchEntity b; + b.type = SketchEntity::Type::Circle; b.center = Vec2d( 50, 0); b.radius = 20.0; + + doc.add_extrude_entities({a, b}, SketchPlane::XY(), 10.0, false, BooleanMode::New, "Extrude1"); + + REQUIRE_FALSE(doc.recompute()); + REQUIRE_CONTAINS(doc.error, "disjoint"); +} + +// Same plate-with-a-bore, but the bore circle is wound CLOCKWISE (geometric winding opposite +// the CCW rectangle). The old wires_to_face reversed every hole wire unconditionally +// (wires[i].Reversed()), which only produced a correct hole when the circle was wound the same +// way as the outer loop; a clockwise circle got reversed into matching the outer boundary and +// the prism swept it solid — measured 220274 mm^3 = box (192000) + disc (28274), the filled-bore +// signature. The current ShapeFix_Face::FixOrientation path is winding-independent. Flipping the +// plane normal reverses gp_Circ's parametrisation (gp_Ax2(center, -Z) sweeps clockwise seen from +// +Z) while the rectangle's 2D coordinates stay CCW. +TEST_CASE("add_extrude_entities builds a plate with a bore (clockwise circle)", "[CadDocument][holes]") +{ + using Catch::Matchers::WithinAbs; + + CadDocument doc; + + SketchPlane cw_plane = SketchPlane::XY(); + cw_plane.normal = Vec3d(0, 0, -1); + + const Vec2d A(-60, -80), B(60, -80), C(60, 80), D(-60, 80); + std::vector entities = { + {SketchEntity::Type::Line, A, B}, + {SketchEntity::Type::Line, B, C}, + {SketchEntity::Type::Line, C, D}, + {SketchEntity::Type::Line, D, A}, + }; + SketchEntity bore; + bore.type = SketchEntity::Type::Circle; + bore.center = Vec2d(0, 0); + bore.radius = 30.0; + entities.push_back(bore); + + doc.add_extrude_entities(entities, cw_plane, 10.0, false, BooleanMode::New, "Extrude1"); + REQUIRE(doc.recompute()); + REQUIRE(doc.error.empty()); + REQUIRE(doc.display_mesh.facets_count() > 0); + + auto mp = doc.body_mass_properties(0); + REQUIRE(mp.valid); + const double expected_vol = 120.0 * 160.0 * 10.0 - M_PI * 30.0 * 30.0 * 10.0; + REQUIRE_THAT(mp.volume, WithinAbs(expected_vol, 1.0)); + + int face_count = 0, solid_count = 0; + for (TopExp_Explorer fe(doc.bodies.back().shape, TopAbs_FACE); fe.More(); fe.Next()) ++face_count; + for (TopExp_Explorer se(doc.bodies.back().shape, TopAbs_SOLID); se.More(); se.Next()) ++solid_count; + INFO("volume mm^3 = " << mp.volume << ", faces = " << face_count << ", solids = " << solid_count); + REQUIRE(solid_count == 1); + REQUIRE(face_count == 7); +} +