Ok, let's play the naming game, list your preferences (up to 5), the first gets 5 points from each voter, the second 4 usw.
FreeCADPlaneButNot2DButNot3DCommunityFlatSketchGCS (+6): Ok disqualified for giving more than 5 points.
FCGCS (+5): FreeCAD's GCS, so not a library, our GCS of FreeCAD.. Currently there is just only one in Sketcher, no need to differentiate, the other is in a separate module.
FCSketcherGCS (+4): FreeCAD's Sketcher GCS.
SketcherGCS (+3): Sketcher GCS. We do know in what project we are working, don't we?
SketchGCS (+2): Sketch GCS.
FlatGCS (+1): Well it is a flat GCS.
Now to the more serious staff...
DeepSOIC, I have squashed your code to make it into a more handable number of commits. The result is this:
https://github.com/abdullahtahiriyo/Fre ... allsquahed
I am forcing you by no means to accept this squashing. It is just a way for me to facilitate the discussion of changes, where I have questions.
I see that you have improvements in several fronts. I have tried to maintain those separated.
DeepSOIC, this is by no means an intent to critisize your work. I just raise some questions so that others more competent than me can take a look and maybe propose advantageous alternative solutions, or may just tell me: there is nothing to worry about. These are all GCS/solver architecture related issues that. I know you are more interested in other opinions than in hearing me repeat about the same. I hope this helps you get additional input.
A. SOLVER RESPONSITIVITY TWEAKS
=============================
This commit:
https://github.com/abdullahtahiriyo/Fre ... 77d9937713
Two changes:
1. Reduction of the number of iterations
I have seen a couple of posts ago that this is related to your observation that over that limit, it is a hang (tens is single root, 5 of tens is double root, over 100 is hang). I am ok with this. I just put it here in case some has an objection, a difference experience or wants to comment. If it works well with your horn, then I think there is a good chance it works reasonably well overall, at least until we make the solver more accessible to the user (the real user, in the UI).
2. Repaint() instead of update() to force render for every movePoint.
I know you have explained it several times already, I just do not manage to find a full explanation of what is exactly be being repaint and in which situation. How does a user appreciate this?
AngleViaPoint
============
In commit:
https://github.com/abdullahtahiriyo/Fre ... 4b8f6ad285
In file:
src/Mod/Sketcher/App/ConstraintPyImp.cpp
3. You have changed the FirstIndex/FirstPos/SecondIndex/SecondPos naming to Argument1, Argument2,... is there a reason for this? I liked the old fashion naming of FirstIndex/FirstPos, a human would know that a defined order in the parameters exist. Anyway, this is a very minor issue to me.
src/Mod/Sketcher/App/Sketch.h:
4. I raised this point the first time I looked into your code. Now with the comments is easier to understand why this functions are exposed to the UI. I do not like the fact that the UI calls the solver to resolve its issues (get the normal direction of an object, or angle between normals of two objects).
+ //This func is to be used during angle-via-point constraint creation. It calculates
+ //the angle between geoId1,geoId2 at point px,py. The point should be on both curves,
+ //otherwise the result will be systematically off (but smoothly approach the correct
+ //value as the point approaches intersection of curves).
+ double calculateAngleViaPoint(int geoId1, int geoId2, double px, double py );
+
+ //This is to be used for rendering of angle-via-point constraint.
+ Base::Vector3d calculateNormalAtPoint(int geoIdCurve, double px, double py);
I see that there is no an easy solution. I leave it here so that others who may want to look into it, and may come with a more successful solution (than what I am capable of).
P.S.: With the conics I have come across the problem that some times I need in the UI the normal values. I am calculating them there in the code where I need them. OCC provides the parameters D0,D1,D2 for any Curve, see:
http://dev.opencascade.org/doc/refman/h ... curve.html
One possibility that I was thinking of to solve my problem (that maybe solves yours also) would be to expose the methods D0 and D1 of opencascade, so you pass a "parameter of the curve", and you get the point at the curve and the first derivative. You may want to take a look.
5. The variable:
+ bool remapped; //indicates that pvec has changed and saved pointers must be reconstructed (currently used only in AngleViaPoint)
I do like this idea (I do not know how much applicable may be for other constraints, but avoiding this sounds reasonable). I would change the name from remapped to something more descriptive (the thing is that the comment is clear to me the variable name is not).
A similar thing happens to me with:
+void ConstraintAngleViaPoint::ReconstructEverything()
What is everything? When I look a the code I understand it, but not without looking. Of course, those are minor issues.
6. CGS::Vector2D. I see you have your reasons:
DeepSOIC wrote:Regarding separate Vector2D in Geo.h.
I'm reluctant to replacing it with Base::Vector2d because:
* mine is a bit incompatible (no big deal)
* that will make flatGCS to depend on FreeCAD. Currently it's AFAIK independent (except some console dumping stuff) and can be stripped off really easily. This is a big deal.
* my implementation is tiny. I didn't even bother to overload operators, although that can clean up some math code.
I've made a glance at Eigen's vectors and.. well.. they are sooo powerful and look like a huge overkill for the purpose. But using them is OK in terms of encapsulation since Solver needs Eigen anyway.
A reasoned decision always has more weight in the evaluation by others. I do not know what Werner thinks, and he will surely let you know if he does not like it
, if it is ok for him, I would rename it to something else, not to cause confusion to the user (e.g. GCS::Vector).
7. src/Mod/Sketcher/App/freegcs/Geo.cpp
The Copy() Method, like in Line::Copy():
All the geometries live in the sketcher in the arrays of each type of geometry, and its pointers are always valid during the time the solver runs. Why make an object copy and not just assign the pointer? Maybe I am wrong or I am missing something
That is all I wanted to point out that may affect the solver/GCS architecture.
After reviewing the current code today, I see that a lot of concerns I had are gone.In favour of the functionality, I would like to say that:
1. The argument that tangency flipping of endpoint-to-endpoint tangency is fixed by this new constraint is convincing to me and justified the negligible loss of performance with respect to old curve to endpoint code.
2. There is no substitution of code, just a selection of which code to execute that solves the tangent flipping.
3. If this code gets to master, I would be very much happy DeepSOIC, if you could substitute my awful ellipse to ellipse/circle/arc tangent via construction line with your AngleViaPoint tangency via point (using a smart point dropping-in algorithm that inserts a point in the UI out of nowhere
).
Yes, it is a long post. It is not that I do not want to do it shorter, it is that I do not have the capacity