Sketcher: Bezier curves

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sketcher: Bezier curves

Post by wmayer »

The doubt I have is whether the ones in SketchObject.cpp should return bool or should be void and rise exceptions for any "return false".
It would be good for real errors to raise an exception and let the calling instance (here the command) handle it. At the moment when returning false it sometimes means that the function fails due to wrong input parameters and sometimes to due a real problem. So, the calling instance cannot distinguish between these cases and thus it would be better to raise exception when needed. But don't just use Base::Exception but one if its sub-classes.
- If you are using OCC>=6.9.0, then you should see the knots and be able to tweak their multiplicity (no icons a this time, see the drop down big "X" in the bspline bar).
It isn't very intuitive. When using the command to increase multiplicity it always complains with: None of the selected elements is a knot of a bspline or the knot has already reached the maximum multiplicity. Apparently the selection is wrong because I selected the whole curve. So, the command should give a better error message.

After a while I figured out that the points lying on the spline represent a knot. When clicking on it then the command has done something but it also raises an error (but don't know when exactly): Gui::Command::activated(0): Unknown C++ exception thrown

At some point it happens that the whole spline is corrupted and I cannot even close the sketch any more.

Code: Select all

unknown C++ exception in ViewProvider::eventCallbackUnhandled unknown C++ exception
I then have to kill the application to get out of this state.

OS: Windows 7
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.17.10235 +33 (Git)
Build type: Release
Branch: sketcher_bspline
Hash: 1f651137b7185d70e2fa814e5f683e4522019501
Python version: 2.7.8
Qt version: 4.8.7
Coin version: 4.0.0a
OCC version: 7.0.0
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

wmayer wrote:It would be good for real errors to raise an exception and let the calling instance (here the command) handle it. At the moment when returning false it sometimes means that the function fails due to wrong input parameters and sometimes to due a real problem. So, the calling instance cannot distinguish between these cases and thus it would be better to raise exception when needed. But don't just use Base::Exception but one if its sub-classes.
Thank you for your insight. So as far as I understand, you would not discard maintaining the boolean return type while raising exceptions. I have fixated in your separation between for example failing due to wrong input parameters and due to a real problem. I might have a misconception.

When I call:

Code: Select all

               try {
                    Gui::Command::doCommand(
                        Doc,"App.ActiveDocument.%s.modifyBSplineKnotMultiplicity(%d,%d,%d) ",
                        selection[0].getFeatName(),(*it)->Second, (*it)->InternalAlignmentIndex + 1, 1);

                    applied = true;

                    // Warning: GeoId list might have changed as the consequence of deleting pole circles and
                    // particularly bspline GeoID might have changed.

                }
                catch (const Base::Exception& e) {
                    Base::Console().Error("%s\n", e.what());
                }
I cannot know whether:
bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int multiplicityincr)

returned true or false. Or maybe is there a way?
wmayer wrote:It isn't very intuitive. When using the command to increase multiplicity it always complains with: None of the selected elements is a knot of a bspline or the knot has already reached the maximum multiplicity. Apparently the selection is wrong because I selected the whole curve. So, the command should give a better error message.
Ok. Here I am not sure what to do. Because I coded it, for me it has always been obvious that the points on the bspline where the knots, so obvious that I never questioned it. What do we need to make it obvious that those points are the knots?

