[Sketcher] General Tangency with B-splines (re-do) by AjinkyaDahale · Pull Request #11853 · FreeCAD/FreeCAD

AjinkyaDahale · 2023-12-29T06:57:14Z

Replaces #9125 with the branch in my personal fork (as opposed to the Ondsel one).

Part of this work was done while I was part of Ondsel Inc., and the rest of the work is being supported through an FPA grant.

chennes

This is starting to come into shape, nice work. As we discussed in our chat, there are still a few issues to be worked out:

Selection order should not matter (right now it does, you get strange results if you select the spline last instead of first).
I will probably always want to place my point on the spline first, then add the tangency constraint -- consuming that extra constraint doesn't work right now.
There is a phantom extra constraint in my over-constrained error messages now.
Attempting tangency constraint without selecting a point needs to pop up a warning message explaining how to resolve the issue.

src/Mod/Sketcher/App/Sketch.cpp

chennes · 2024-01-01T17:02:06Z

Is it possible to create the point if it doesn't exist? Just generate one as part of the process?

ETA: We discussed this in Discord and are concerned about the possibility of it blowing up your design due to weird placement of the generated point, so maybe it's not a good idea...

AjinkyaDahale · 2024-01-01T17:31:07Z

Is it possible to create the point if it doesn't exist? Just generate one as part of the process?

ETA: We discussed this in Discord and are concerned about the possibility of it blowing up your design due to weird placement of the generated point, so maybe it's not a good idea...

Unlike other curves, a Bspline can have tangents and perps with the same curve an arbitrary number of times.

Additionally, insertion of a point seems like a new construct. If someone wants to take a jab at it with a B-spline, that would be great.

I was actually hoping to copy the complaint over direct (2 selection) tangent from that for hyperbolas and all, but seems that has been "worked around" by adding the point.

AjinkyaDahale · 2024-01-01T17:37:01Z

Sidenote: the order of selection shouldn't matter now.

AjinkyaDahale · 2024-01-03T02:55:45Z

Meanwhile, could we have an icon for 3-selection tangent, in the future, it may be a good idea to also "consume" any point-on-object constraint on, say, a line as well when we select it, another curve, and a point? This constraint without the point-on-object doesn't make sense.

@obelisk79 @chennes something for the DWG, maybe.

AjinkyaDahale · 2024-01-03T03:58:42Z

3. There is a phantom extra constraint in my over-constrained error messages now.

This should be fixed now.

obelisk79 · 2024-01-03T04:09:25Z

Meanwhile, could we have an icon for 3-selection tangent, in the future, it may be a good idea to also "consume" any point-on-object constraint on, say, a line as well when we select it, another curve, and a point? This constraint without the point-on-object doesn't make sense.

@obelisk79 @chennes something for the DWG, maybe.

I'm not sure what a 3 selection tangent icon would look like. Instinctively I'm inclined to suggest not having a separate tangency symbol for this case, even though it is unique 'under the hood', I would need to better understand the need/benefit for exposing this difference to the user.

Additionally, your statement about point-on-object constraints seems to contradict itself.

AjinkyaDahale · 2024-01-03T04:18:12Z

I'm not sure what a 3 selection tangent icon would look like. Instinctively I'm inclined to suggest not having a separate tangency symbol for this case, even though it is unique 'under the hood', I would need to better understand the need/benefit for exposing this difference to the user.

Try the following:

Create a parabola arc and a circular arc. Also a point between them.
Add tangent constraint with these 3.

You will see there are 2 or 3 icons now: one for tangent and two for point-on-object. I am saying that these should be combined into one constraint, so that we can't have a tangent constraint where the curves don't even touch. The icon for that (combined) constraint should be distinct from a normal tangent to show there's a point on it. Do you think that might be confusing?

Additionally, your statement about point-on-object constraints seems to contradict itself.

Could you explain the contradiction? I'm saying a 3-selection tangent (i.e. with curves and a point) do not make sense if the point is not on both curves.

AjinkyaDahale · 2024-01-04T13:20:22Z

