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: 20242
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sketcher: Bezier curves

Post by wmayer »

Arguments:
It is still not absolutely clear what you mean by consistent. However, it may not be important as you have detailed two implementations and I understand that.
In C++ I can e.g. do this:

Code: Select all

GeomPoint* pnt = new GeomPoint();
pnt->setPoint(x, y, z,);

// create a copy
GeomPoint* cpy = new GeomPoint();
cpy->setHandle(Handle_Geom_CartesianPoint::DownCast(pnt->handle()));
...
Here I have two points with different tag ids.
I do not favour the second implementation (createNewTagId), if only, because it implies (constructor assigning a new tagid + clone assigning a tag id + createNewTagId assigning a new tagid), unless there is no other way.
Now look at this:

Code: Select all

void function(Geometry* geom)
{
    Geometry* copy = geom->clone();
    ...
}
Now I have a copy which has the same tag id. If I want the copy to have a different tag id there must be a way to achieve this. So, this means either clone() creates a new tag or we add a method (createNewTagId) to do so afterwards.
The thing is that before my changes the clone function was producing a clone, if we implement our solution 1, clone will not produce a clone, if only, because the tag is different. I think this is confusing and should be avoided.
I disagree. The clone is an independent copy which you can modify afterwards. So, with clone() using the same tag id we end up with two different geometries that have the same tag id.

Unless I missed something then the use cases where the clone must have the same tag id as the original is only when either the clone or the original is only hold temporarily. To make this clearer I have exposed the getTag to Python:

Code: Select all

doc=App.newDocument()
sketch=doc.addObject("Sketcher::SketchObject","Sketch")
c=Part.Circle()
n=c.copy() # uses clone() of the C++ class!
n.Center.x=5.0

sketch.addGeometry(c)
sketch.addGeometry(n)

sketch.Geometry[0].Tag
sketch.Geometry[1].Tag # this is the same tag id but a different geometry!
I was referring to clone and copy as concepts more than member functions.
But this is on a much higher level and only dedicated to the sketcher. I am talking about the lower level API which in itself should be consistent.
In the sketcher I need copies (different tagid) and clones (same tagid).
As said above the clone with the same tag id usually only happens where either the clone or the original will be deleted very soon.
Since you can create a copy or a clone with the same API function the question to me is what is easier to do:
* create a clone with the same tag id and create a new one afterwards
* create a copy with a different tag id and support a way that it has the same tag id
how do I handle now that tag? It should be uniform. Already as part of Carbon Copy, I have a separate commit with a copy() member in Geometry, like clone() but without assigning the tag. This is used in carbon copy, but will be used for example in array, symmetric element.
I haven't checked your code. Just in case here you don't have to make copy virtual and reimplement it for all sub-classes. It suffices to have a non-virtual method like:

Code: Select all

Geometry* Geometry::copy()
{
   Geometry* cpy = clone();
   cpy->tag = ...
   return cpy;
}
But there is still the problem with the above example that you can change the geometry of the clone so that you have two different geometries with the same tag id. To avoid this the clone() actually must return a const Geometry*
Having two separate well-documented copy methods, clone() and copy(), and in addition your "assigntagid (geometry *) for the sethandle and handle constructors".
In this case the assigntagid is not needed any more. The programmer then has to just use the correct method.
Btw, after merging your branch I have added the method createNewTag. If we come to the conclusion we don't need it for client programmers it suffices to make it protected.
Question: Do you have anything against me adding the commits of carbon copy on this pull request? That it will avoid that I have to make adaptations to both branches that will finally cancel out, as I have geometry changes in both.
Before continuing can you rebase it on latest master please?
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

wmayer wrote:Before continuing can you rebase it on latest master please?
I never realised it was already merged until now. Otherwise I would not have asked.

Yesterday I was fixing outstanding issues. I think I have everything but the comb issue. Today I have rebased that work on master.

I acknowledge I have fully understood your explanation on the geometry API. I will think and work on this today.

Thanks for bearing with me. :)
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

abdullah wrote:I haven't checked your code. Just in case here you don't have to make copy virtual and reimplement it for all sub-classes. It suffices to have a non-virtual method
Good advice! :)

I have done a pull-request:
https://github.com/FreeCAD/FreeCAD/pull/682

I have temporarily put into protected the CreateTag() and an assignTag() that I wrote yesterday, just in case we feel the need later on. Here it boils down to what we want to allow. ATM I am fine with none, but I do not care if they are both exposed or only one of them. I do not feel the need ATM.

I have created a Geometry* copy() function. This function does not assign a tagid. This function is virtual and implemented in each derivate.
I have changed virtual Geometry * clone() to be non-virtual and call copy() and assign the tag in the parent class.
Python uses now c++ copy in the python copy function.

With this I think most of the issues.

About the protected functions

In principle none should be needed, as the user always (I think) has the opportunity to choose to create a clone or a copy and choose what he wants.

