abdullah wrote:1. I hate labels in c++.
Just delete them. I was a bit of hacking into the existing code, it is worth reformatting a bit. I just tried to kill one of the two equal hard-coded messageboxes.
2. I hate reusing fields for a purpose different of what they were designed (and named for). This is the case of: src/Mod/Sketcher/App/ConstraintPyImp.cpp:
The fields of the constraint structure are used as intended. These are the python arguments assigned to C variables with irrelevant names. Otherwise, this requires the whole reformatting of the if(parsetuple(..."siiiO"...)){...} Well, maybe I just didn't find the right place.. Actually I had a temptation to completely rewrite the whole python implementation there, making it to start from getting the constraint name and doing argument parsing afterwards. Feel free to do whatever you want with it.
4. ... Maybe (I am not sure), less code can be repeated by making the last parameter of addAngleViaPointConstraint default to 0.
Not really useful because angle=0 is not just any tangency, it is internal tangency. But yes, it can be done, I just didn't feel the need for it.
6. About "SketchObject.cpp". Why do we need those functions? who calls them? Maybe I am totally off. But at this moment, I do not see it. Help me understand.
This one is important, and yes, 9 is related. For the constraint to work properly, the point must be on both curves. So we need to add necessary point-on-curve constraints without causing redundancy.
Testing if there already is a point-on-object constraint doesn't really solve the problem, because the point might be forced onto object in more complex ways, for example:
* the point is coincident to a point that is on the object (e.g. when selecting an endpoint of the line which was previously made coincident to an endpoint of an ellipse arc)
* the point might be an endpoint of an internal-aligned major diameter, which is on an ellipse but is not a point-on-object
* due to a complex action of a number of other constraints
This is also the reason for a repeated test for point on object1: if the point was on curve by accident, it could have been pulled off object1 when it was being constraining onto object2.
There is also an important decision to expose those autoadded point-on-object constraints. It was done to be able to manually resolve a redundancy in case this smart detection system does it the wrong way. It is also important to make saving/loading of files independent of this smartness.
7.... i. You are basing your implementation in: controlling a remap (via the remapped boolean), so as to "reconstruct" the object, you make copies of the curve objects. Why do you need a copy of the curves? Why do not you use a pointer assignment instead? I may not understand all that you are doing here, so go easy on me
This is also very important. I assumed (but didn't really check) that remap is called from within solver (to account for simplification of parameters vector done during system analysis for example). I don't like the idea of unexpectedly messing with objects that were passed by reference, this is why a copy is needed.
8. A new Vector2D implementation in src/Mod/Sketcher/App/freegcs/Geo.h
Just do it. I only warn you that I have made a method getNormalized which doesn't exist in the available implementations, so some change will be necessary in calculateNormal in ellipse and maybe (I don't remember) in the constraint. (there is a more invasive normalize() method in existing 2d vector, which changes the underlying vector instead of creating a new one)
