hi... nice to meet you...
em... I assume you are the logari81, Konstantinos Poulios, the great developer and creator of CGS and constraints in the current sketcher... nice to meet you!!!
If feel honoured that you drop by in this thread.
logari81 wrote:I see that you have come quite far with supporting ellipses.
We, FreeCAD, as you know, we are a strong community, we manage to keep it going by contributions from different people. We have faced many difficulties, some because we are not proficient in the understanding of the code, others because mainly I miss the math concepts to see it right. We keep going...
logari81 wrote:that's a good habit. I can give you some hints specifically about freegcs:
Of course, feel free to dig into the code and give me as many hints as you deem necessary.
logari81 wrote: AFAIR I avoided starting variables with capital letters and I avoided underscores. So, X_0, X_1 etc. would rather be written x0, x1 etc.
I understand your concerns, as those are not part of any c/c++ coding style I have ever seen. However, there is a good reason for they being so. The C code in the partials is directly generated by sage from the formulas (saves the human error factor in editing long formulas). This has many other advantanges, as for example improving the formula without any editing (improve, paste the whole, compile). Of course, I could change all the sage worksheet, so that it uses x0 instead of X_0, but when one works with formulas, with most languages, sage included, underscode is used to indicate subindex (I think it is LaTeX inherited), and formulas are much better understood with subindexes. One thing I did is to constrain that syntax to the sections where the formulas generated by sage are copied, as a trade-off between maintaining good coding practices/aesthetics and avoiding to have to reedit otherwise correctly generated formulas.
logari81 wrote:- I have noticed an enumaration value named "P2OnEllipse". The numer "2" in such names is read "to", so in this case it doesn't make much sense. The name should rather be "PointOnEllipse"
Ok. This is clearly a typo that has propagated. I will change this. Thanks!!
logari81 wrote:- focus1_X, focus1_Y is not very uniform with the rest notation, I would rather write it focus1.x, focus1.y.
You are absolutely right. I introduced it yesterday. However, I think I can not use focus1.x as name of a variable, but of course I can name it focus1X and focus1Y for consistence...
logari81 wrote:One more substantial observation is about the "ConstraintType" enumaration. One should add values to this enumeration really sparingly. Actually the elements of this enumeration are meant to approximate the minimum number of mathematical primimitives, that the complete set of constraints can be based upon
Sorry. I fail to follow your reasoning. There is no physical limit, AFAIK, for the amount of enums an enum can have (well yes, related to the number of bits of an unsigned int in the given platform, which we are far away from reaching). If what you mean is that, we should reuse primitives as much as possible, avoiding to generate new ones when one can be reused, then I fully agree with you, but I fail to see how I could reuse existing primitives for a rather more complex shape like an ellipse. If you see how I could spare primitives do not hesitate to let me know. I would be more than willing to reduce them.
logari81 wrote:11 such primitives where sufficient for building all previously available constraints. That said, adding another six primitives for supporting ellipses and elliptical arcs seems -in a first look- to be a bit too much.
Well, no offense, but I think this is mainly due to the comparable simplicity of the preexisting element types in comparison with the ellipse. Being able to make a line tangent to a circle by just saying put a line at this distance from the center point of a circle was a piece of cake compared with making a line tangent to the ellipse. This, of course, can be that due to my limited knowledge of geometry I have failed to see further. Really if you can help us here, man! you are more than welcome.
I would try to explain the reasons behind each, if I can recall them...The additional values in the enum are:
P2OnEllipse = 13 => The only way we have come to is to make an specific solver constraint. I do not see how I could do it with existing constraints.
TangentEllipseLine = 14 => same as for the previous one applies.
Point2EllipseDistance = 15 => Hey!!! Thanks man, this was simply not used, part of previous attempts (just removed it)
EqualMajorAxesEllipse = 17 => This could be avoided by selecting other parametrization of the ellipse, where the length of the major axis is a parameter, but would have generated the need of another one, like the minor, or would have brought stability problems (like when we defined it with center, major, minor and phi, where phi is the inclination of the major axis).
EllipticalArcRangeToEndPoints = 18 => the parametrization in an ellipse is with t, the eccentric anomally, I have found no way of doing this with normal constraints. However, it could be removed if we consider DeepSOIC's proposal. I would really ask you for more input with this. I will very much welcome your opinion on why to go in one or other direction, or why not to go in one or another direction.
InternalAlignmentPoint2Ellipse = 16 => This is really the solution we found, so that the UI is not bloated with tons of shape specific constraints (usually needed for complex forms).
As said. I would very much welcome any further input.
logari81 wrote:Some other observations:
- I have seen that in a recent patch you compare radii to decide between major and minor radius. From my experience this kind of checks are not robust and indicate that there could be some alternative and more rigorous way to formulate your equations.
If I may, with all due respect, I think you might have misunderstood the reason of the comparison.
It is not to decide between major and minor in the sense of applying one or the other. It is to decide which one to assign first to the GeomEllipse provided by OCC, so that, once the solver got to the right values for the axes a,b; these are assigned to the major and minor parameters of GeomEllipse in such a way (i.e. in such an order), that in the moment after assigning the first one and before assigning the second one, OCC's GeomEllipse always has a major length higher or equal to the minor length. If that is not the case, GeomEllipse will throw an exception. So, it is not a problem that the solver wants to assign to the ellipse an a<b, it is a problem of the temporal values stored in GeomEllipse, and OCC's limitation that major must be >= minor.
logari81 wrote:- I haven't really investigated that very much but from my intuition a nice way for parametrizing the ellipse would be its center (2 dofs), one point at its intersection with one of its axes (2dofs) and the radius along the other axis (1dof). I do not differentiate which axis is major and which is minor, it shouldn't matter.
I really have to give it a thought. I may even do test implementation... Thanks for the tip!!
logari81 wrote:- I don't know if compilers have specific optimizations for the pow function, with exponent equal to 2, but one would normally avoid using pow for squares.
A modern compiler should optimize it as shown in:
http://stackoverflow.com/questions/6321 ... h-x-double
both produce the same assembly according to the most voted answer.
My reason to have pow instead of a simpler multiplication is related to the fact that sage directly generates the code corresponding to the formula for me.
Thank you very much for dropping by. Do not hesitate to drop some more lines if you have the time and energy to do so... I am only happy that after seing what I have done with your files your heart is still beating