tomate44 / CurvesWB

Experiments for a FreeCAD Surface workbench
GNU Lesser General Public License v2.1
114 stars 24 forks source link

Bug in EdgeOnFace.valueAtPoint() which breaks BlendSurface/BlendSolid for circular edges #94

Closed janjan closed 12 months ago

janjan commented 1 year ago

System info

I am using FreeCAD 0.21.1 and Curves Workbench 91dd891840087f1ee45898ce3c9a391c288abd31 (the latest commit as the time of writing this issue)

Bug Description

I just tried to design a new duct for my 3D printer with this fantastic workbench but I ran into an issue where the BlendSurface and BlendSolid commands do not work in all cases if one of the edge is circular.

Luckily I could reduce the problem to a very simple model:

image

error2.FCStd.zip

As you can see the red surface just does not look right. To reproduce the problem just download the zip file above, remove the .zip extension from the file name, open the file, delete the red surface, and then recreate it with the BlendSurface command

If you play around a bit more you will see that the other 3 edge pairs work perfectly fine, only the right one is broken

Debugging

I decided to dig deeper into it and added some visualization to figure out which points are used to create the surface by adding some stuff to blendcurve_at in blendcurve.py:

    def blendcurve_at(self, par):
        e1, e2 = self.rails

        vecs1 = self.edge1.valueAtPoint(e1.value(par))._vectors
        vecs2 = self.edge2.valueAtPoint(e2.value(par))._vectors

        point = FreeCAD.ActiveDocument.addObject("PartDesign::Point", "start_point_0000")
        pl = FreeCAD.Placement(vecs1[0], FreeCAD.Rotation(0,0,0))
        point.Placement = pl
        point.ViewObject.ShapeColor = (0.0, 1.0, 0.0, 0.6)

        point = FreeCAD.ActiveDocument.addObject("PartDesign::Point", "end_point_0000")
        pl = FreeCAD.Placement(vecs2[0], FreeCAD.Rotation(0,0,0))
        point.Placement = pl
        point.ViewObject.ShapeColor = (0.0, 0.0, 1.0, 0.6)

        return BlendCurve(self.edge1.valueAtPoint(e1.value(par)), self.edge2.valueAtPoint(e2.value(par)))

This is how it looks on a working pair of edges:

image

And this is how it looks like on a broken pair of edges:

image

As you can see it uses completely wrong points to calculate the surface

Possible fix

After this first result I did some more digging and I think I might have found the culprit:

This is the function in valueAtPoint in class EdgeOnFace in blend_curve.py

    def valueAtPoint(self, pt):
        "Returns PointOnEdge object at given point"
        if isinstance(pt, FreeCAD.Vector):
            par = self._edge.Curve.parameter(pt)
            if par < self._edge.FirstParameter and self._edge.Curve.isClosed():
                par += self._edge.LastParameter - self._edge.FirstParameter
            return self.value(abs_par=par)

Since the circular edges from my example are closed curves the code sometimes changes the value of par in a way that is not compatible with circular edges

I fixed it in my local branch with this this code which just exclused circular edges from that calculation

            if par < self._edge.FirstParameter and self._edge.Curve.isClosed():
                if self._edge.Curve.TypeId == 'Part::GeomCircle':
                    # circles don't need a special treatment because the parameter is periodical by 2*pi and it automagically works
                    pass
                else:
                    par += self._edge.LastParameter - self._edge.FirstParameter

After reading some more code I came up with this solution which might be better because it is more general:

            if par < self._edge.FirstParameter and self._edge.Curve.isClosed():
                if self._edge.Curve.isPeriodic():
                    pass
                else:
                    par += self._edge.LastParameter - self._edge.FirstParameter

It seems that if a curve is periodic we don't have to calculate that offset

After applying any of my fixes the model looks good:

image

Since I have almost zero knowledge of the FreeCAD code I have no idea if these are actually good fixes or if they might break other stuff. But at least I hope it gives you some ideas on how to find the real problem if my proposed fixes are not sufficient

janjan commented 1 year ago

I just looked at the other issues again and I assume that this is the same issue as in #77

tomate44 commented 12 months ago

Thank you very much for your investigation. I have pushed your fix in new version 0.6.15