About the error message:
wrong_selection.png
wrong_selection.png (24.88 KiB) Viewed 1988 times
The window title is "Wrong selection" and the content indicates what the command expects, i.e. a knot. Do you have any proposal about how to improve it?
wmayer wrote:After a while I figured out that the points lying on the spline represent a knot. When clicking on it then the command has done something
bspline_knot_mult.png
bspline_knot_mult.png (22.34 KiB) Viewed 1988 times
bspline_knot_mult_after_increase.png
bspline_knot_mult_after_increase.png (22.91 KiB) Viewed 1988 times
bspline_knot_mult_after_increase_and_subsequent_decrease.png
bspline_knot_mult_after_increase_and_subsequent_decrease.png (19.96 KiB) Viewed 1988 times
wmayer wrote:but it also raises an error (but don't know when exactly): Gui::Command::activated(0): Unknown C++ exception thrown
This is unexpected. I have been testing this quite a lot (because of the issues I have with the 5 pole-bsplines that sometimes do not work) and I never came across such a thing. I am more than interested in a way to reproduce it or any other further information that could help to narrow it down.

On the lack of intuitiveness of not having icons

The dropdown toolbar icon has "memory", so if the last time you selected "Increase multiplicity" upon direct clicking without dropping down, it will increase, but if you last selected "Decrease multiplicity", it will decrease it. I tell you this because I have had unexpected results because I wanted to increase the multiplicity, but actually the command decreased it.

point selection is not cleared

I took the decision not to clear the selection when executing the commands (so that you can continue changing multiplicity of the same knot). For increasing after increasing, it seems to be a good idea, but when it involves decreasing I have realised it does not work so well, so I am going to change it to always clear it.

Note: I have managed to segfault FC by fully removing one knot (ask to decrease multiplicity when multiplicity was 1). This is a bug to correct.
I have not been able to make it complain in my tests.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

abdullah wrote:point selection is not cleared

I took the decision not to clear the selection when executing the commands (so that you can continue changing multiplicity of the same knot). For increasing after increasing, it seems to be a good idea, but when it involves decreasing I have realised it does not work so well, so I am going to change it to always clear it.

Note: I have managed to segfault FC by fully removing one knot (ask to decrease multiplicity when multiplicity was 1). This is a bug to correct.
I have not been able to make it complain in my tests.
Just pushed a commit clearing the selection after multiplicity change request and fixing the segfault when removing one knot.

There is another bug that when removing one knot the internal alignment geometry is not restored AND that the knot point is not actually deleted. I have to take a further look in my next coding slot.
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sketcher: Bezier curves

Post by wmayer »

Note: I have managed to segfault FC by fully removing one knot (ask to decrease multiplicity when multiplicity was 1). This is a bug to correct.
I have not been able to make it complain in my tests.
This might be the same issue I encountered but on Windows it doesn't segfault due to compiler settings and instead raise the Gui::Command::activated(0): Unknown C++ exception thrown
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

wmayer wrote:
Note: I have managed to segfault FC by fully removing one knot (ask to decrease multiplicity when multiplicity was 1). This is a bug to correct.
I have not been able to make it complain in my tests.
This might be the same issue I encountered but on Windows it doesn't segfault due to compiler settings and instead raise the Gui::Command::activated(0): Unknown C++ exception thrown
Dear Werner,

I have implemented a unique identifier for geometry, as means to retrieve the right geometry after deletion (where our GeoId identifier might have changed depending on the order of the deleted geometry in the list). This would probably be useful also to improve reported problems with the delete command in the sketcher in the future.

I have based my code in the unique identifier used in the constraints. I have changed the random number generator seed initializer to use ctime instead of QT, as it is my understanding that Part should not contained QT code.

I would really appreciate if you could take a look to this specific commit and tell me if it is acceptable, as I will continue developing on top of this functionality:
https://github.com/abdullahtahiriyo/Fre ... 6d3a01bd4c

BTW, the bug should be gone now.

Thanks in advance.

New list of tasks before merge:
1. Exception throwing instead/in addition to boolean returning (in the previous list)
2. Review all the bspline functions to use unique identifier instead of the tricky re-read the selection implementation that does not always work.
3. Avoid that construction points (e.g. knots) are constrained (lock constraint,...) as they are fixed solver geometry.
4. (facultative) implement some iterative procedure to make the curve change as little as possible when removing a knot or reducing its multiplicity.
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sketcher: Bezier curves

Post by wmayer »

I have implemented a unique identifier for geometry, as means to retrieve the right geometry after deletion (where our GeoId identifier might have changed depending on the order of the deleted geometry in the list). This would probably be useful also to improve reported problems with the delete command in the sketcher in the future.
Well, if it's needed to fix this issue it's OK for me.
I have based my code in the unique identifier used in the constraints. I have changed the random number generator seed initializer to use ctime instead of QT, as it is my understanding that Part should not contained QT code.
There is basically no restriction not to use Qt stuff in App. You are just not allowed to use Qt GUI classes in App modules.
I would really appreciate if you could take a look to this specific commit and tell me if it is acceptable, as I will continue developing on top of this functionality:
Thinking a bit further. A clone of a geometry is an independent copy, i.e. it doesn't share the underlying OCC geometry with the original. But a clone will get the same tag id as the original and I am not sure if this will lead to problems in the future. Also, you can create a copy by using the methods setHandle/handle but here the copy will have a different tag id so there is a certain inconsistency in the API.

Am I right that it's a key point in the context of the sketcher that a clone has the same tag id as the original? So it might be better not to copy the tag id in the clone() method but assign it explicitly when needed.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

wmayer wrote:Thinking a bit further. A clone of a geometry is an independent copy, i.e. it doesn't share the underlying OCC geometry with the original. But a clone will get the same tag id as the original and I am not sure if this will lead to problems in the future.
In the context of the sketcher, a clone is extensively used by the property subsystem to make copies (that do not share the underlying "data members" with the original). This works for constraints as currently in master and this geometry unique identifier is a symmetric extension to that. Constraints may change (value for example), but still they keep their identifier (which allows for example keeping up to date the expression engine). Still within the undo stack, different versions of constraints coexist with the same identifier and different member data (in order to allow to undo). Constraints include a clone and a copy method. Most operations use clone as the objective is to keep track of a constraint evolution. A few operations require copy, because the objective is not to evolve a constraint, but to create a new one. For example, this is the case of sketcher arrays or symmetry tool, where copies are created to be independent constraints simultaneously in use.

Note that these identifiers are runtime unique, they are not handled by the persistence methods.
wmayer wrote:Also, you can create a copy by using the methods setHandle/handle but here the copy will have a different tag id so there is a certain inconsistency in the API.
The methods setHandle/handle do a copy process that is not a clone within the meaning of above. If any portion of code is currently using clone when the intention is to generate a separate entity, that portion should change for consistency. For example, imagine that unique identifiers were the solution to fix the geometry deletion issues, if the sketcher symmetry creating methods use clone, they should change ways (either use the setHandle/handle or we could implement a copy member to allow more concise coding). Conversely, if a method evolving a geometry currently uses copy constructor, it should be rewritten to use clone or at least to manually assign a tag. I think you get the idea, they fit different use cases. Both should coexist happily.
wmayer wrote: Am I right that it's a key point in the context of the sketcher that a clone has the same tag id as the original? So it might be better not to copy the tag id in the clone() method but assign it explicitly when needed.
Yes, you are right that it is the key point and only reason for proposing it.

As you certainly have noticed, I am talking about the sketcher, as I am unaware of any implication in any other context. As no other part of FreeCAD uses the unique identifier today, nothing will break to leave the assignment inside the clone as proposed. However, I agree it is indeed worth to leave in Part::Geometry a structure from which other parts of FreeCAD could benefit. Whether one or the other solution is best for FreeCAD in general is outside my current overall FreeCAD knowledge.

What I see today is:

If the tag assignment is left outside clone(), then more sketcher code has to be changed (mostly related propertylists assigning clones when evolving a geometry). Additionally, you then will not have the symmetry of use of unique identifiers between geometry and constraints.
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sketcher: Bezier curves

Post by wmayer »

Note that these identifiers are runtime unique, they are not handled by the persistence methods.
Yes, I have expected this.
Conversely, if a method evolving a geometry currently uses copy constructor, it should be rewritten to use clone or at least to manually assign a tag.
Copy constructors are explicitly disallowed in the Geometry class framework. Some classes offer an overloaded constructor where the handle to the actual geometry can be passed or only the default constructor where a copy can be created with setHandle. Here it's not possible to assign the tag id (and anyway not what I like) to the copy because it doesn't know where the handle comes from.
As you certainly have noticed, I am talking about the sketcher, as I am unaware of any implication in any other context.
When introducing new stuff for a certain purpose we cannot expect that it's only used exactly in this context it was originally aimed for. But if it's too complicated to force this code-wise then at least the documentation must make it clear what to expect and what not.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

Thanks for taking the time to discuss this. I really appreciate it. I will be verbose to try to expose problems. Let me know if you see flawed arguments or misconceptions.
wmayer wrote: Copy constructors are explicitly disallowed in the Geometry class framework. Some classes offer an overloaded constructor where the handle to the actual geometry can be passed
Ok. This was bad terminology. Thanks for pointing it out. Somehow in my head I mixed the intended purpose with the actual syntax. It is not a copy constructor as does not take the same object, but a handle to the OCC object. It is just an overloaded constructor. Any constructor, as per default c++ behaviour will call the default "parent class(es)" constructor(s) and then execute the overloaded constructor. So there will be no problem that the tag is not initialized to be unique per default, the perceived problem is "just" that the tag cannot be set to something we already have.

1. All geometry derivatives implement the clone function as per Geometry class requirement. So all of them can create a clone in the sense of a copy with the same tag.
2. If clone is not used and the overloaded constructor is used, then a newly generated tag is created by default (so not a clone, but a copy).

I want to think that not allowing to assign a tag is intended behaviour. If this is not the case, then I think we would need to write overloaded versions of those constructors and functions taking an additional parameter representing a tag (or Geometry object having the tag to copy). But let's assume it is intended behaviour for a moment. The question evolves to: do I have the tools to create clones and copies in all the situations which I can face today?

I mean I can have handles of different types, trimmed curves, and still want to clone (copying is not a problem with the current implementation). So I am here with a handle and a Geometry object having the tag I would like to clone.

I would tend to think that developers should be told, via documentation, that if you want a clone, you can not use an overloaded constructor. If you use an overloaded constructor you get a copy. So, how do I clone if I want to?

I would say that the solution is with the second way you just mentioned:
wmayer wrote:or only the default constructor where a copy can be created with setHandle
In this case:

1. If I want to "clone", I can just use the clone function for construction purposes and then assign the handle.
2. If I want to "copy", then I just use "any constructor" to get a random tag and then assign the handle.

Am I wrong?

If I am not wrong, this I think it could be a great answer for the problem in the previous point. We just create setHandle functions in the Geometry derivates where they are missing. The if you want to clone, you use the clone function, then assign the handle.

What do you think?
wmayer wrote:Here it's not possible to assign the tag id (and anyway not what I like) to the copy because it doesn't know where the handle comes from.
I am not sure I am understanding you well. I understand that you are pointing out a potential problem (not being able to assign the tag id) to make me aware (thanks!). I perceive that in principle, if there is not a problem today AND if after thinking there is not a perceived need for it, you would prefer not to allow assigning tag ids. Right?

If this is it, I do not think we have the need of allowing to assign an arbitrary tag today and I can not think with the information I have today of a case where it would be needed. If we ever need that, the best solution I can come with is to overload the "overloaded constructors"/sethandle functions with new versions that take an extra parameter, the tag to be assigned. If you want your arbitrary tag, you use the overloaded version with the tag, if not you can just use the version available as of today.

Some reference of combinations of overloaded constructors and sethandle methods:

setHandle alone:
GeomLineSegment (trimmed curve)

setHandle AND overloaded constructor with same type handle, itself (e.g. Handle_Geom_Ellipse for an Ellipse):
GeomBezierCurve
GeomBSplineCurve
GeomArcOfCircle
GeomEllipse
GeomOffsetCurve (this has an additional overloaded constructor for Handle_Geom_Curve)
GeomTrimmedCurve
GeomBSplineSurface
GeomCylinder
GeomCone
GeomSphere
GeomToroid
GeomPlane
GeomOffsetSurface (this has an additional overloaded constructor for Handle_Geom_Surface)
GeomPlateSurface (this has an additional overloaded constructor for GeomPlate_BuildPlateSurface and another for Handle_Geom_Surface)
GeomTrimmedSurface
GeomSurfaceOfRevolution (this has an additional overloaded constructor for Handle_Geom_Curve)
GeomSurfaceOfExtrusion (this has an additional overloaded constructor for Handle_Geom_Curve)


setHandle with trimmed curve handle AND overloaded constructor with underlaying untrimmed curve handle (e.g. Handle_Geome_Circle for ArcOfCircle):

GeomArcOfEllipse
GeomArcOfHyperbola
GeomArcOfParabola

No setHandle, only overloaded constructors:
GeomPoint (Handle_Geom_CartesianPoint and Base::Vector3d)
GeomCircle
GeomHyperbola
GeomParabola
GeomLine (Handle_Geom_Line and Base::Vector3d)
GeomBezierSurface
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

abdullah wrote:New list of tasks before merge:
1. Exception throwing instead/in addition to boolean returning (in the previous list)
2. Review all the bspline functions to use unique identifier instead of the tricky re-read the selection implementation that does not always work.
3. Avoid that construction points (e.g. knots) are constrained (lock constraint,...) as they are fixed solver geometry.
4. (facultative) implement some iterative procedure to make the curve change as little as possible when removing a knot or reducing its multiplicity.
Today I invested in making the Geometry code homogeneous to enable to use the setHandle in all the instances. I refactored all the overloaded constructors taking a handle to use the setHandle function instead of repeating code.

Then I did 2.

Then I gave a shot to (1). I modified all the false returns with exceptions to try to find my way, like this:

Code: Select all

bool SketchObject::modifyBSplineKnotMultiplicity(int GeoId, int knotIndex, int multiplicityincr)
{
    #if OCC_VERSION_HEX < 0x060900
    Base::Console().Error("This version of OCE/OCC does not support knot operation. You need 6.9.0 or higher\n");
    return false;
    #endif
    
    if (GeoId < 0 || GeoId > getHighestCurveIndex())
        throw Base::ValueError("BSpline GeoId is out of bounds.");
    
    if (multiplicityincr == 0) // no change in multiplicity
        throw Base::ValueError("You are requesting no change in knot multiplicity.");
    
    const Part::Geometry *geo = getGeometry(GeoId);
    
    if(geo->getTypeId() != Part::GeomBSplineCurve::getClassTypeId())
        throw Base::TypeError("The GeoId provided is not a B-spline curve");

...
Then the exception propagates to the console, or the UI, but the messages are like this:
freecad_exception_propagation.png
freecad_exception_propagation.png (20.14 KiB) Viewed 1813 times
with a code like this:

Code: Select all

                catch (const Base::Exception& e) {
                    Base::Console().Error("%s\n", e.what());
                    QMessageBox::warning(Gui::getMainWindow(), QObject::tr("Error"),
                                         QString::fromUtf8(e.what()));
                    getSelection().clearSelection();
                }
As it is written, it does not have translations either (it is just a test code)

I have no clue which one is the right way of propagating these messages from sketchobject to the toolbar commands. I am not even sure if what I show above is what is should be. Any help is appreciated.
Post Reply