Sketcher: Arc to Other distance constraint #9166

FlachyJoe · 2023-04-02T20:37:05Z

Propagate circles distance constraint to arcs.

FlachyJoe · 2023-04-02T20:43:29Z

It works but constraints can be shown in strange placements

NB : it's also the case for Tangent Constraint

adrianinsaval · 2023-04-03T14:55:56Z

Is there a way of moving the angle of the constraint after placing it?

FlachyJoe · 2023-04-03T17:04:56Z

@adrianinsaval Angle is defined by the shorter distance between circles so it can't move.
In the concentric case it could but I don't know how to implement this.
Another solution I see is to draw a thin constraint-colored circle below the arc as an helper.

freecadci · 2023-04-03T19:47:00Z

for feature branch PR_9166. Pipeline 826752983 was triggered at 94ac740. All CI branches and pipelines.

FlachyJoe · 2023-04-13T15:19:00Z

#9257 fixes the display problems

freecadci · 2023-06-18T11:17:04Z

for feature branch PR_9166. Pipeline 903564958 was triggered at 581e64a. All CI branches and pipelines.

PaddleStroke · 2023-06-22T08:21:37Z

Why is this not going in 0.21 ? As circle to circle distance was introduced in 0.21 it would make sense that this goes in as well.

Edit: It's a known issue: (Besides, I can't seem to be able to use circle distances when the tool is activated first (before selection).
If I select the circles first, then start the distance tool, then it works.
But if I try to start the tool, then select circles, the circles don't get selected.
Using windows10, master from 1 day ago.
Do you have this behavior on your side? Should I create an issue for it?)

FlachyJoe · 2023-06-22T19:01:08Z

@PaddleStroke this PR requires #9703

PaddleStroke · 2023-06-22T20:37:40Z

This one could go in as well Imo. It's not a big change and would make things more consistent rather than having half of circle-circle distance merged in 0.21.

adrianinsaval · 2023-06-22T20:44:27Z

I tend to agree but we would need someone to review it.

PaddleStroke · 2023-06-22T21:04:18Z

I can give it a go tomorrow if my review is deamed sufficient.
#9703 is mostly ui stuff similar to what I've done many time so I should be able to review.
This PR is probably a lot of copy paste of the circle-circle constrain, so I think it'll ok for me.

PaddleStroke · 2023-08-26T06:44:35Z

#9703 merged so this one can become a not-draft Pull request

PaddleStroke

Code looks good to me.
Main complain is that the syntax changes should not belong to this PR and should be a separate PR.
Without the syntax changes this PR would be like 150 lines. Here it's 1500 ... It makes reviewing much more difficult and long.

I have not built to test.

src/Mod/Sketcher/Gui/Utils.cpp

src/Mod/Sketcher/Gui/ViewProviderSketch.cpp

src/Mod/Sketcher/Gui/Utils.cpp

src/Mod/Sketcher/Gui/EditModeConstraintCoinManager.cpp

chennes · 2023-08-28T16:10:23Z

@FlachyJoe can you resolve those conflicts?

FlachyJoe · 2023-08-30T20:34:58Z

@PaddleStroke Do I need to add anything to make this work with the Constrain Contextual implementation?

PaddleStroke · 2023-08-30T20:49:00Z

Just rebase and chennes will merge I think. On my side I'm currently implementing the point to circle constraint in sketcher_dimension, so I can look at arc to circle at the same time. By the way do you plan to do point to arc as well? And line to arc ?

…

On Wed, Aug 30, 2023, 22:35 Florian Foinant-Willig ***@***.***> wrote: @PaddleStroke <https://github.com/PaddleStroke> Do I need to add anything to make this work with the Constrain Contextual implementation? — Reply to this email directly, view it on GitHub <#9166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEYO6MK7ARYLXKL4PAHZSY3XX6PX5ANCNFSM6AAAAAAWQQP7UU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

FlachyJoe · 2023-08-31T20:45:35Z

do you plan to do point to arc as well? And line to arc ?

Yes I'll do it. I maybe can add these as commit to this PR so all the arc stuff come in the same time.

