Sketcher: Add concentric capabilities to coincidence constraint. #7703

PaddleStroke · 2022-11-04T12:06:32Z

Enable the coincidence tool to select edges. If those edges are circle, ellipse, arc or arc of ellipse, then it create concentric constraint.

#7546 (comment)

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, we ask you to conform to the following items. Pull requests which don't satisfy all the items below might be rejected. If you are in doubt with any of the items below, don't hesitate to ask for help in the FreeCAD forum!

Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
Your branch is rebased on latest master git pull --rebase upstream master
All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
All commit messages are well-written ex: Fixes typo in Draft Move command text
Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
Commit messages include issue #<id> or fixes #<id> where <id> is the issue ID number from our Issues database in case a particular commit solves or is related to an existing issue. Ex: Draft: fix typos - fixes #4805

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 1.0 Changelog Forum Thread.

freecadci · 2022-11-04T12:16:11Z

for feature branch PR_7703. Pipeline 685710757 was triggered at 66ca9e6. All CI branches and pipelines.

freecadci · 2022-11-04T14:44:46Z

for feature branch PR_7703. Pipeline 685863484 was triggered at 37564f6. All CI branches and pipelines.

chennes · 2022-11-04T15:51:53Z

src/Mod/Sketcher/Gui/CommandConstraints.cpp

@@ -2033,42 +2046,67 @@ void CmdSketcherConstrainCoincident::activated(int iMsg)

 void CmdSketcherConstrainCoincident::applyConstraint(std::vector<SelIdPair> &selSeq, int seqIndex)
 {
+    SketcherGui::ViewProviderSketch* sketchgui = static_cast<SketcherGui::ViewProviderSketch*>(getActiveGuiDocument()->getInEdit());


Just a stylistic comment, you don't have to change it here, but in future code: when using static_cast, dynamic_cast, etc. where we already explicitly state the type, that's a good place to use auto for the variable definition. Sort of a micro-example of the DRY principle.

This code has just been moved.
But I take note of your message. I will try to use auto in those situation in the future.
If I come accross such occurence while making a PR, should I always fix such things along?

It's hard to make a general statement about it: knowing now that this code was simply relocated, then no, I'd say leave it alone. Switching to auto is a minuscule change that makes almost no practical difference, so the very small increase in readability is not worth complicating the review. On the other hand, if you ran across an old-style for loop, and you were already touching that line of code for some other reason, then I'd say yes, let's go ahead and switch to a ranged-for. Brand new code is different, but working with old code IMO there's more wiggle room. There isn't a "policy" either way, we just discuss it in the PR review process and come to a consensus. You are always welcome to push back and tell me why you don't think a particular suggestion makes sense in this particular context: I won't take it personally, we're both trying to make incremental improvements to the code.

chennes

Overall looks good to me, I like the re-use of the coincident constraint here. A little bit of refactoring to reduce code duplication and this should be good to go.

chennes · 2022-11-04T16:00:54Z

src/Mod/Sketcher/Gui/CommandConstraints.cpp

+            geo->getTypeId() == Part::GeomCircle::getClassTypeId() ||
+            geo->getTypeId() == Part::GeomArcOfCircle::getClassTypeId())) {
+            QMessageBox::warning(Gui::getMainWindow(), QObject::tr("Wrong selection"),
+                QObject::tr("Select two vertices from the sketch. Or two circles, ellipses, arcs or arcs of ellipse for concentric constraint."));


I'd suggest a slight wording change:

Suggested change

QObject::tr("Select two vertices from the sketch. Or two circles, ellipses, arcs or arcs of ellipse for concentric constraint."));

QObject::tr("Select two vertices from the sketch for a coincident constraint, or two circles, ellipses, arcs or arcs of ellipse for a concentric constraint."));

and since it's used twice, it's probably best to create it above and re-use it, e.g.

const QString errorMessage = QObject::tr("Select two vertices from the sketch for a coincident constraint, or two circles, ellipses, arcs or arcs of ellipse for a concentric constraint."); ... QMessageBox::warning (Gui::getMainWindow(), QObject::tr("Wrong selection"), errorMessage );

Really, this whole code block has a lot of nearly identical lines. Maybe create a function that takes a Geometry * and returns whether it is "concentric-capable"? You could even make it a lambda right inside applyConstraint, if it really wasn't needed anywhere else in the code.

It's also in activated()
I thought about making a function, but wasn't sure where to put the function.
I thought about lambda in applyConstraint, but then the occurence in activated is left behind.

Ok just on top of the file I guess. There are a few other functions like this there.

Yep - probably in an anonymous namespace, just like the other PR. That will help cut down on what's already a beast of a file!

Other functions aren't in anonymous namespace. So doing so would break homogeneity.

So I'm unsure. What do you think about that?

I'd say don't worry about the anonymous namespace bit for now: it's relatively recent modern C++ coding advice that we should seek to follow when it's reasonable to do so, but don't stress ourselves out over. Someday maybe someone will decide to do a cleanup pass on this file and address some of the linter issues, and they can put all of those functions in an anonymous namespace then. It's not glamorous work, but it's also pretty low-hanging fruit 😄 .

src/Mod/Sketcher/Gui/CommandConstraints.cpp

PaddleStroke · 2022-11-04T16:32:44Z

I just fixed the sentence, and added the function to remove code repeat and forced pushed.

PaddleStroke · 2022-11-04T16:36:50Z

I forgot your comment for the tooltip. I jsut added it and re-force-pushed.