Now, tangent-at-point will "consume" any existing point-on-object constraint between the involved point and B-spline(s).

With this, all issues described in #11853 (review) should be fixed.

chennes

Although this part of the code is not my area of expertise, I have some general comments on your modifications. I don't view these as merge-preventers, just suggestions for improvement that you can implement or not as you see fit.

Also, I don't much like abbreviations in variable and function names. Even for English speakers I think it makes the code more opaque, and for those for whom English is not their primary language I think it makes it much harder to understand. I know this is endemic in Sketcher (and FreeCAD as a whole), but I hate to see new code doing it.

Finally, I'd love to see some automated tests implemented in the gtest framework for this new feature.

src/Mod/Sketcher/Gui/CommandConstraints.cpp

src/Mod/Sketcher/App/planegcs/Geo.cpp

src/Mod/Sketcher/App/planegcs/Constraints.cpp

chennes · 2024-01-13T01:33:02Z

src/Mod/Sketcher/App/Sketch.cpp

+                    std::swap(geoId1, geoId2);
+                    std::swap(crv1, crv2);
+                    std::swap(pos1, pos2);
+                    // FIXME: Confirm whether or not this is needed


Leaving this here would make me nervous: can you confirm?

This one was about the angle: is there benefit in making the angles direction specific? That is, should this be the (counterclockwise) angle between the first and second selected curves? In that case, the same angle with selection reversed should look as if the older selection was provided with a negative angle. But it currently seems like this angle is already adjusted elsewhere: I tried different selections and same angle, and the shape remains same.

This may have use specifically when negative angles have some parametric use, though I can't think of any right now.

I think removing this would require us to first properly document what the angle constraint means for us, not just for B-splines, but for other curves as well.

AjinkyaDahale · 2024-01-21T16:53:45Z

Although this part of the code is not my area of expertise, I have some general comments on your modifications. I don't view these as merge-preventers, just suggestions for improvement that you can implement or not as you see fit.

Going through them as I can.

Also, I don't much like abbreviations in variable and function names. Even for English speakers I think it makes the code more opaque, and for those for whom English is not their primary language I think it makes it much harder to understand. I know this is endemic in Sketcher (and FreeCAD as a whole), but I hate to see new code doing it.

I'd be OK with changing some of the less obvious abbreviations in the Sketcher side. However, many on the GCS side may be mathematical terms.

AjinkyaDahale · 2024-01-21T17:42:54Z

Found some issue with the angle rendition.
Screencast from 2024-01-21 23-09-35.webm

Could use help from someone acquainted with the drawing aspects. @PaddleStroke maybe?

PaddleStroke · 2024-01-23T10:13:52Z

@AjinkyaDahale if the angle switch like this, I think the start and end pos (of the line or of the spline) are maybe switching

AjinkyaDahale · 2024-01-23T10:42:08Z

@AjinkyaDahale if the angle switch like this, I think the start and end pos (of the line or of the spline) are maybe switching

@PaddleStroke Would you be able to take a look? It may take me a good amount of time getting acquainted with that side of the code by myself.

PaddleStroke · 2024-01-23T14:01:46Z

I don't know what I am looking at. Did you implement angle constraint between angle and bspline ?
I looked at your PR and you are not editing EditModeConstraintManager.cpp which you likely should.
Look at line 1268 'case Angle' of processConstraint.

Here you want to handle the special case of line + bspline by setting the startAngle and range of the constraint.

AjinkyaDahale · 2024-02-08T01:35:50Z

@chennes I would suggest keeping the planegcs commits separate from, and placed before, the Sketcher ones. IIRC there is already a project where planegcs is made into a library by itself.

Of course apart from that it's ideal if the whole thing compiles at each commit.

EDIT: OK I see things fall apart a little in some commits, like in [Sketcher][planegcs] Fix direction issue with B-spline tangents.
fcfafa5, where both sketcher and planegcs are edited. I could try to split that commit into two if you're convinced about squashing these into two commits.

AjinkyaDahale · 2024-02-08T06:47:13Z