EDIT : @PaddleStroke should I allow you to push in my arc-distance branch so the required Contextual code will be merged together?

PaddleStroke · 2023-09-01T05:11:12Z

Yes I'll do it. I maybe can add these as commit to this PR so all the arc stuff come in the same time.

Good idea.

should I allow you to push in my arc-distance branch so the required Contextual code will be merged together?

That may make things a bit complex especially since there's already open PR touching Sketcher_Dimension (and one that changes the arc stuff in it). So just focus on adding the constraints and once that merge we'll add support for them in Dimension so that we don't have conflicts.

FlachyJoe · 2023-09-01T10:37:51Z

Switch back to Draft until arc-line and arc-point commits are ready.

abdullahtahiriyo · 2023-12-05T14:29:34Z

At it now.

abdullahtahiriyo · 2023-12-05T14:45:14Z

src/Mod/Sketcher/App/Sketch.cpp

 int Sketch::addDistanceConstraint(int geoId1, int geoId2, double* value, bool driving)
 {
    geoId1 = checkGeoId(geoId1);
    geoId2 = checkGeoId(geoId2);

-    if (Geoms[geoId1].type == Circle) {
+    GCS::Circle *c1, *c2;


Generally, we declare variables inside the scope where they are necessary.

I do understand that you can make use of c1 in the two blocks (if and else). However, in practice it is not even the same "c1", but just a circle. In the case of c2, it is not even necessary in the first block.

abdullahtahiriyo · 2023-12-05T14:47:33Z

src/Mod/Sketcher/App/Sketch.cpp

+        }
+        GCS::Line* l = &Lines[Geoms[geoId2].index];
+        int tag = ++ConstraintsCounter;
+        GCSsys.addConstraintC2LDistance(*c1, *l, value, tag, driving);


And if c1 is not either a circle or an arc, then we pass an uninitialised c1 to addConstraintC2LDistance...

abdullahtahiriyo · 2023-12-05T14:54:11Z

src/Mod/Sketcher/App/Sketch.cpp

+        else if (Geoms[geoId2].type == Arc) {
+            c2 = &Arcs[Geoms[geoId2].index];
+        }
+        if (c1 == nullptr || c2 == nullptr) {


Here we are relying on c1 being nullptr if it was not assigned... however, it is not for granted that a pointer is initialised with nullptr.

abdullahtahiriyo · 2023-12-05T16:29:34Z

src/Mod/Sketcher/Gui/CommandConstraints.cpp

+            radius2 = circleSeg2->getRadius();
+            center2 = circleSeg2->getCenter();
+        }
+        if (radius1 == 0.0 || radius2 == 0.0) {


This code does not show intent clearly. It relies on what is supported now for edges and the fact that they may have radii.

The whole block is heavy to read because what is written is implied rather than explicit. The branching is not very well structured either... we should look into recoding this.

abdullahtahiriyo · 2023-12-05T17:35:28Z

src/Mod/Sketcher/Gui/CommandConstraints.cpp

@@ -4274,6 +4253,45 @@ void CmdSketcherConstrainDistance::activated(int iMsg)

            return;
        }
+        double ActDist = 0.0;


Splitting the block of circle-circle with the circle-line is a little bit awkward.

abdullahtahiriyo · 2023-12-05T19:17:20Z

src/Mod/Sketcher/Gui/EditModeConstraintCoinManager.cpp

@@ -815,61 +814,75 @@ void EditModeConstraintCoinManager::processConstraints(const GeoListFacade& geol
                case DistanceY: {
                    assert(Constr->First >= -extGeoCount && Constr->First < intGeoCount);
                    Base::Vector3d pnt1(0., 0., 0.), pnt2(0., 0., 0.);
+                    pnt1 = geolistfacade.getPoint(Constr->First, Constr->FirstPos);


Then pnt1 can also be directly initialised to this.

abdullahtahiriyo · 2023-12-05T19:17:54Z

src/Mod/Sketcher/Gui/EditModeConstraintCoinManager.cpp

-                                    pnt1.ProjectToLine(
-                                        ct - l2p1,
-                                        l2p2 - l2p1);  // project on the line translated to origin
+                                if (isCircle(*geo1) || isArcOfCircle(*geo1)) {


This code keeps being repeated and could be refactored.

abdullahtahiriyo · 2023-12-05T19:26:57Z

src/Mod/Sketcher/Gui/Utils.cpp


-    point1 = circle1->getCenter();
-    point2 = circle2->getCenter();
+    if (geom1->is<Part::GeomCircle>()) {


This is again the repeated code of above.

abdullahtahiriyo · 2023-12-05T19:39:24Z

src/Mod/Sketcher/Gui/ViewProviderSketch.cpp

+                if (Constr->FirstPos != Sketcher::PointPos::none){ // circular to point distance
+                    Base::Vector3d ct;
+                    double rad = 0.0;
+                    if (isCircle(*geo2)){


abdullahtahiriyo · 2023-12-06T15:27:16Z

For fixing the issues above, I provide some commits:
https://github.com/abdullahtahiriyo/FreeCAD_sf_master/commits/arc-distance

However, this branch heavily conflicts with master because of some changes brought to how to represent angles.

I would propose:

you take a look to my fixes and see if you agree. If you do, just cherry-pick them into your branch.
you then need to address the conflicts.
because the conflicts do not seem straight-forward, we should do some testing after solving them.

Does it sound ok?

…factor duplicated code

FlachyJoe · 2023-12-12T21:05:45Z

@abdullahtahiriyo done

abdullahtahiriyo · 2023-12-17T15:01:10Z

Thanks for the correction in the refactored function with the wrong type cast.

I will merge this.

We should monitor the change to make distances negative possible. I have done some testing, but the tests are not exhaustive. I think there will not be unintended effects for existing sketches, but I definitely want to hear about it if anybody realises.

With respect to sketching stability:

I only want to note here that I am unsure about how stable sketches using this construct will be (also the same goes for circle), due to the flipping that may occur:

But that, I think is a matter of learning from it.

FlachyJoe · 2023-12-17T17:31:28Z

Thank you @abdullahtahiriyo.
@PaddleStroke you can now upgrade Contextual Dimensions.

PaddleStroke · 2023-12-18T07:55:03Z

Great work guys ! I will implement in sketcher_dimension asap.

PaddleStroke · 2023-12-20T11:57:33Z

Point to line distance is broken. I am not sure but this might be this PR that introduced this problem.
See discussion : https://forum.freecad.org/viewtopic.php?t=83649

FlachyJoe · 2023-12-20T12:55:29Z

@PaddleStroke thank you to show this.
The problem was with the constraint display:

Fixed by #11784

PaddleStroke · 2023-12-20T13:21:04Z

Ah interesting. Great thanks for the quick fix !

maxwxyz · 2023-12-24T20:16:27Z

@FlachyJoe could this PR introduce the issues in the current weekly build or maybe #11784 :

github-actions bot added the WB Sketcher Related to the Sketcher Workbench label Apr 2, 2023

FlachyJoe mentioned this pull request Apr 3, 2023

[Issue] Geometric constraints on Arcs are displayed away of supports #9177

Closed

2 tasks

abdullahtahiriyo added the 0.22 for the v0.22 development cycle label May 7, 2023

FlachyJoe force-pushed the arc-distance branch from 94ac740 to 581e64a Compare May 11, 2023 21:19

sliptonic added this to the 1.0 milestone Jul 10, 2023

PaddleStroke mentioned this pull request Aug 3, 2023

Sketcher: Add command to switch arc helpers #9703

Merged

FlachyJoe force-pushed the arc-distance branch from 581e64a to 9638f98 Compare August 14, 2023 14:21

PaddleStroke reviewed Aug 26, 2023

View reviewed changes

FlachyJoe force-pushed the arc-distance branch from 9638f98 to 201a283 Compare August 26, 2023 10:13

FlachyJoe marked this pull request as ready for review August 26, 2023 10:13

FlachyJoe force-pushed the arc-distance branch from 201a283 to 17f6de1 Compare August 31, 2023 20:41

FlachyJoe marked this pull request as draft September 1, 2023 10:36

FlachyJoe force-pushed the arc-distance branch from 8d27281 to 8b4831a Compare November 29, 2023 20:14

chennes assigned chennes and abdullahtahiriyo and unassigned chennes Dec 4, 2023

abdullahtahiriyo reviewed Dec 6, 2023

View reviewed changes

FlachyJoe force-pushed the arc-distance branch from 720f263 to 5cbb381 Compare December 12, 2023 20:17

FlachyJoe and others added 6 commits December 12, 2023 21:25

Sketcher: Arc to arc or circle distance constraint

d1eac1c

Sketcher: Extend distance constraint to arcs

f70dd7a

Sketch: Distance - fix uninit pointers and scope

bd3c781

Sketcher: Redraft circle/arc to circle/arc constraint command

949e4ec

Sketcher: Utils/CommandConstraints/EditModeConstraintCoinManager - re…

bf7fd41

…factor duplicated code

Refactor ViewProvider move constraint code

e4a03c4

FlachyJoe force-pushed the arc-distance branch from 5cbb381 to e4a03c4 Compare December 12, 2023 20:26

FlachyJoe added 2 commits December 12, 2023 21:38

Fix Sketcher::getRadiusCenterCircleArc(circle)

38ae587

Fix isCircleOrArc usage

2a84be6

abdullahtahiriyo merged commit df32ad2 into FreeCAD:main Dec 17, 2023
9 checks passed

FlachyJoe mentioned this pull request Dec 20, 2023

Sketcher: Fix Point to Line Distance #11784

Merged

This was referenced Jan 4, 2024

Sketcher: add arc-to-x distance to auto-dimension tool #11891

Merged

[Feature Request] Sketcher: Improve Dimension between Circle/Arc (Distance Constraints) #11918

Open

FlachyJoe mentioned this pull request Jan 7, 2024

[Sketcher] Redo Improve angle & radius constraint rendering (#11507) #11926

Merged

Sketcher: Arc to Other distance constraint #9166

Sketcher: Arc to Other distance constraint #9166

Conversation

FlachyJoe commented Apr 2, 2023

FlachyJoe commented Apr 2, 2023

adrianinsaval commented Apr 3, 2023

FlachyJoe commented Apr 3, 2023 • edited

freecadci commented Apr 3, 2023

FlachyJoe commented Apr 13, 2023

freecadci commented Jun 18, 2023

PaddleStroke commented Jun 22, 2023 • edited

FlachyJoe commented Jun 22, 2023

PaddleStroke commented Jun 22, 2023

adrianinsaval commented Jun 22, 2023

PaddleStroke commented Jun 22, 2023

PaddleStroke commented Aug 26, 2023

PaddleStroke left a comment

Choose a reason for hiding this comment

chennes commented Aug 28, 2023

FlachyJoe commented Aug 30, 2023

PaddleStroke commented Aug 30, 2023 via email • edited

FlachyJoe commented Aug 31, 2023 • edited

PaddleStroke commented Sep 1, 2023

FlachyJoe commented Sep 1, 2023

abdullahtahiriyo commented Dec 5, 2023

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

abdullahtahiriyo commented Dec 6, 2023

FlachyJoe commented Dec 12, 2023

abdullahtahiriyo commented Dec 17, 2023

FlachyJoe commented Dec 17, 2023

PaddleStroke commented Dec 18, 2023

PaddleStroke commented Dec 20, 2023 • edited

FlachyJoe commented Dec 20, 2023

PaddleStroke commented Dec 20, 2023

maxwxyz commented Dec 24, 2023

FlachyJoe commented Apr 3, 2023 •

edited

PaddleStroke commented Jun 22, 2023 •

edited

PaddleStroke commented Aug 30, 2023 via email •

edited

FlachyJoe commented Aug 31, 2023 •

edited

PaddleStroke commented Dec 20, 2023 •

edited