freecadci · 2022-11-04T16:39:24Z

for feature branch PR_7703. Pipeline 685991591 was triggered at c1c168e. All CI branches and pipelines.

freecadci · 2022-11-04T16:43:36Z

for feature branch PR_7703. Pipeline 685994285 was triggered at 129011e. All CI branches and pipelines.

chennes

This looks great now, thanks! I'm going to wait for the CI to complete before merging, but it compiles, runs, and behaves correctly on MSVC/Win11.

0penBrain · 2022-11-13T15:29:40Z

@PaddleStroke This PR introduced severe regression.
To reproduce :

Open new sketch
Draw an arc
Draw a line
Select both arc (edge) + one of the line endpoints
Add coincidence constraint --> A malformed constraint is created

This virtually is the case with any conic + any point selected.

PaddleStroke · 2022-11-14T09:38:18Z

Fixed in : #7794

0penBrain · 2022-11-14T11:13:37Z

Wiki pages about Coincident constraint and Release notes to be updated

PaddleStroke · 2022-11-14T11:50:13Z

done
https://wiki.freecadweb.org/Sketcher_Workbench
https://wiki.freecadweb.org/Sketcher_ConstrainCoincident
https://wiki.freecadweb.org/Release_notes_1.0

PaddleStroke · 2022-11-14T11:52:22Z

@luzpaz can you please check and let me know if those changes are correct? Don't hesitate to modify if necessary.

0penBrain · 2022-11-14T12:22:20Z

@luzpaz can you please check and let me know if those changes are correct? Don't hesitate to modify if necessary.

What is important is that information about changes is pushed there. Then we have awesome people working on the wiki and they eventually can fix wording and so on (the good strategy IMO is that you learn from what these regulars fix).
Just 2 points:

Except in Release notes, think about tagging your changes with {{Version|1.0}} ;)
Be careful with translation tags (better not do it if you're unsure). For example in the page about Coincident constraint, you use a translation tag that is already used in the page, so that probably will be a mess. :)

github-actions bot added the WB Sketcher Related to the Sketcher Workbench label Nov 4, 2022

PaddleStroke mentioned this pull request Nov 4, 2022

[Feature Request] Concentric constraint in the Sketcher #7546

Closed

2 tasks

PaddleStroke force-pushed the Concentric branch from 66ca9e6 to 37564f6 Compare November 4, 2022 14:34

chennes reviewed Nov 4, 2022

View reviewed changes

chennes requested changes Nov 4, 2022

View reviewed changes

PaddleStroke force-pushed the Concentric branch from 37564f6 to c1c168e Compare November 4, 2022 16:32

Sketcher: Add concentric capabilities to coincidence constraint.

129011e

PaddleStroke force-pushed the Concentric branch from c1c168e to 129011e Compare November 4, 2022 16:36

chennes approved these changes Nov 4, 2022

View reviewed changes

chennes merged commit caa953a into FreeCAD:master Nov 4, 2022

PaddleStroke mentioned this pull request Nov 14, 2022

Sketcher: Coincidence constraint: fix error introduced by concentric #7794

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sketcher: Add concentric capabilities to coincidence constraint. #7703

Sketcher: Add concentric capabilities to coincidence constraint. #7703

PaddleStroke commented Nov 4, 2022

freecadci commented Nov 4, 2022

freecadci commented Nov 4, 2022

chennes Nov 4, 2022

PaddleStroke Nov 4, 2022

chennes Nov 4, 2022 •

edited

chennes left a comment

chennes Nov 4, 2022

chennes Nov 4, 2022

PaddleStroke Nov 4, 2022

PaddleStroke Nov 4, 2022

chennes Nov 4, 2022

PaddleStroke Nov 4, 2022

PaddleStroke Nov 4, 2022

chennes Nov 4, 2022

PaddleStroke commented Nov 4, 2022

PaddleStroke commented Nov 4, 2022

freecadci commented Nov 4, 2022

freecadci commented Nov 4, 2022

chennes left a comment

0penBrain commented Nov 13, 2022 •

edited

PaddleStroke commented Nov 14, 2022

0penBrain commented Nov 14, 2022

PaddleStroke commented Nov 14, 2022

PaddleStroke commented Nov 14, 2022

0penBrain commented Nov 14, 2022

	QObject::tr("Select two vertices from the sketch. Or two circles, ellipses, arcs or arcs of ellipse for concentric constraint."));
	QObject::tr("Select two vertices from the sketch for a coincident constraint, or two circles, ellipses, arcs or arcs of ellipse for a concentric constraint."));

Sketcher: Add concentric capabilities to coincidence constraint. #7703

Sketcher: Add concentric capabilities to coincidence constraint. #7703

Conversation

PaddleStroke commented Nov 4, 2022

freecadci commented Nov 4, 2022

freecadci commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chennes Nov 4, 2022 • edited

Choose a reason for hiding this comment

chennes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaddleStroke commented Nov 4, 2022

PaddleStroke commented Nov 4, 2022

freecadci commented Nov 4, 2022

freecadci commented Nov 4, 2022

chennes left a comment

Choose a reason for hiding this comment

0penBrain commented Nov 13, 2022 • edited

PaddleStroke commented Nov 14, 2022

0penBrain commented Nov 14, 2022

PaddleStroke commented Nov 14, 2022

PaddleStroke commented Nov 14, 2022

0penBrain commented Nov 14, 2022

chennes Nov 4, 2022 •

edited

0penBrain commented Nov 13, 2022 •

edited