[Sketcher] Command for B-spline by knot

[AjinkyaDahale]

Adds a new DrawSketchHandler for creating B-splines by interpolation. Each provided point is treated as a knot.

Fixes #8529.

This project is being funded by the Open Toolchain Foundation under the title "Open Toolchain Foundation - Curve drawing tool in Sketcher Workbench"

[freecadci]

for feature branch PR_8530. Pipeline 782079746 was triggered at c95887e. All CI branches and pipelines.

[freecadci]

for feature branch PR_8530. Pipeline 782088517 was triggered at 63a4dcc. All CI branches and pipelines.

[freecadci]

for feature branch PR_8530. Pipeline 782680918 was triggered at d921776. All CI branches and pipelines.

[freecadci]

for feature branch PR_8530. Pipeline 783360623 was triggered at a664343. All CI branches and pipelines.

[freecadci]

for feature branch PR_8530. Pipeline 784271854 was triggered at 8c829c2. All CI branches and pipelines.

[AjinkyaDahale]

@abdullahtahiriyo Do you have time to make a review? I have a minimal product ready, which so far only utilizes existing OCCT functionality.

spline-by-interpolation-demo-.2.mp4

Promising, but a lot of limitations. The rational B-spline issues are still there. Since the weights are not constrained, dragging can result in erratic behaviour.

[abdullahtahiriyo]

I hope to be able by the end of the week, I have to PRs I must deal with before that.

[freecadci]

for feature branch PR_8530. Pipeline 786808629 was triggered at 1aee6b3. All CI branches and pipelines.

[freecadci]

for feature branch PR_8530. Pipeline 786848232 was triggered at 23739d9. All CI branches and pipelines.

[freecadci]

for feature branch PR_8530. Pipeline 786859011 was triggered at cfef445. All CI branches and pipelines.

[abdullahtahiriyo]

Ok. You were fast fixing the mid to start and the typos.

I did not do a detailed review. Just had a quick look and tested it.

The command delivers on its description. If interpolating per knot is what is intended, it is okeish.

As it is now, weights are not constrained to non-rational, which leads to lots of DoFs. Even after constraining weights to non-rational, it leads to more DoFs than necessary for interpolation.

Aside from the issues of weights becoming near zero and lack of convergence associated to unconstrained poles and the excess of DoFs, it does what it promises by the title.

I ran the same example of interpolating through 5 fixed points, with this tool (result is 7 poles) and generating a normal 5 pole BSpline and constraining it point on object. The number of DoFs is lower with the second method. It is easier to work with, if interpolation is all that is needed.

I will give it another go later on or during the weekend.

Do you have a roadmap of how many different new construction methods you want to implement?

[AjinkyaDahale]

Ok. You were fast fixing the mid to start and the typos.

Thanks. It was more of something I missed rather than a bug to hunt so it was straightforward.

The command delivers on its description. If interpolating per knot is what is intended, it is okeish.

Indeed. OK-ish is an apt description. It delivers on the expectation, but the results might not be exactly what the user intended. Part::GeomBSplineCurve::interpolate GeomAPI_Interpolate supports providing parameters at the interpolated points, which can be used to get some better curves (say by making centripetal Catmull-Rom splines).

As it is now, weights are not constrained to non-rational, which leads to lots of DoFs. Even after constraining weights to non-rational, it leads to more DoFs than necessary for interpolation.

I believe there I once modified a command (in Sketcher App) that automatically applies the weight equality constraints. I'll dig into that and see if it can be used. Indeed there are more DoFs than necessary, and I don't know what is the typical method people employ for that in software with more mature spline support.

It would be restrictive to not allow all possible curves satisfying the constraints. I think the best idea here would be to take users' input once this is ready.

I ran the same example of interpolating through 5 fixed points, with this tool (result is 7 poles) and generating a normal 5 pole BSpline and constraining it point on object. The number of DoFs is lower with the second method. It is easier to work with, if interpolation is all that is needed.

The problem here is that it is intended that the user only provide the interpolation points and the software take care from there. In that case, how do you suggest we differentiate between which of the provided points becomes knots and which become just point-on-object?

Do you have a roadmap of how many different new construction methods you want to implement?

For this particular project I just intend to add this and a periodic version. The word of the project is technically satisfied, but the results are not fully satisfactory. Apart from the possibility of non-uniformity (i.e. unequal gaps between knot parameters), I also want to add the ability to specify the degree of continuity at every knot.

In the future, this construction method may also be able to allow specifying tangents, but that's outside the scope of the present PR.

[abdullahtahiriyo]

@AjinkyaDahale

Ok. Yes, I think we could benefit quite a lot from additional input from users.

• We both understand that a fit can be done with a lesser amount of DoFs.
• We both understand that not constraining poles is "dangerous", as convergence to near zero weights leads to unexpected behaviour.
• It is unclear to both of us which are the user expectations, and to this respect whether more DoFs may be preferable in some (or majority) of applications or the contrary.