Here, we might need to expose clone() to python. I have not done it because I do not know if you agree. If you want me to do it, just tell me.
wmayer wrote:But there is still the problem with the above example that you can change the geometry of the clone so that you have two different geometries with the same tag id. To avoid this the clone() actually must return a const Geometry*
I am not sure I want to do this. I may want to have two different geometries with the same tag id ( would not this be the case of having two evolutions of the same shape, one in the current state and an old version in the undo pipe?
wmayer wrote:In this case the assigntagid is not needed any more. The programmer then has to just use the correct method.
Btw, after merging your branch I have added the method createNewTag. If we come to the conclusion we don't need it for client programmers it suffices to make it protected.
I mean, I can think that somebody may create a Geometry in one function from a handle, and then want to add another tag id in another function. And also the need to detach one Geometry of its previously used id. But as I said before, I have not met the need yet.

Let me know if the changes are not acceptable for you.
wmayer
Founder
Posts: 20242
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sketcher: Bezier curves

Post by wmayer »

I am not sure I want to do this. I may want to have two different geometries with the same tag id ( would not this be the case of having two evolutions of the same shape, one in the current state and an old version in the undo pipe?
Yes, but in this case the two geometries are in two different instances of the PropertyGeometryList. But the code example above shows that two different geometries with the same tag id are listed in the same instance of the PropertyGeometryList. And this is something I guess shouldn't be allowed, no?

One way to avoid such situations could be to use the copy method inside addGeometry.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

wmayer wrote:Yes, but in this case the two geometries are in two different instances of the PropertyGeometryList. But the code example above shows that two different geometries with the same tag id are listed in the same instance of the PropertyGeometryList. And this is something I guess shouldn't be allowed, no?
Having two equal tag ids in the same PropertyGeometryList is something that shall not be allowed. That is the whole point of adding the tag. To differentiate between them. No doubts there.

What I am not sure of is whether that pointer shall be made const. I think I need to be able to have a non-const pointer as a result of a clone. Ok, let's try out of fun:

Code: Select all

    const Geometry *clone(void) const;
First compilation error that clone() line down there, which I fix by adding const (below the const is already added).

Code: Select all

void PropertyGeometryList::setValue(const Geometry* lValue)
{
    if (lValue) {
        aboutToSetValue();
        const Geometry* newVal = lValue->clone();
        for (unsigned int i = 0; i < _lValueList.size(); i++)
            delete _lValueList[i];
        _lValueList.resize(1);
        _lValueList[0] = newVal;
        hasSetValue();
    }
}
Second compilation error, the "_lValueList[0] = newVal;"

error: invalid conversion from ‘const Part::Geometry*’ to ‘__gnu_cxx::__alloc_traits<std::allocator<Part::Geometry*> >::value_type {aka Part::Geometry*}’ [-fpermissive]

I would say this one is because std::vector requires that the template object can be copied at the assigning.
wmayer wrote:One way to avoid such situations could be to use the copy method inside addGeometry.
You can always bypass it via addGeometry(const std::vector<Part::Geometry *> &geoList, bool construction/*=false*/), where no clone is used, but it would add an extra security for/from the sketcher python user.

Note: I have just added it to the pull request.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

wmayer wrote:
The curvature comb where two adjacent curves are made C2-continuous is fixed whereas moving a single poles still affect the curvature comb of the whole curve.
I have identified the problem. It is the general comb scaling factor combrepscale, which is calculated as the maximum of the bsplines, each bspline calculated as:

Code: Select all

double temprepscale = ( 0.5 * maxdisttocenterofmass ) / maxcurv; // just a factor to make a comb reasonably visible
if I substitute this with lets say 50, there are not issues at all. I keep investigating why.

Alternatively, it would be great to have a constant representation reference related to the size (in mm) of the viewport or scene, for example of the Y dimension of the scene. If I could know that the Scene has for example 100 mm in vertical, I could set a fraction of this (lets say a 10%) to the combrepscale. I have tried this route, however I am yet to find the right method. I am somewhere:

Code: Select all

           Gui::MDIView *mdi = this->getActiveView();
           Gui::View3DInventor *view = qobject_cast<Gui::View3DInventor*>(mdi);
           Gui::View3DInventorViewer* viewer = view->getViewer();
           SoGetBoundingBoxAction action(viewer->getSoRenderManager()->getViewportRegion());
           
           SbBox3f box = action.getBoundingBox();
 
           /*const SbViewportRegion& vp = viewer->getSoRenderManager()->getSize();

           SbVec2f siz = vp.getViewportSize();
           float dX, dY; siz.getValue(dX, dY);*/
           
           float dX, dY, dZ; box.getSize(dX,dY,dZ);

           double temprepscale = std::max(dX,dY)/10;
I managed to get the size in pixels and in points, but not yet in mm. :(

Edit: Probably using only a constant of the representation window will not work nicely either. I managed to get the boundingbox of the root scenography (which is not the screen as the scenography can be bigger or shorter) and still, when multiplying by the curvature, depending on the comb goes out of the window.

Code: Select all

           Gui::MDIView *mdi = this->getActiveView();
           Gui::View3DInventor *view = qobject_cast<Gui::View3DInventor*>(mdi);
           Gui::View3DInventorViewer* viewer = view->getViewer();
           SoGetBoundingBoxAction action(viewer->getSoRenderManager()->getViewportRegion());
           action.apply(viewer->getSceneGraph());
           
           SbBox3f box = action.getBoundingBox();
           
           /*const SbViewportRegion& vp = viewer->getSoRenderManager()->getSize();

           SbVec2f siz = vp.getViewportSize();
           float dX, dY; siz.getValue(dX, dY);*/
           
           float dX, dY, dZ; box.getSize(dX,dY,dZ);

           double temprepscale = std::max(dX,dY)/10; // just a factor to make a comb reasonably visible
            
            if( temprepscale > combrepscale )
                combrepscale = temprepscale;
It seems there is a need to compensate for the curvature with something like maxcurv above, but with some kind of hysteresis so that the representation factor does not get updated on every mouse move... I have to think deeper...
wmayer
Founder
Posts: 20242
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sketcher: Bezier curves

Post by wmayer »

What I am not sure of is whether that pointer shall be made const. I think I need to be able to have a non-const pointer as a result of a clone. Ok, let's try out of fun:
That doesn't solve the problem anyway because instead of the cloned geometry you can modify the original geometry. So, forget about this.
You can always bypass it via addGeometry(const std::vector<Part::Geometry *> &geoList, bool construction/*=false*/), where no clone is used, but it would add an extra security for/from the sketcher python user.
This is not quite true. In fact inside the method it doesn't directly create a clone but it creates a new array with old and new geometries and then inside PropertyGeometryList::setValues it creates all the clones.

IMO, the addGeometry(const std::vector<Part::Geometry *> &geoList, bool construction/*=false*/) could be modified this way that for all newly added geometries it creates a copy (it's not tested code but just to demonstrate how it could look like):

Code: Select all

int SketchObject::addGeometry(const std::vector<Part::Geometry *> &geoList, bool construction/*=false*/)
{
    const std::vector< Part::Geometry * > &vals = getInternalGeometry();

    std::vector< Part::Geometry * > newVals(vals);
    std::vector< Part::Geometry * > copies;
    copies.reserve(geoList.size());
    for (std::vector<Part::Geometry *>::const_iterator it = geoList.begin(); it != geoList.end(); ++it) {
        Part::Geometry* copy = (*it)->copy();
        if(construction && copy->getTypeId() != Part::GeomPoint::getClassTypeId()) {
            copy->Construction = construction;
        }
        
        copies.push_back(copy);
    }
    
    newVals.insert(newVals.end(), copies.begin(), copies.end());
    Geometry.setValues(newVals);
    for (std::vector<Part::Geometry *>::iterator it = copies.begin(); it != copies.end(); ++it)
       delete *it;
    Constraints.acceptGeometry(getCompleteGeometry());
    rebuildVertexIndex();
    
    return Geometry.getSize()-1;
}
wmayer
Founder
Posts: 20242
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sketcher: Bezier curves

Post by wmayer »

I have identified the problem. It is the general comb scaling factor combrepscale, which is calculated as the maximum of the bsplines, each bspline calculated as:
The point is that all these numbers are calculated for every move of a pole which then of course give you always a different ratio.
if I substitute this with lets say 50, there are not issues at all. I keep investigating why.
If you use hard-coded numbers then of course the ratio is fix.
Alternatively, it would be great to have a constant representation reference related to the size (in mm) of the viewport or scene, for example of the Y dimension of the scene. If I could know that the Scene has for example 100 mm in vertical, I could set a fraction of this (lets say a 10%) to the combrepscale. I have tried this route, however I am yet to find the right method. I am somewhere:
The downside of this is that the ratio will be independent of the curve and thus when zooming out a lot you will get a curve that is just a few pixels but a huge curvature comb.

Another idea would be to compute the ratio once when you switch on the curvature comb and then use this number inside the draw method.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher: Bezier curves

Post by abdullah »

wmayer wrote: IMO, the addGeometry(const std::vector<Part::Geometry *> &geoList, bool construction/*=false*/) could be modified this way that for all newly added geometries it creates a copy (it's not tested code but just to demonstrate how it could look like):
Not a single mistake. Not a single typo. I added this in the pull request.
wmayer wrote:The downside of this is that the ratio will be independent of the curve and thus when zooming out a lot you will get a curve that is just a few pixels but a huge curvature comb.

Another idea would be to compute the ratio once when you switch on the curvature comb and then use this number inside the draw method.
I implemented in the pull request the hysteresis option, which allows to also update if the representation ratio factor goes over 2 times or 0.5 times the value being used. This solves the problem you mention.

Let me know if you see any issue.
wmayer
Founder
Posts: 20242
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sketcher: Bezier curves

Post by wmayer »

All good now. Thanks for your quick fixes and awesome contributions!
Post Reply