From 9f2b2bc511f3ecfa0b794c6d8b97837918d4a3e2 Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Fri, 31 Jul 2026 19:21:42 +0200 Subject: [PATCH] =?UTF-8?q?Design:=20sketch=20means=20a=20tool=20=E2=80=94?= =?UTF-8?q?=20the=20offer=20works=20inside=20a=20sketch,=20and=20the=20app?= =?UTF-8?q?=20stops=20contradicting=20itself=20about=20the=20plane?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from the rig: pick a plane, press Sketch, and you are told to pick a plane. The app prescribed a sequence and then refused to acknowledge that you had followed it. Right-click did nothing, so there was no way to reach a drawing tool except the toolbar this tab exists to retire. act_sketch was two lines: set the mode, then print "Click a face or a reference plane in the viewport, then a sketch tool" — unconditionally, without ever asking whether a plane was already chosen. The plane was never lost; m_ref_plane held it and begin_sketch captures it when the first tool is armed. The sentence was simply false. It now asks. sketch_plane_target() is a companion to sketch_plane_from_selection that distinguishes "the user chose XZ" from "nothing chosen, falling back to XY" — a distinction m_ref_plane cannot express on its own, being always a valid index, so m_plane_picked carries it. With a target the readout names it and the offer opens on the Create row; without one the old prompt stands, because then it is true. The card above the status line was a local wxStaticText that nothing could update, so it went on asking for a plane two inches from a line saying the plane was chosen. It is a member now and the two are written together. Right-click inside a sketch was excluded wholesale so that it could end a polyline chain, abandon an anchor, exit a tool. That made every sketch row in the atlas unreachable. The honest test is not which mode we are in but whether the tool actually USED this right-click, and only the tool knows: on_mouse now wraps on_mouse_impl and records that once, for every terminator, instead of threading a flag through the twenty-odd sites that consume a RightDown. The canvas read-and-clears it on the matching release. Underneath all of it was one confusion — MODE versus SESSION — at four sites. begin_sketch does not run until the first tool is armed, so is_sketching() is false for exactly the interval between "press Sketch" and "pick a tool", which is precisely when the drawing tools must be on offer. The keyboard learned this once already (snaporca-0ud, whose comment states the rule) and I reintroduced it in offer_selection_kind and again in show_offer_menu, where the offer built from the FEATURE map and rendered nine rows that all refused the sketch selection. Both now call sketch_map_applies(), so they cannot drift apart again. The keyboard keeps its own split: its "sketching" gates undo and delete-last-entity, which genuinely need a live session. Verified on :11 against a fresh build. Pick XZ, press Sketch: card reads "Drawing on XZ", status reads "Sketching on XZ — pick a tool", offer opens with Create and Reference live and the six rows needing geometry greyed. Right-click while idle opens the offer. Right-click as a terminator does NOT — the chain ends, the line lands on XZ, its length field arms at 49.36 mm. That last one is the regression the blanket exclusion was buying and the reason this shape of fix was chosen over a mode test. Refs snaporca-6vs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LyRwbuq6fjn3VV9U9UvhBM --- src/slic3r/GUI/DesignCanvas.cpp | 5 ++- src/slic3r/GUI/DesignPanel.cpp | 66 ++++++++++++++++++++++++++--- src/slic3r/GUI/DesignPanel.hpp | 12 ++++++ src/slic3r/GUI/DesignSketchTool.cpp | 14 ++++++ src/slic3r/GUI/DesignSketchTool.hpp | 6 +++ 5 files changed, 96 insertions(+), 7 deletions(-) diff --git a/src/slic3r/GUI/DesignCanvas.cpp b/src/slic3r/GUI/DesignCanvas.cpp index 68683896f6..3739e353d9 100644 --- a/src/slic3r/GUI/DesignCanvas.cpp +++ b/src/slic3r/GUI/DesignCanvas.cpp @@ -816,7 +816,10 @@ void DesignCanvas::set_on_context_menu(std::function cb) }); m_canvas_widget->Bind(wxEVT_RIGHT_UP, [this](wxMouseEvent& e) { const wxPoint d = e.GetPosition() - m_ctx_press; - if (m_on_context_menu && !is_sketching() && !inline_busy() + // Always read-and-clear, even when another guard already rules the offer out, or a + // terminator recorded under one condition would still be pending under the next. + const bool terminated = m_sketch_tool.take_right_consumed(); + if (m_on_context_menu && !terminated && !inline_busy() && std::max(std::abs(d.x), std::abs(d.y)) <= 8) { m_on_context_menu(m_canvas_widget->ClientToScreen(e.GetPosition())); return; // consumed diff --git a/src/slic3r/GUI/DesignPanel.cpp b/src/slic3r/GUI/DesignPanel.cpp index f142059ee1..e4fd483f67 100644 --- a/src/slic3r/GUI/DesignPanel.cpp +++ b/src/slic3r/GUI/DesignPanel.cpp @@ -516,11 +516,33 @@ DesignPanel::DesignPanel(wxWindow* parent) }; auto* b_sketch = icon_btn("design_sketch", _L("Sketch")); + // Sketch means a tool. Pressing it used to set the mode and then ask, unconditionally, + // for the very thing the user had just done — click XZ, read "XZ plane selected, press + // Sketch", press Sketch, and be told to click a reference plane. The plane was never + // lost (m_ref_plane holds it and begin_sketch captures it when the first tool is armed); + // the sentence was simply false, and with right-click excluded in sketch mode there was + // no door to the tools at all, so the only way on was the toolbar this tab is retiring. std::function act_sketch = [this] { set_ui_mode(UiMode::Sketch); + wxString where; + const bool have_plane = sketch_plane_target(where); m_status->SetForegroundColour(wxNullColour); - m_status->SetLabel(_L("Click a face or a reference plane in the viewport, then a sketch tool")); + m_status->SetLabel(have_plane + ? wxString::Format(_L("Sketching on %s — pick a tool"), where) + : _L("Click a face or a reference plane in the viewport, then a sketch tool")); m_status->Refresh(); + if (m_sketch_hint) { // the card must agree with the status line, not argue with it + m_sketch_hint->SetLabel(have_plane + ? wxString::Format(_L("Drawing on %s.\nPick a tool, or right-click for the list."), where) + : _L("Click a face or a reference plane, then a sketch tool.")); + m_sketch_hint->Refresh(); + m_cards->Layout(); + } + // Hand over the tools rather than naming them in a status line. CallAfter so the + // mode change has settled before a modal menu takes the loop; the menu carries each + // tool's shortcut, so pressing the key instead of picking a row costs nothing. + if (have_plane) + CallAfter([this] { show_offer_menu(wxGetMousePosition()); }); }; b_sketch->Bind(wxEVT_BUTTON, [act_sketch](wxCommandEvent&) { act_sketch(); }); m_keys_feature[SHIFT('S')] = act_sketch; @@ -2681,10 +2703,13 @@ DesignPanel::DesignPanel(wxWindow* parent) // looking and pointing. A combo duplicated that decision somewhere the geometry could not // see it, and once a face could be picked it went further and displayed a stale row that // contradicted the real target. snaporca-e1p. - auto* hint = new wxStaticText(m_cards, wxID_ANY, + // Kept as a member, not a local: the card has to be able to STOP saying this. It asked + // for a plane even when one had just been picked, directly contradicting the status line + // two inches below it, which by then read "Sketching on XZ". + m_sketch_hint = new wxStaticText(m_cards, wxID_ANY, _L("Click a face or a reference plane, then a sketch tool.")); - hint->SetForegroundColour(dp_sec_text()); - m_box_sketch_session->Add(hint, 0, wxLEFT | wxRIGHT | wxTOP | wxBOTTOM, 12); + m_sketch_hint->SetForegroundColour(dp_sec_text()); + m_box_sketch_session->Add(m_sketch_hint, 0, wxLEFT | wxRIGHT | wxTOP | wxBOTTOM, 12); } cards->Add(m_box_sketch_session, 0, wxEXPAND); @@ -3291,6 +3316,7 @@ DesignPanel::DesignPanel(wxWindow* parent) // while the committed feature landed on the new one. if (base >= 0) { m_ref_plane = base; + m_plane_picked = true; // chosen, not merely defaulted to m_pick_face = m_pick_face_body = -1; // last pick wins: a plane beats a stale face if (m_viewport && m_viewport->is_sketching()) m_viewport->set_sketch_plane(plane_from_choice(m_ref_plane)); @@ -4890,6 +4916,25 @@ SketchPlane DesignPanel::sketch_plane_from_selection(wxString& what) const return plane_from_choice(m_ref_plane); } +// Is there a sketch target the USER chose, and what is it called? Distinct from +// sketch_plane_from_selection, which always answers because it falls back to m_ref_plane — +// a caller that wants to say "sketching on X" needs to know whether there is anything to fall +// back FROM, so it can ask for a plane instead of claiming one the user never picked. +bool DesignPanel::sketch_plane_target(wxString& what) const +{ + const int fb = (m_sel_solid_face >= 0 && m_sel_solid_body >= 0) ? m_sel_solid_body : m_pick_face_body; + const int fi = (m_sel_solid_face >= 0 && m_sel_solid_body >= 0) ? m_sel_solid_face : m_pick_face; + SketchPlane p; + if (fb >= 0 && fi >= 0 && m_doc.plane_of_face(fb, fi, p)) { + what = (m_doc.bodies.size() > 1) + ? wxString::Format(_L("the picked face of Body %d"), fb + 1) + : _L("the picked face"); + return true; + } + if (m_plane_picked) { what = ref_plane_name(m_ref_plane); return true; } + return false; +} + // --------------------------------------------------------------------------------------------- // The object-driven offer (charter §4.1). Right-click the geometry and get a vertical list in the // ratified row order, with the verbs that do not apply DISABLED IN PLACE carrying their reason. @@ -4897,6 +4942,11 @@ SketchPlane DesignPanel::sketch_plane_from_selection(wxString& what) const // mockups are drawn from, so a drawing and the product cannot drift apart. // --------------------------------------------------------------------------------------------- +bool DesignPanel::sketch_map_applies() const +{ + return m_ui_mode == UiMode::Sketch || (m_viewport && m_viewport->is_sketching()); +} + // Which kind of thing is selected, as an OfferSel. Classified by the level the pick cycle has // actually REACHED, so the menu describes what is highlighted — a header that names a face while // the whole body is lit would be lying, and this menu's whole value is that it tells the truth @@ -4904,7 +4954,7 @@ SketchPlane DesignPanel::sketch_plane_from_selection(wxString& what) const // sketch_plane_from_selection, which deliberately uses m_pick_face. snaporca-3a2.) int DesignPanel::offer_selection_kind() const { - if (m_viewport && m_viewport->is_sketching()) + if (sketch_map_applies()) return int(OfferSel::SkNone); const int nb = int(m_doc.bodies.size()); @@ -4959,7 +5009,11 @@ void DesignPanel::show_offer_menu(const wxPoint& screen_pos) { const int kind = offer_selection_kind(); const uint32_t bit = offer_bit(OfferSel(kind)); - const bool sketching = m_viewport && m_viewport->is_sketching(); + // Which verb MAP applies is a question about the MODE, not about whether a session is + // running — the same distinction the keyboard already had to learn (snaporca-0ud). Gated on + // is_sketching() the offer opened on entering a sketch showing the FEATURE rows, every one + // of them refusing the sketch selection, so it read as a menu of nine dead entries. + const bool sketching = sketch_map_applies(); const int bodies = int(m_doc.bodies.size()); int sketches = 0; diff --git a/src/slic3r/GUI/DesignPanel.hpp b/src/slic3r/GUI/DesignPanel.hpp index 3c345cb57b..bdaf60f2c5 100644 --- a/src/slic3r/GUI/DesignPanel.hpp +++ b/src/slic3r/GUI/DesignPanel.hpp @@ -235,6 +235,9 @@ private: // list: a picked planar face wins, otherwise the reference plane last clicked in 3D. `what` // comes back as something to show the user, so the choice is visible without a combo. SketchPlane sketch_plane_from_selection(wxString& what) const; + // Whether that resolution has anything the USER picked behind it, rather than the default + // reference plane. Lets a caller say "sketching on XZ" only when it is actually true. + bool sketch_plane_target(wxString& what) const; // True when Extrude should build only the click-selected loop (a region of the // resolved sketch is selected and it carries entities). bool extrude_uses_loop() const; @@ -328,6 +331,7 @@ private: // persists until Finish (Phase 3). wxSizer* m_box_sketch_session{nullptr}; wxStaticText* m_hdr_sketch_session{nullptr}; + wxStaticText* m_sketch_hint{nullptr}; // "click a plane" / "drawing on X" — must match the status wxStaticText* m_hdr_extrude{nullptr}; wxStaticText* m_hdr_dressup{nullptr}; wxStaticText* m_hdr_hole{nullptr}; @@ -397,6 +401,9 @@ private: // >=3 indexes resolve_datum_planes(). Set by CLICKING a ghost plane in the viewport — there is // deliberately no dropdown for it. snaporca-e1p. int m_ref_plane{0}; + // m_ref_plane is always a VALID plane, so it cannot itself distinguish "the user chose XY" + // from "nobody has chosen anything yet". This does. + bool m_plane_picked{false}; ComboBox* m_shape{nullptr}; ComboBox* m_plane{nullptr}; ComboBox* m_mode{nullptr}; @@ -611,6 +618,11 @@ private: std::map> m_verb_actions; void show_offer_menu(const wxPoint& screen_pos); int offer_selection_kind() const; // an OfferSel, as int to keep the header light + // Does the SKETCH half of the map apply? A mode question, not a session one: begin_sketch + // does not run until the first tool is armed, so between "press Sketch" and "pick a tool" + // is_sketching() is still false — precisely when the drawing tools must be on offer. The + // is_sketching() arm covers re-opening a committed sketch, which enters the session first. + bool sketch_map_applies() const; void run_offer_action(const char* action); // Face-as-profile extrude (Onshape): when Extrude is opened on a picked solid face with // no sketch source, this carries that global face id so the kernel extrudes the face. diff --git a/src/slic3r/GUI/DesignSketchTool.cpp b/src/slic3r/GUI/DesignSketchTool.cpp index bb214f6151..038cedc303 100644 --- a/src/slic3r/GUI/DesignSketchTool.cpp +++ b/src/slic3r/GUI/DesignSketchTool.cpp @@ -7563,7 +7563,21 @@ std::vector DesignSketchTool::connected_loop(int seed) const return out; } +// Right-click has two jobs in a sketch, and they were resolved by giving one of them everything: +// the offer was excluded in sketch mode wholesale so a right-click could end a polyline chain, +// abandon an anchor or exit a tool. That made every sketch row in the atlas unreachable. +// The honest test is not "which mode are we in" but "did the tool actually USE this right-click", +// and only the tool knows. Wrapping on_mouse records that once, for every terminator, instead of +// threading a flag through the twenty-odd sites that consume a RightDown. bool DesignSketchTool::on_mouse(wxMouseEvent& evt, GLCanvas3D& canvas) +{ + const bool consumed = on_mouse_impl(evt, canvas); + if (evt.RightDown()) + m_right_consumed = consumed; + return consumed; +} + +bool DesignSketchTool::on_mouse_impl(wxMouseEvent& evt, GLCanvas3D& canvas) { // Track the cursor in canvas client px so the in-canvas value editor can open right // where the user clicked (Onshape places the field at the click, not via a camera diff --git a/src/slic3r/GUI/DesignSketchTool.hpp b/src/slic3r/GUI/DesignSketchTool.hpp index d4c7fde6d9..862d51fd85 100644 --- a/src/slic3r/GUI/DesignSketchTool.hpp +++ b/src/slic3r/GUI/DesignSketchTool.hpp @@ -89,6 +89,10 @@ public: bool is_active() const { return m_active; } bool has_entities() const { return !m_entities.empty(); } bool on_mouse(wxMouseEvent& evt, GLCanvas3D& canvas); + // True if the LAST right-press was consumed as a gesture terminator (end a polyline chain, + // abandon an anchor, exit a tool). Read-and-clear: the canvas asks on the matching release to + // decide whether that right-click was the user's, in which case it opens the offer. + bool take_right_consumed() { const bool b = m_right_consumed; m_right_consumed = false; return b; } void render(GLCanvas3D& canvas); // Persistent committed sketches to draw even when no session is active (e.g. an @@ -913,6 +917,8 @@ private: // button is free for it, which is the CAD convention (Onshape/SolidWorks). GLSelectionRectangle m_rubber; void pick_bodies_in_rectangle(); // resolve the swept rectangle -> whole-body selection + bool on_mouse_impl(wxMouseEvent& evt, GLCanvas3D& canvas); // the body; on_mouse wraps it + bool m_right_consumed{false}; // last RightDown was a gesture terminator, not a menu void render_solid_highlight(); void render_datum_planes(); // translucent rectangles for datum/reference planes void render_view_helpers(); // world origin planes + axis triad (P / A toggles)