Sketch: how to handle reversed external arcs?

[DeepSOIC]

Hi! I've encountered a problem with my branch (AngleViaPoint constraint) and I have a number of ways to fix it.
The problem arises from the thing that when creating a link to external geometry, sometimes arcs of circles are imported in reverse. The reversed condition is characterized by:
OCC side:
* arc's Axis.Z is negative
* arc's relation between x,y and parameter is vertically mirrored, resulting in arc being drawn clockwise (the normal way is CCW)
Sketcher side:
* the angle values stored there do not correspond to start point/end point coordinates. This doesn't cause any trouble so far, since they are barely used.
* the net effect is that sketcher code thinks that the arc is the other part of the circle. This does not affect most of stuff, but it does matter for point-to-point tangency, which is reversed as a result.

At the moment, I need to fix the tangency, but I think it is a good reason to introduce proper handling of the case.
Porposal 1.
Introduce a new field to all arcs, called "reversed".
- requires potentially a lot of code to account for the property, increases overall complexity
- the property is a burden for regular geometry, which is always CCW so far.
+ does not break existing sketches
+ makes all sketcher elements directional (currently, only lines are), which may become interesting in certain cases.
Proposal 2.
Just swap start and end points when converting from Part::GeomArcOfCircle to GCS::Arc. Convert angle values, so that the arc is the proper CCW arc.
+ small code changes
- breaks existing sketches if those use endpoints of reversed arcs. I feel this would be ~30-50% of sketches with links to arcs.
+ the sketcher code becomes a bit less dependent on OCC (it would not matter if OCC suddenly decides to swap the direction of arc)
Proposal 3.
Just fix the tangency, and leave the messy arcs intact. Revert back to Sketch-type object creation in calculateAngleViaPoint.
+ small code changes
- seriously slower calculateAngleViaPoint (which doesn't really matter, because it is never called in a loop, and is essentially constant in execution time, and the time is not noticeable. It may matter for scripting though.)
+ does not break existing sketches
- leaves a mess in the external arcs, that may be causing problems I'm not aware of, or cause future bugs.

So, Proposal 2 has a net +1, but breaking existing sketches is a big deal. But links to external geometry are really unstable now anyway, so probably no one will notice

Guys! I want your opinions! This is a hard decision for me.

[DeepSOIC]

And a quick note where the problem appears right now.
Steps:
1. Create sketch. Draw a slot. Close.
2. Pad the sketch.
3. Rotate the view to see the side of the pad where the sketch was, and map a new sketch to the face.
4. Create link to external geometry - to an arc.
Draw another arc, and try to apply tangent constraint by selecting two endpoints (point-to-point tangency). It is likely you won't be able to achieve smooth tangency - you'll get "sharp" tangency instead.
If you use angle-via-point branch, the tangency created will be of a wrong type (sharp instead of smooth, and vice versa).

[abdullah]

Edit: removed this [If I were to program it without backward compatibility I would force all arcs to be CCW. But that is not the case here.]

If I understand you correctly, this only happens: when using external geometry. Would it be an option to add "extra code" to the import external geo command, so that the direction of the arc is checked and if in -z, change it on creation of the sketch arc?

[DeepSOIC]

If I understand you correctly, this only happens: when using external geometry.

yes

Would it be an option to add "extra code" to the import external geo command, so that the direction of the arc is checked and if in -z, change it on creation of the sketch arc?

It is the Proposal 2.

[abdullah]

Would it be an option to add "extra code" to the import external geo command, so that the direction of the arc is checked and if in -z, change it on creation of the sketch arc?

It is the Proposal 2.

Well, I am referring to actually changing the OCC arc's direction when asking to become an external element, not just the GCS. I do not know the code by heart (and I do not have a PC handy), but one option could be sketch enters edit mode, the occ wrappers are checked to see if the arc is CW or CCW in the sketch direction, converts to CCW where needed. If a convertion was needed (there was an external element preexisting), for backwards compat, the start and endpoints are also exchanged in the constraints. Any newly introduced external geometry is converted to CCW directly...

Anythung sounds wrong?

EDIT: if the conversion can not be made permanent (I do not remember how this external geo works code wise), then maybe it is possible to convert to CCW prior to calculation in occ and undo afterwards, or go with your proposal 2, but with the patch of exchanging endpoints in GCS constraints... sorry I can not be of more help from my mobile...

[DeepSOIC]

Well, I am referring to actually changing the OCC arc's direction when asking to become an external element, not just the GCS. I do not know the code by heart (and I do not have a PC handy), but one option could be sketch enters edit mode, the occ wrappers are checked to see if the arc is CW or CCW in the sketch direction, converts to CCW where needed. If a convertion was needed (there was an external element preexisting), for backwards compat, the start and endpoints are also exchanged in the constraints. Any newly introduced external geometry is converted to CCW directly...Anythung sounds wrong?

I'm afraid there's everything wrong with it. First, I feel that it may be impossible to change due to some const-ness, and if it is... reversing it may even break the shape it is part of (I heard there was an option to reverse a line to make a twisted ruled surface... somewhere...). and the third is - imagine what happens on recompute. The arc gets regenerated, thus becomes reversed again. reversing it back will trigger another swapping in constraints... that becomes a hell.
Feel free to correct me if I'm wrong. I essentially have zero opencascade knowledge, and very little knowledge of freecad's overall architecture.

go with your proposal 2, but with the patch of exchanging endpoints in GCS constraints...

I'll think on it. The first impression is that it will be a challenge to keep it from swapping the constraints over and over. It feels like it is going to morph into an equivalent of a "reversed" property, thus negating the last positive point of the proposal (that it would not matter if OCC suddenly decides to swap the direction of arc) and essentially becoming equivalent to proposal1.

[abdullah]

Have you tried to make a normal, non-external arc "reversed" by moving its end points? I ask this to ensure that the extent of the problem is only external geo...

The thing is that I do not know how the external geo is generated for the sketch. Certainly I am not proposing to change the edge, but the arc shown in the sketch... though this might not be possible... I will try to take a look tomorrow...

If transparent handling following option 2 is not feasible, then option 1 might be a way to go. This is a GCS property afaiu, so solver related. The check may be restricted to external geo only...

However, if it is feasible to work 100% with CCW geo, I think thus is the way to go...

[DeepSOIC]

I think I have a good idea. It involves a lot of work, but.
1. We go for Proposal 2.
2. We create a special Sketch Porter procedure that is run upon file loading. We introduce Sketcher version numbering and write it down to FCStd files. Now, whenever we see a file is loading that has a not-current version number, the porting procedure is run (one or more, depending on the difference in versions).
Once it is done, making breaking changes to the sketcher becomes piece-o-cake!

[abdullah]

@DeepSOIC

I am generally not scared of "a lot of work". Although the version checking proposal may be a solution for this and other problems, I am of the preliminary opinion that it may be overkilling for this specific problem.

It is quite a nightmare to navigate FC's code with a mobile device, but what would you think of adapting the function named "rebuildExternalGeometry" in sketchobject.cpp, so that when the Geom arcs of "circle/ellipse/..." are created from the external edge, these are created as CCW??

For my device it would be a nightmare to check if in all the situations (load from file, add a new external geo) this function is executed. I think it does, if not it may still be part of a bigger fix...

[abdullah]

@DeepSOIC

Would you change for me one external arc giving the problem to an arc of ellipse and check what happens? I tell you this because in the function of my last post there seems to be some extra code dealing with directions for aoe.

I can not remember everything I did for ellipse...