[freecadci]

for feature branch PR_8530. Pipeline 787726225 was triggered at dfa47c8. All CI branches and pipelines.

[AjinkyaDahale]

Here's a list of tasks remaining. I've divided them into things that can be done with present OCCT functionality, and things that may need a new algorithm.

With OCCT functionality:

• Clamp the weights (say by equality constraints) so they don't collapse to zero.
• Create non-uniform B-splines The default B-splines created in OCCT are already non-uniform.
• Fix Periodic B-splines. They are currently possible but the results are not great.

New functionality needed:

• Let user choose degree.
• Let user choose multiplicity of each knot.

[AjinkyaDahale]

I cannot compile the branch right now to check what we have.

If that is fixed and you gentlemen insist, we could go a two steps approach here.

Looked into it and the solution indeed was straightforward. With the latest commit periodic B-splines should automatically close.

I do not think the two situations are the same.

We are using slightly different definitions of "same" here. The behaviour is same to the extent that the closure does not happen. As you can see in the latest commit, the solutions also are the same. The only difference is in the response from OCCT after the command eventually reaches it, which you could rightly argue reflects differently to the user. Do note that in case of the regular tool, the resultant B-spline has repeat control points (which are coincident) instead of the last point being deleted.

Coming to the second issue, I think that's yet another deeper issue, since I tried just making the two points, without invoking the B-spline tool, and the vertices still (unreliably) disappeared.

Then this is for sure a separate issue. Hopefully somebody who can reproduce will debug and come with a solution.

I'll make a issue then.

Could you check the error of the CI?
From what I could tell it's from a test failure not related to this PR. In another branch in a fork also the test failed, but the same Branch succeeded in its PR.

[freecadci]

for feature branch PR_8530. Pipeline 813413127 was triggered at 5d760a4. All CI branches and pipelines.

[sliptonic]

Do we agree?

You guys are all over it and doing an awesome job! Thank you.

I will keep pushing for faster iteration in the future. It's unfortunate that we've caused people to rely on the development branch being stable. It's making us be too tentative about merging things. Faster iteration solves a lot of our problems and creates very few new ones.

[AjinkyaDahale]

@abdullahtahiriyo disappearing vertices issue reported: #8978.

[abdullahtahiriyo]

I will keep pushing for faster iteration in the future. It's unfortunate that we've caused people to rely on the development branch being stable. It's making us be too tentative about merging things. Faster iteration solves a lot of our problems and creates very few new ones.

Brad, here we have diverging views.

I think it is very fortunate that people rely on master being reasonably stable. Because this ensures we get timely feedback. If master became unstable, (a lot of) people would stop using it and a lot of feedback would only come from (yearly) releases. Intensive testing of master aside, within release cycle corrections are vital for FreeCAD success.

New features are shaped by the fast feedback only because there is feedback. There is feedback because a substantial amount of users understand they can rely on FreeCAD master being reasonably stable.

Of course there is a balance between quality and merge rate. As often in life, virtue lies on the right balance.

Specific of this PR, Ajinkya generally produces good quality code. Some corner cases with unacceptable behaviour were detected. Other unrelated issues were detected. We (mostly Ajinkya) separated ones from the others. Fixed the ones relating to this feature (I presume). And as soon as I have time (will definitely happen before the weekend), I will test and merge.

Some guidelines remain (this has nothing to do with this PR, it is just general information):

• A PR with well written code has much better chances of being merged early. It also puts a lower burden on the maintainer. For this, I recommend that every contributor critically reads their own code before making the PR.
• A well tested PR has much better chances of being merged early. It also puts a lower burden on the maintainer. For this, the contributor should have some minimal testing in place. Perhaps builds can be given for volunteer testers for some contributions as needed.

My approach may be perceived as egoistic as it leans toward maintainer time. Reality is that what I want is to merge right away. Review code once, test once and hit the button. This leaves more time available for me to review other PRs and to code my own ideas.

This does not mean I do no want to be consulted early if the developer sees impeding difficulties or have doubts about the soundness of the architecture being used. I should be consulted early to avoid loss to effort and frustration. It means that, when the situation is balanced, I can make an optimum allocation of resources, and often, life allowing, I can be reasonably responsive to all.

For times were I cannot be responsive, and other maintainers are not available or prefer to defer to me due to the nature of the PR, I would propose that contributors run development of different features in parallel, so that they are not stopped waiting. If the features are of different WBs, it may be better for diversification, but also several features of a single WB could work.

[freecadci]

for feature branch PR_8530. Pipeline 815889714 was triggered at d8fa4d2. All CI branches and pipelines.

[abdullahtahiriyo]

Yes!!

Thank you very much.