In order to use the classes in Geo.h in the tests, they all have to be defined with the SketcherExport macro, e.g.
class SketcherExport DeriVector2
{
    ...

These commits were apparently lost when I force pushed. Do you have them on your machine?

AjinkyaDahale · 2024-02-08T07:15:04Z

I propose to partially squash this PR: I'm planning on merging the following commits into one, unless you object @AjinkyaDahale: ...[planegcs] Remove some numerical testing 48c31df

It might be a good idea to keep this commit by itself so in the future if someone intends to make these numerical tests in gtest, this could be useful.

chennes · 2024-02-08T14:26:36Z

These commits were apparently lost when I force pushed. Do you have them on your machine?

Not worth trying to find them, all I did was add the appropriate export to all the classes in that file.

These are intended to use when calculating normal simply with points could be numerically expensive or otherwise nonviable.

As opposed to "punctual" that already exists for curves.

For use in angle-via-point with complex curves.

The following commits were squashed into this [Sketcher] Handle some corner cases in AngleViaPoint [Sketcher] Avoid redundant constraints with B-splines... When involving tangent, perpendicular and angle constraints. [Sketcher] Add pre-commit changes [Sketcher] Do not allow 2-selection tangent with B-spline Also... [Sketcher] Report error when using direct tangency with B-splines [Sketcher] Fix malformed constraint when B-spline is selected second To clarify, this means the second curve selected. The position of the point in selection order does not matter in angle-via-point. [Sketcher] Fix wrong number for B-Spline tangent on redundancy [Sketcher] Remove existing point-on-object in some redundant cases Particularly when point constrained on a B-spline is being used for tangent, perpendicular or angle via point with the same B-spline. [Sketcher] Fix direction issue with B-spline tangents. Without these changes the solver might try to "twist" the B-spline to make the angle between curves be 0 instead of PI (which may be closer to the initial shape).

If needed this can be moved to a gtest,

Needed for gtests currently.

AjinkyaDahale · 2024-02-08T14:35:30Z

These commits were apparently lost when I force pushed. Do you have them on your machine?

Not worth trying to find them, all I did was add the appropriate export to all the classes in that file.

I think I saw two of them. Anyway, let's see if this succeeds.

EDIT: Still figuring it out. Hopefully 2-3 more force pushes to go.

...for use in tests.

AjinkyaDahale · 2024-02-08T17:14:52Z

@chennes if this works should I put the _USE_MATH_DEFINES under an ifdef?

chennes · 2024-02-08T17:59:45Z

Yeah, something like

#ifdef FC_OS_WIN32
#define _USE_MATH_DEFINES
#endif

Syres916 · 2024-02-09T13:37:18Z

@AjinkyaDahale sorry to raise this after merging but all those new error messages in addTangentConstraint, should they not be translated if they are important user feedback?

…es_1.0.gif|384px]]</br>Щёлкните по изображению, если анимация не запускается. | Добавлена касательная к рёбрам B-сплайна, что устраняет необходимость использования конечных точек и различных обходных путей. [FreeCAD/FreeCAD#11853 Запрос на..."

github-actions bot added the Mod: Sketcher Related to the Sketcher Workbench label Dec 29, 2023

AjinkyaDahale mentioned this pull request Dec 29, 2023

[Sketcher] [DO NOT MERGE] General Tangency with B-splines #9125

Closed

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch 2 times, most recently from a273f36 to 3ce56b9 Compare December 30, 2023 04:15

chennes self-requested a review December 31, 2023 01:41

chennes requested changes Dec 31, 2023

View reviewed changes

chennes reviewed Jan 1, 2024

View reviewed changes

src/Mod/Sketcher/App/Sketch.cpp Show resolved Hide resolved

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch from 851b7d1 to 24739d2 Compare January 2, 2024 16:38

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch 3 times, most recently from 18e8854 to f1ef316 Compare January 4, 2024 13:16

AjinkyaDahale requested a review from chennes January 4, 2024 13:20

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch 2 times, most recently from bd3f51c to 36af05b Compare January 5, 2024 17:28

chennes reviewed Jan 13, 2024

View reviewed changes

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch from 36af05b to aedd7e2 Compare January 21, 2024 16:45

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch from aedd7e2 to 6527d68 Compare January 21, 2024 17:58

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch from 28e80a3 to 8c37fd1 Compare February 8, 2024 06:39

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch from 8c37fd1 to 6094657 Compare February 8, 2024 14:24

AjinkyaDahale added 6 commits February 8, 2024 20:04

[Sketcher][planegcs] Support angle via point with params

6a3c055

These are intended to use when calculating normal simply with points could be numerically expensive or otherwise nonviable.

[Sketcher][planegcs] Implement parametric BSpline::CalculateNormal

5ba050b

As opposed to "punctual" that already exists for curves.

[planegcs] Implement calculateAngleViaParams

82f0359

For use in angle-via-point with complex curves.

[planegcs] Remove some numerical testing

b1ef4be

If needed this can be moved to a gtest,

[planegcs] Implement BSpline::Value()

da48a72

Needed for gtests currently.

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch from 6094657 to 0ed9a9b Compare February 8, 2024 14:34

[planegcs] Add SketcherExport macro to classes in Geo.h

1e19926

...for use in tests.

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch 2 times, most recently from f8edc03 to c1e8adc Compare February 8, 2024 16:58

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch from c1e8adc to a641f8a Compare February 8, 2024 17:38

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch from a641f8a to 5c80bd0 Compare February 8, 2024 18:13

AjinkyaDahale added 3 commits February 8, 2024 23:46

[planegcs][test] Add Spline and Arc tangent test

459d210

[planegcs] Simplify GCS::BSpline::CalculateNormal()

73434e0

[Sketcher] Handle spline selection in Dimension tool

3f1f638

AjinkyaDahale force-pushed the sketcher-general-tangency-splines branch from d89f459 to 3f1f638 Compare February 8, 2024 18:16

chennes merged commit 9695bf0 into FreeCAD:main Feb 8, 2024

AjinkyaDahale deleted the sketcher-general-tangency-splines branch March 21, 2025 15:34

Uh oh!

Conversation

AjinkyaDahale commented Dec 29, 2023

Uh oh!

chennes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chennes commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AjinkyaDahale commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AjinkyaDahale commented Jan 1, 2024

Uh oh!

AjinkyaDahale commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AjinkyaDahale commented Jan 3, 2024

Uh oh!

obelisk79 commented Jan 3, 2024

Uh oh!

AjinkyaDahale commented Jan 3, 2024

Uh oh!

AjinkyaDahale commented Jan 4, 2024

Uh oh!

chennes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chennes Jan 13, 2024

Choose a reason for hiding this comment

Uh oh!

AjinkyaDahale Jan 13, 2024

Choose a reason for hiding this comment

Uh oh!

AjinkyaDahale Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

AjinkyaDahale commented Jan 21, 2024

Uh oh!

AjinkyaDahale commented Jan 21, 2024

Uh oh!

PaddleStroke commented Jan 23, 2024

Uh oh!

AjinkyaDahale commented Jan 23, 2024

Uh oh!

PaddleStroke commented Jan 23, 2024

Uh oh!

AjinkyaDahale commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AjinkyaDahale commented Feb 8, 2024

Uh oh!

AjinkyaDahale commented Feb 8, 2024

Uh oh!

chennes commented Feb 8, 2024

Uh oh!

AjinkyaDahale commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AjinkyaDahale commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chennes commented Feb 8, 2024

Uh oh!

Syres916 commented Feb 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

chennes commented Jan 1, 2024 •

edited

Loading

AjinkyaDahale commented Jan 1, 2024 •

edited

Loading

AjinkyaDahale commented Jan 3, 2024 •

edited

Loading

AjinkyaDahale commented Feb 8, 2024 •

edited

Loading

AjinkyaDahale commented Feb 8, 2024 •

edited

Loading

AjinkyaDahale commented Feb 8, 2024 •

edited

Loading