Solver/CGS architecture changes: Request for comment

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!
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Solver/CGS architecture changes: Request for comment

Post by abdullah »

DevJohan wrote:I agree with what abdullah wrote about getting everything together, my code is somewhat on the fringe. I am currently pulling his code into my branch. The amount of conflicts are negligible and easily solved but the amount of new code is quite impressive.

From what I have seen so far I get the feeling there is a fair amount of cleanup and sorting out to be made in the freegcs code. There are some references in the code to thread page on this forum which I think should instead be references to a wiki-page or more specific post where the error functions and partials are explained.

I think flatGCS is a good name, it gets my vote. (do I get a vote :D)
Grwat idea!! I do not know if it is possible to accomodate a wiki for solver architecture documentation. It would be great. The partials developed for Ellipse are discussed it the thread and the full formulation is made in sage, the sage worksheets are in the attachments of the thread posts. I am in favour of a well documented gcs, because understanding it is not straightforward from the code, and understanding it is key for improving it. Discussions in a thread would benefit from it.

FlatGCS is ok for me.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: Solver/CGS architecture changes: Request for comment

Post by DeepSOIC »

So my actions now:
1. rebase my code on Abdullah's Ellipse_final_tested_rebased branch
2. finish my changes (add tangency lockdown, add perpendicularity, probably remove Vector2D from Geo.h(I want to discuss that thing))
3. (is it needed?) reorganize my commits
4. make a pull request to Abdullah/Werner.
As for 1 and 3, I have no experience doing it, so it will take time, and maybe I'll have to ask for help.

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.
jmaustpc
Veteran
Posts: 11207
Joined: Tue Jul 26, 2011 6:28 am
Location: Australia

Re: Solver/CGS architecture changes: Request for comment

Post by jmaustpc »

abdullah wrote:FlatGCS is ok for me.
how about 2dGCS? "Flat" is a bit of a common word, a more mathematical type word like Plane or 2d would be my preference. But of course it doesn't really matter much.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: Solver/CGS architecture changes: Request for comment

Post by DeepSOIC »

2dGCS starts with a digit, is not pronounceable, and 2d is not necessarily on a planar surface (which is a very interesting challenge to implement BTW). PlaneGCS is probably better, but with no objective reason I prefer "flat" :D .
User avatar
DevJohan
Posts: 41
Joined: Sun Jul 13, 2014 2:36 pm
Location: Stockholm, Sweden

Re: Solver/CGS architecture changes: Request for comment

Post by DevJohan »

SketchGCS would be an alternative which is more specific that flatgcs.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: Solver/CGS architecture changes: Request for comment

Post by DeepSOIC »

OK, first attempt to rebase made me buried under a heap of conflicts :x FAIL
EDIT: Another attempt is much better, not everything is in conflict now. :roll: phew..
EDIT2: Done rebasing! :D
EDIT3: Now, github's simple GUI app won't push my rebased branch, so that everyone can see/use the result. Ugh.
EDIT4: Fixed! The branch is EllipseAngleViaPoint_rebase :mrgreen:
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Solver/CGS architecture changes: Request for comment

Post by abdullah »

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 :lol:
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: Solver/CGS architecture changes: Request for comment

Post by DeepSOIC »

abdullah wrote:1. Reduction of the number of iterations
Note that I have it done here only for 2-system BFGS (aka SQP?), which is used when moving points by mouse. The hangs also often happen in dogleg (as DevJohan mentioned, when adding a wrong constraint).

Not really often, but sometimes, it does happen that the solver solves in more than 100 iterations. But the solution it gives in such cases is often bogus anyway... like all the sketch is warped inside out... with the constraints fulfilled of course ;) .
abdullah wrote:2. Repaint() instead of update() to force render for every movePoint.
I feel like I want write a book on this :twisted: .
Most is explained in Qt documentation: update, repaint.
update() postpones the actual redraw to happen as soon as FreeCAD processes all pending messages. Effectively, when mousemoves are arriving faster than FreeCAD can consume them, the view is not redrawn until the user stops moving the mouse. Repaint() triggers a redraw immediately (and doesn't return until it has finished AFAIK). Update() is to be used in response to any change that requires updating of the window. Repaint() is intended for animation (and when dragging stuff with mouse - it is an animation).
I am experiencing this behavior all over FreeCAD. The most frustration for me is Draft wb, because I don't see if it snaps properly. The problem is coupled with the fact I use a digitizer pen instead of a mouse. Even when I hold the pen steady, the pointer moves chaotically due to digitizer noise, and it can take a whole second before FreeCAD finds a chance to redraw.
All that makes me wonder - am I the only one who has such a problem? I can make a video if needed.

Unfortunately, changing what I've changed here to repaint solves the problem for sketcher only. I have not yet done the investigation in Draft, but I've seen a few people asking for how to redraw an animation from python, and there is a way.
abdullah wrote: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.
I have done this because the meaning of the arguments differs depending on the constraint. Do you remember that comment in the code: "Let's goof up some terminology"? ;)
abdullah wrote:OCC provides the parameters D0,D1,D2 for any Curve, see:
That may very well replace it. I've also seen (while doing investigation on splines in Loft) that there's a OCC function to compute the parameter value of the point of a curve closest to given coordinates, which would nail it once and for all.

As an alternative, I have an interesting idea to use the solver to precalculate a datum value of an arbitrary constraint, without actually knowing what the constraint is. But, I'd like to leave the existing stuff for now. This may need a lot of work.
abdullah wrote: I would rename it to something else, not to cause confusion to the user (e.g. GCS::Vector).
Std::vector will then be a confusion. At least, both Vector2d's represent the same thing.
You probably remember that the decision was not reasoned when it was made. I invented these reasons later.
abdullah wrote:+ 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()
Yeah, not the best naming here. Maybe doing this directly upon redirectParams/reconstru
ctParams is better, but that means they have to be made virtual and be reimplemented by a constraint.... I find that just as bad as with the bool variable, and I can't see a better solution straight away.
abdullah wrote: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
I have explained that already... I want the constraint to never ever modify the object passed to it.
Looks like redirectParams is never actually called from anywhere, and may be deleted together with all this bool and the relevant mess in Geo.cpp. Doh!.
abdullah wrote: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.
It's not done yet, I was asking for if it's OK to use datum field for the purpose and got a positive answer, but I haven't done it yet.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: Solver/CGS architecture changes: Request for comment

Post by DeepSOIC »

A bit more on update/repaint
OS: Windows
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.15.4161 (Git)
Branch: EllipseAngleViaPoint
Hash: b91eed876bff95502f52c5b9e4eaaa2a2edc65df
Python version: 2.7.5
Qt version: 4.8.6
Coin version: 4.0.0a
OCC version: 6.7.0
I typically run my notebook in power save mode, which is only 700MHz processor speed (doesn't this sound like ancient? ;) ). But it's still surprisingly quicker in most of stuff than my (relatively) beasty development PC. The difference is likely due to an SSD.
With FreeCAD, switching to high performance helps quite a lot (it probably becomes fast enough to process mousemoves faster than they are generated by digitizer; I mostly mean Draft tools here).
My development PC has a regular mouse, and more processing power, so the difference is not as pronounced. But it becomes even worse than on the notebook when I use a debug build. It is slow as a dead cow.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: Solver/CGS architecture changes: Request for comment

Post by DeepSOIC »

Another comment on slot commit: it is already in master. I just don't know how to get rid of it.
Post Reply