B-Spline Constraints: Fully Funded! Thanks everybody!

Info about new community or project announcements, implemented features, classes, modules or APIs. Might get technical!
PLEASE DO NOT POST HELP REQUESTS OR OTHER DISCUSSIONS HERE!
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: B-Spline Constraints: Almost Done! Help Reach Goal!

Post by abdullah »

jnxd wrote: Thu Nov 24, 2022 1:21 pm Just made some changes in error and grad that should discourage making the line degenerate (this is for #7607). This case doesn't hang any more. Could you take a look?

P.S: If you're reading this after Friday I will probably not be able to make changes (no PC in hand). We could revisit this when I'm back or if the changes are minor please try making them yourself if you have the time.
Yes, I have tested this today. I found it works very nicely now.

The only review remaining here now (from my side), is the code. I did do a first reading. I see the infrastructure for angle via point is being used internally for this line-knot tangency. I need to think if this is actually the way to go.

Anyway, this are minor things and there is now a strong prospect to merge the tangency at knot pretty soon.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: B-Spline Constraints: Fully Funded! Thanks everybody!

Post by jnxd »

The only review remaining here now (from my side), is the code. I did do a first reading. I see the infrastructure for angle via point is being used internally for this line-knot tangency. I need to think if this is actually the way to go.
@abdullah totally agree that this doesn't seem well. Im open to ideas.
My latest (or last) project: B-spline Construction Project.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: B-Spline Constraints: Fully Funded! Thanks everybody!

Post by abdullah »

jnxd wrote: Mon Nov 28, 2022 6:02 am
The only review remaining here now (from my side), is the code. I did do a first reading. I see the infrastructure for angle via point is being used internally for this line-knot tangency. I need to think if this is actually the way to go.
@abdullah totally agree that this doesn't seem well. Im open to ideas.

I am in the middle of figuring out the future impact on the Python API.

In the meantime, I have noted what seems to be a graphic inconsistency with other tangency constraints between an point and a curve. I do understand that a knot is not an endpoint, but I would like to share my thoughts, in case we can improve this representation.

For the "tangency at knot", when applied, the representation is this:
Screenshot_20221201_181717.png
Screenshot_20221201_181717.png (19.72 KiB) Viewed 278032 times
This is: there is a tangency constraint icon and a point on object icon.

This is different from the endpoint to curve tangency, where only a sketcher tangency constraint is present:
Screenshot_20221201_181916.png
Screenshot_20221201_181916.png (7.68 KiB) Viewed 278032 times
Aside from the pure graphic inconsistency, I think we might be creating an unwanted situation. If after creation, we remove the point on object constraint, the line can be moved away from the knot and it appears to lead to a situation where the tangency makes no sense (or I am not able to make sense out of it):
Peek 01-12-2022 18-25.gif
Peek 01-12-2022 18-25.gif (579.96 KiB) Viewed 278032 times
Somehow, I wonder if it would not make much more sense to make the "point on object" part of the constraint solver only. In other words, that when processing the tangency constraint, the corresponding "point on object" at solver level is added, so the behaviour is the same as now, but at sketcher level only the tangency is shown. This would prevent the unwanted situation above.

On a further thought, I wonder if it would be a better option then to switch to the Point-Curve tangency constraint format:

Code: Select all

addConstraint(Sketcher.Constraint('Tangent',11,2,10)) 
Currently this format is only used for endpoint to curve tangencies (I think). Because a knot is not an endpoint, we would be overloading the syntax, but without interfering with the endpoint-to-curve syntax, which remains possible (also for B-Spline in the future), at the cost of checking whether we are handling an endpoint or a knot at sketch.cpp level.

Do you think this makes sense? Do you see any hindrance (that I might have missed)?
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: B-Spline Constraints: Fully Funded! Thanks everybody!

Post by jnxd »

Do you think this makes sense? Do you see any hindrance (that I might have missed)?
I think it doesn't make sense to make just tangent here when tangent-at-point exists, but I'll get back sometime once I have my hands on a computer.

BTW, this is not the only place where one tangent command creates two constraints of PoO and tangent. See the relevant code for three selection tangent constraint when the point is not an end point of either curve. Admittedly this needs to be streamlined, but I believe the streamlining needs to be done throughout the interface not just one case. Again, I'll get back in a couple of days.
My latest (or last) project: B-spline Construction Project.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: B-Spline Constraints: Fully Funded! Thanks everybody!

Post by abdullah »

It is ok for me to exchange arguments about this when you have time, and I think it is much more important that it may seem at a first glance. A solver constraint can be changed or improved with restricted impact (if it provides the same functionality in a different way, without impact at all). The decision on how to code sketcher constraints impacts the Python interface and even storage/serialisation. Changing it afterwards may imply macros breaking (Python impact) or problems to provide backwards compatibility with legacy files (if one sketcher constraint was stored in one way and that way of storing needs to be used to code a different situation).
jnxd wrote: Thu Dec 01, 2022 6:52 pm BTW, this is not the only place where one tangent command creates two constraints of PoO and tangent. See the relevant code for three selection tangent constraint when the point is not an end point of either curve.
This case is still different. A ternary tangency creates a TangentViaPoint constraint which internally uses AngleViaPoint solver constraint and can provide a tangency with any curve. There is an expectation that a ternary tangency (i.e. a AngleViaPoint), when supported for a type, provides tangency between any set curves. The Tangent at knot does not use the AngleViaPoint infrastructure and can only provide tangencies with lines.

In fact, because of the current reuse of AngleViaPoint infrastructure there appears to be a bug, that the branch allows to make an tangency between a circle and a knot, or an ellipse and a knot, while not yet supported.
jnxd wrote: Thu Dec 01, 2022 6:52 pm I think it doesn't make sense to make just tangent here when tangent-at-point exists, but I'll get back sometime once I have my hands on a computer.
I think that depiction of tangent at knot as tangent at point may be misleading. This may also be at the bottom of view of (lack of) homogeneity of interface.

TangentViaPoint is a set of functionality originally created to provide tangencies between (complex) curves. It avoids having to create a specific solver constraint for each pair of curves (example ellipse-hyperbole, ellipse-parabola). In fact, it is enough that a new curve implements point on object and is able to provide its normal at parameter u, for it to work. In this type of constraint, a point is introduced at sketcher level. The point is not part of, or is associated with any of the curves (e.g. it is not an internal geometry point). It may be used to block the point of tangency at a given position (for example at an angle with respect to an axis). It does come with the issue that the tangency is a compound of one tangency and two point on objects. This is also characteristic, the point is point on object restricted to both curves.

At the time, we tried making this point "solver only", but this gave rise to problems with diagnostic (and at sketcher level diagnostic, i.e. the DoFs and accurate detection of conflicting/redundant constraints, is arguably the most important feature of all). The decision was to allow the suboptimal "two sketcher constraints" solution, with the understanding that it was a corner case (only for complex curves, as simple curves have satisfactory specific solutions). At the moment we did not see further. It begs the follow up question: Can this be done in another way? Yes. This may include, for example, specific constraints for each pair, or a new tangency constraint encapsulating the three constraints (but constraints have currently limitations of how many curves/positions can store). It is not straightforward (or without impact), but I am not against discussing possible improvements to this. We also need to take care of backwards compatibility (constraints are stored independently now). This can be a separate effort.

There is another important point before I go on with the comparison. Although it does not concern the sketcher level representation of constraints, endpoint tangencies, as well as endpoint to curve tangencies are all implemented with AngleViaPoint (curve to curve are not, unless it is the situation above). This means that despite having a different Python interface (which does not point at all to a TangentViaPoint), at solver level and in a way transparent to the user, many tangencies are internally implemented with the same AngleViaPoint solver code. So, while using TangentViaPoint sets some expectations on the interface (Python interface), it is possible to implement some constraint where these expectations are not there using AngleViaPoint. This can cause confusion, and this is why I indicate it here. However, we need to separate the intention provided by the Python interface from the actual implementation at solver level. The implementation at solver level can reasonably change transparently to the user. The Python interface cannot change in a transparent way to the user. For endpoint to endpoint, or endpoint to curve tangencies, the Python code looks like this:
App.getDocument('hang').getObject('Sketch').addConstraint(Sketcher.Constraint('Tangent',17,1,18,2))
App.getDocument('hang').getObject('Sketch').addConstraint(Sketcher.Constraint('Tangent',19,2,18))
For a TangentviaPoint, it looks like this:
>>> App.getDocument('hang').getObject('Sketch').addGeometry(Part.Point(App.Vector(-55.721522,45.582154,0)))
>>> App.getDocument('hang').getObject('Sketch').addConstraint(Sketcher.Constraint('PointOnObject',16,1,11))
>>> App.getDocument('hang').getObject('Sketch').addConstraint(Sketcher.Constraint('PointOnObject',16,1,10))
>>> App.getDocument('hang').getObject('Sketch').addConstraint(Sketcher.Constraint('TangentViaPoint',11,10,16,1))
Internally, at solver level all the tangencies above are AngleViaPoint constraints (with the angle being fixed at solver level so to be tangent). In the former, the end-point-to curve also adds a solver level PointOnObject as part of the tangency constraint. This is done fully transparently for the user. In the latter, the pointOnObject constraints are sketcher level.

Now, coming back to the second part of the original discussion. A knot is an internal alignment geometry. For a given B-Spline, the knots are at fixed positions (i.e. changing the positions of the knots do change the B-Spline, leading to a different B-Spline). These are the first two differences (above the point is "free" before tangency is applied, above the point defines the point of tangency over two curves from a continuum of possible tangency positions). A tangency at a knot position is perfectly defined by the knot position itself. This also leads to a further difference, while above there are two sketcher level point on object constraints, in the current implementation tangent at knot position, there is only one sketcher level point on object involved.

These differences make it possible for a tangent at knot, to be implement the PointonObject solver level constraint transparently to the user, as in the case of the curve-to-endpoint indicated above.

In addition, the tangent at knot does not make use of the generic AngleViaPoint infrastructure, but uses a specific constraint at solver level (which is valid exclusively for lines). Therefore, it should be separated from the code adding an AngleViaPoint (the same as the "simple tangencies" between curves are separated from the ones using AngleViaPoint).

In fact, it may be possible for you or for another developer in the future, to write the tangency function for B-Splines used by AngleViaPoint. Together with two point on object (support needs to be there as well), it could provide using AngleViaPoint all the tangencies at any position of a B-Spline with another curve (at the cost of a sketcher level point, two sketcher level PoO and one tangency constraint in the current infrastructure, or else if we develop the infrastructure further as discussed above to remove such limitation). For this, I think that keeping the TangentViaPoint Python interface for this specific case of tangency is more sound.

Please, give it a thought and come back as necessary.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: B-Spline Constraints: Fully Funded! Thanks everybody!

Post by jnxd »

@abdullah, as you mentioned multiple times in the last post, let's get to it when I have the time.

However, regarding this statement:
In fact, it may be possible for you or for another developer in the future, to write the tangency function for B-Splines used by AngleViaPoint.
There is this currently implemented as a "proof-of-concept": https://github.com/AjinkyaDahale/FreeCA ... to-bspline. Is that what you meant?
My latest (or last) project: B-spline Construction Project.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: B-Spline Constraints: Fully Funded! Thanks everybody!

Post by abdullah »

jnxd wrote: Sat Dec 03, 2022 8:49 am There is this currently implemented as a "proof-of-concept": https://github.com/AjinkyaDahale/FreeCA ... to-bspline. Is that what you meant?
No, it is not.

There, there is a new solver constraint to handle BSpline to line tangency. It does not use the solver contraint AngleViaPoint to implement the tangency. This also means it won't be able to handle BSpline to any other curve tangency (including BSpline to BSpline, or Ellipse to BSpline,...).

In that branch (as in the tangency at knot branch), the approach is to use the Python infrastructure of TangencyViaPoint (which is implemented at solver level as AngleViaPoint) to convey the constraint information. Then, at some point, this is intercepted and instead of creating an AngleViaPoint, the specific constraint is created. This is what I mean:

In int Sketch::addAngleAtPointConstraint, there is:

Code: Select all

if ((Geoms[geoId1].type == BSpline && Geoms[geoId2].type == Line) ||
        (Geoms[geoId1].type == Line && Geoms[geoId2].type == BSpline)) {
which intercepts the specific case and creates a case specific constraint, instead of handling it via the AngleViaPoint call at the end of the function:

Code: Select all

GCSsys.addConstraintAngleViaPoint(*crv1, *crv2, p, angle, tag, driving);
The latter function would use this other one:

Code: Select all

DeriVector2 BSpline::CalculateNormal(const Point &p, const double* derivparam) const
to create a BSpline to any other curve tangency, using AngleViaPoint.

However, that function is not yet coded to provide the normal at any parameter of the B-Spline, only on the endpoints (which provides the current end-point tangency for B-Splines in master code).

When one has that function working over the whole range of the parameter, and point on B-Spline constraint also working, then it should be possible to use the generic TangencyViaPoint infrastructure.

See also:
https://github.com/FreeCAD/FreeCAD/blob ... /Geo.h#L96
https://github.com/FreeCAD/FreeCAD/blob ... .cpp#L1992
https://github.com/FreeCAD/FreeCAD/blob ... .cpp#L2010

Then, I want to say that it is not mandatory that all curve-to-curve tangencies are processed as TangencyViaPoint. It is possible to have "simple tangencies", like in here:
https://github.com/FreeCAD/FreeCAD/blob ... .cpp#L1714

So, for example, B-Spline to line, could be handled with a specific constraint, while B-Spline to Ellipse be handled with TangencyViaPoint. Here, performance, stability... can be used as arguments for the trade off. It is also possible to implement all via TangencyViaPoint, but then it should be via solver AngleViaPoint constraint.

If a simple tangency is what we want (or more generally, if we are not using AngleViaPoint solver constraint), we should not call this:
https://github.com/FreeCAD/FreeCAD/blob ... .cpp#L1726

Probably counterintuitively, one of the most important considerations is the Python interface, that needs to show intention and be reasonably futureproof.

So there are two vectors in the decision making:
1. How to handle the tangency at UI level and translate it into the right Python command.

1a. It can be to reuse TangencyViaPoint in Python if future proof and showing the correct intention, or using another Python command (not TangencyViaPoint).

1b. In any case, creating the right amount of other constraints (or even free points when necessary) and encapsulating at solver level point on objects as much as possible.

2. How to actually translate the Python command into the solver.

Aside from the fact that the code needs to show intention (so TangencyViaPoint code should not be intermixed with code not using AngleViaPoint at solver level), the user does not care how the problem is ultimately solved, so any solution is possible. This even provides for changing the solution in the future with a better one in a transparent fashion for the user.
jnxd wrote: Sat Dec 03, 2022 8:49 am as you mentioned multiple times in the last post, let's get to it when I have the time.
I am trying that you have all the information to understand why I think what we are discussing is important, so when you have the time, you can process it and come back with counterarguments or proposals.

My intention is to merge this before Christmas. It is very useful.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: B-Spline Constraints: Fully Funded! Thanks everybody!

Post by jnxd »

abdullah wrote: Sat Nov 26, 2022 6:43 am
jnxd wrote: Thu Nov 24, 2022 4:37 am Could you point to past such cases?
It will require quite some effort, as it requires to dig out closed tickets relating to driven constraints. Probably finding the commit where I put the driven constraints outside the diagnosis will help.
OK. Let's get to this when (or if) we get to this.
abdullah wrote: Sat Nov 26, 2022 6:43 am I guess you did it because you saw a simplification of the expressions or of the solution. Providing a value of the constraint sounds like a solution, rather an a problem being solved. The motivation for adopting a solution is generally solving a problem (or making it easier to solve a problem).
OK you caught me. I believe the way it went was:
1. I thought of finding the closest "u" whenever it's needed. That may not be too difficult (although I predict issues) in error, but I had questions how it changes with small changes in other variables for grad.
2. I figured out the simplification by adding "u" as a parameter.
3. I rationalized that this can become a "value" of point-on-curve. If this can be exposed and made driving, that is the equivalent of locking a point at a parameter, which can be seen as a generalization of knot constraints in B-splines.
abdullah wrote: Sat Nov 26, 2022 6:43 am In the end, point on object is generally about minimising a distance, so it only makes sense that one needs to know the closest point of the BSpline to the point to be snap, so as to calculate error and gradient. It does not sound that bad to me that this calculation is necessary. How often the calculation may be necessary (to which extend a previous calculation may be reused) appears to be as a different problem.
To the best of my understanding, the only way to find the appropriate "u" is to do so numerically. This will have to be done in every call to error and grad.
A key thought is: Can "u" change if none of the other parameters of the curve (or point) change? The answer to this question should be no (or I am very wrong and need to be corrected right away).
This becomes a question of semantics. "u" can change without other parameters changing, but the value of error will also change. To this extent, think of it again as a value of the constraint. Can the parameter "distance between points" change without changing any other related parameter of the points?
If this is the case, "u" does not need to be a parameter of the system, but can be a value stored within the constraint itself. This value can be updated upon changes to curve/point parameters, and the gradient contribution because of its change can be factored in changes to the other parameters of the curve (in other words, its value then can contribute to the gradients and errors as in the current formulas).

Alternatively, "u" may not be stored at all, but calculated in each iteration. This might also help in building a Point on Object that does not follow the current piece when dragging, but that appropriately jumps to the next piece, when having regard to poles affecting in addition adjacent pieces (not fully sure of this as there are too many open points).

Does this sound reasonable to you or do you see major flaws?
I believe the biggest "flaw" in the alternatives you suggested is the need for numerical methods for finding "u" and the complications that come with it. This could turn out to be a non-issue, however, so I guess the best way is to implement one of the solutions and see if it behaves reasonably well.

As for which one to implement, I believe that "u" needs to be kept stored so that it can be used as a good initial value so long as nothing changes too drastically (this won't happen when dragging but can if some dimensional constraints are changed too much). The issue of following current piece is a different problem (AFAICT) and can be handled independently.
My latest (or last) project: B-spline Construction Project.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: B-Spline Constraints: Fully Funded! Thanks everybody!

Post by jnxd »

abdullah wrote: Sun Dec 04, 2022 6:40 am So there are two vectors in the decision making:
1. How to handle the tangency at UI level and translate it into the right Python command.

1a. It can be to reuse TangencyViaPoint in Python if future proof and showing the correct intention, or using another Python command (not TangencyViaPoint).

1b. In any case, creating the right amount of other constraints (or even free points when necessary) and encapsulating at solver level point on objects as much as possible.

2. How to actually translate the Python command into the solver.

Aside from the fact that the code needs to show intention (so TangencyViaPoint code should not be intermixed with code not using AngleViaPoint at solver level), the user does not care how the problem is ultimately solved, so any solution is possible. This even provides for changing the solution in the future with a better one in a transparent fashion for the user.
The thing is we need a point, and any simple tangency is (currently) a two-argument constraint. On the Python side, a tangency with an additional point already has a name so it makes sense to use that, or make an additional constraint. Just calling it tangency can get confusing quickly.

I guess an additional constraint like "TangencyAtInternalPoint" is a possibility. This will also make possible, say, tangency at the "tips" of an ellipse (though that's already served by other commands anyway).

On the translation to solver side, I do believe that "TangencyViaPoint" does semantically sound like it should encompass the present code, since there's a point and there's a tangency at it. But from what I gather from your previous message, this was itself a "compromise". I'm not opposed to the idea of keeping exclusively for constraints that use AngleViaPoint, but if so let's explicitly document it somewhere, either in the function's description or in implementation.

As a side note here, do you see a harm in moving the point-on-object constraints associated with TangencyViaPoint to the solver? So in python it is just

Code: Select all

>>> App.getDocument('hang').getObject('Sketch').addGeometry(Part.Point(App.Vector(-55.721522,45.582154,0)))
>>> App.getDocument('hang').getObject('Sketch').addConstraint(Sketcher.Constraint('TangentViaPoint',11,10,16,1)) 
and that adds the point on object constraints (16 on 11 and 16 on 10) in "TangenctViaPoint"? This will make some existing scripts create redundant constraints, but will make future work a little more streamlined, since removing the point-on-object constraints shouldn't be possible.
abdullah wrote: Sun Dec 04, 2022 6:40 am My intention is to merge this before Christmas. It is very useful.
That sounds doable, even if that's just 15 days away.
My latest (or last) project: B-spline Construction Project.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: B-Spline Constraints: Fully Funded! Thanks everybody!

Post by jnxd »

abdullah wrote: Sun Dec 04, 2022 6:40 am

Code: Select all

GCSsys.addConstraintAngleViaPoint(*crv1, *crv2, p, angle, tag, driving);
The latter function would use this other one:

Code: Select all

DeriVector2 BSpline::CalculateNormal(const Point &p, const double* derivparam) const
to create a BSpline to any other curve tangency, using AngleViaPoint.

However, that function is not yet coded to provide the normal at any parameter of the B-Spline, only on the endpoints (which provides the current end-point tangency for B-Splines in master code).
That's straightforward for any parameter value, but at any point sounds like we need to find the closest point first. Or if there's a generalization of the normal that doesn't need to find the closest point, that would also be good (for example for a circle this is the vector from the center to the point.
My latest (or last) project: B-spline Construction Project.
Post Reply