Sketcher: Ellipse support

[jmaustpc]

If axis-switching for both methods is the consensus, I'd be happy to tweak the code to do that. I'm actually leaning that way a bit now.

It would make the two construction methods more consistent, if they can both axis switch.

[marktaff]

Since the constraints system is undocumented, and a bit of 'black magic', I thought I'd poke around in Constraints.cpp to begin to understand the system. I started in ConstraintPointOnEllipse::error() with a simple sketch with two ellipses to which I applied the 'Point on object' constraint. By value-printing the err variable, I noticed that err approached zero over successive calls until err was 'small' enough, at which point it started over and repeated the algorithm with the exact same input data!

It turns out that we are solving the sketch twice with the same input data, i.e. hitting Sketcher::Sketch::solve() in Sketch.cpp.
The first call comes from SketcherGui::ViewProviderSketch::solveSketch() in ViewProviderSketch.cpp.
The second call comes from Sketcher::SketchObject::solve() in SketchObject.cpp

The calls are of the form:
if (edit->ActSketch.solve() == 0)
if (sketch.solve() != 0)

So perhaps we need a sketch property like .isSolved to prevent solving the same system twice.

This issue is compounded when you delete multiple items from the sketch, such as one of the ellipses complete with internal geometry. If we are deleting 10 items, there is no need to delete one item, then solve the sketch (twice!), then delete the next item, then solve the sketch (twice!), and so on. We should just delete all the items, then solve the sketch once.

Also, if we delete an ellipse, shouldn't the internal geometry also be deleted?

Initial indications are that this redundant solving happens every time the sketch changes.

When we construct an ellipse (9 items total: ellipse, lines, points, constraints), we hit Sketcher::Sketch::solve() 28 times. I haven't investigated that bit yet, but I do suspect that there are many wasted calls there as well.

[jmaustpc]

Also, if we delete an ellipse, shouldn't the internal geometry also be deleted?

Hi Mark
just a quick comment on this bit...I am wondering if there are use cases where the internal geometry would be an edge from an other part of the sketch or an external geometry...hence would not want to be deleted...

But as far as I know, the rest sounds like something that needs fixing.

Jim

[jmaustpc]

I put this together as an example of using an external geometry line as an internal geometry (using Abdullah's "hidden in the menu only internal geometry" constraint)...no doubt there are other ways to do this,

The line is green in this screen shot.

ellipseoncube.jpg (20.75 KiB) Viewed 1390 times

here is the FreeCAD file

ellipseinternalgeometry_externalgeometryedge.fcstd

(13.14 KiB) Downloaded 22 times

[DeepSOIC]

I am wondering if there are use cases where the internal geometry would be an edge from an other part of the sketch or an external geometry...

Easy. If the element is not constrained to anything other than the ellipse, it's a safe bet it's not needed. It is already implemented btw: if you hit the restore/hide internal geometry button, when all geometry is already there, it is smart deleted.

[abdullah]

Hi folks!!
I am proud of you for so many bug reports and coding in my absence. It is great to be part of this!!!

Lets do a summary!!!

OK. Compiled, run and immediately something popped out. some expected, some not.
1. Mark's creation method still limits itself to a circle when I try to draw b>a. Such a circular ellipse with all internal geometry often becomes locked and confuses the solver.

Please check if this gets corrected in the last version that integrates Mark's patch.

While debugging, I noticed that the constraint solver seems to use only one core (I have eight available).

We need to do a major review of the solver and for sure make it multi-thread. However, I am not multitask myself If I remember well DevJohan was also looking into it. I would suggest that discussion of solver will be centralised in that thread. If actual redesign starts when I am "free" again, I am willing to be part of it. But do not wait for me, if you are eager just attack!!

Here is the extra icon for Mark's alternate ellipse construction method.

I have also updates the standard ellipse construction icon, the improvement is subtle.

Thanks Jim!! Integrated in the lastest push...

I have been testing, I'll post more later, but quickly here are some observations. I have been creating several new ellipses using auto constraint coincident with all of the various points containing in a first ellipse with its internal geometry turned on.
1) your creation method which we first developed and refined mostly works quite well, The auto constraints coincidence works properly on centre point to "what ever point I hover over" of another ellipse, Mark's alternative does not.
2) Mark's construction method auto constrains to the centre of the to be constructed ellipse even though it is operating on the first point clicked which is not centre.
3)Mark's construction system moves and resizes the first existing ellipse when I create the new ones with auto constraints to the first ellipses points. Conversely the default construction system is much better because it leave the existing first ellipse alone and just auto constrains and sizes the newly created ellipse, which is more logical.

This I have to take a look a revisit it later...

Maybe the create/restore geometry should itself be made as a python command, which is then simply invoked by the gui command and during creation. It looks like useful for scripting too.

On the list below.

Bug.
Create two arcs of ellipses.
Select two endpoints from different arcs
Click tangent. The constraint appears but does nothing.

Thanks!! I always forget tangencies selecting points. On the list.

Also, I'm not a big fan of that construction line for tangency of ellipse to circle or ellipse, I'd like to have just a point. But IMO this can be improved after ellipse is in master. The essence is working.

Can you create tangency with a point? I mean, would not it be PoE? In that case the ellipses could intersect, wouldn't they?

The current code in my skt2_marktaff_creation branch is ready to be pulled as of this commit:

I have understood that I have to pull those commits:
5c3e20af1a95c860112289dcdda54ea99778bc3a
7e274bc32d9aa1a12ab52bfa33ed80353540b062
c656d5165c8bae8f101a2b46af6b12348d06cefe

This is, all the changed that departed from my branch until the commit you marked. I believe this is what you wanted. Let me know if it is not.

In fact it seems to fix the problem DeepSOIC was complaining about above!!! Thanks!!!

To be clear, when you drag an ellipse (created with either method), by clicking and dragging the edge, the ellipse immediately jumps such that its centre is where the mouse point is.

I perfectly understand what you are saying. Also see my reply to DeepSOIC in a previous post. Because of the internal geometry can be there or not, dragging by edge is complicated, in that the temporal constraints that must be set by code to do it, would conflict with the internal constraints. Another option is not to allow to drag an ellipse by edge (just ignore the user), is that better? I am open to suggestions.

If axis-switching for both methods is the consensus, I'd be happy to tweak the code to do that. I'm actually leaning that way a bit now.

If it is not much extra coding effort I think it would be welcome. You can feel the resistance to not being able to resize an ellipse to have its minor larger than its major already at the very begining of this thread...

By value-printing the err variable, I noticed that err approached zero over successive calls until err was 'small' enough, at which point it started over and repeated the algorithm with the exact same input data!

Yes. This is done, I think, because the sketch checks for solutions with a dummy sketch the ActSketch (in ViewProvideSketch), and then repeat it for the real. It might need a fix (or it would welcome a fix). Oh I see you have it after in your post. This happens to me for not reading the full post before replying...

Easy. If the element is not constrained to anything other than the ellipse, it's a safe bet it's not needed. It is already implemented btw: if you hit the restore/hide internal geometry button, when all geometry is already there, it is smart deleted.

Well, I am a little bit undecided here. Imagine that you have an ellipse with its internal geometry tangent to a construction line (this line not being constrained to anything). Wouldn't the user freak out if at deleting the ellipse we also delete the tangent construction line? When I though about the "smart delete", I refrained from doing it that way because of that case. If we all can agree that this is not an issue, yes, we can smart delete. Voting poll is open.

LATEST CODE
==========

Ok latest code here. If you are starting a new development please start from here, if not, please continue whereever you are now. I will integrate it anyway.

https://github.com/abdullahtahiriyo/Fre ... master.git
5f6cede..1a57e2f skt2_marktaff_creation

Please test with this code.

BUGS TO GET FIXED
================
1. Tangency between two arcs of ellipse

OPEN ISSUES
===========
1. DeepSOIC's comment on Mark's construction method above
2. Possible centralization of code for internal geometry (separate implementation to be included in both CreoGeo and Constraints or python command). This can allow tangency to ellipse on creation again...
3. Jim's observation of autoconstraints (see above)
4. Ellipse dragged by center when intended dragged by edge

Let me know if something else new arises

[marktaff]

I have understood that I have to pull those commits:
5c3e20af1a95c860112289dcdda54ea99778bc3a
7e274bc32d9aa1a12ab52bfa33ed80353540b062
c656d5165c8bae8f101a2b46af6b12348d06cefe

This is, all the changed that departed from my branch until the commit you marked. I believe this is what you wanted. Let me know if it is not.

Yep, that's what I wanted.

The only commit after that turns off the autoconstraint from suggested constraint for the ellipse center for my construction method. It is a temp hack. With it on, my method exhibits weird behavior where ellipse 1 gets resized/moved when ellipse 2 is constructed on it. I think the root of the issue is the suggested constraint when drawing assumes your construction method (as mine didn't exist when you wrote that code). So that autosuggest code will have to be tweaked to support both methods intelligently. We can add that to the todo list, but I think we should wait until after I refactor to allow axis-switching in my method.

Next up for me is the refactor to allow axis-switching, then more digging into the solve() issues. I don't think we need to wait on a fix for the solve() issues to commit to master though, as those issues are more general than just the ellipses. But hey, maybe that solution will be trivial.

[DeepSOIC]

Can you create tangency with a point? I mean, would not it be PoE? In that case the ellipses could intersect, wouldn't they?

No, I mean a special new solver constraint that takes two ellipses and a point. Me and Ulrich were proposing this here quite a while ago.
I have just tried to implement it myself, but I have some sort of fail... Smells like viewprovider is confused by seeing strange stuff associated with this variant of tangent constraint... I've ran out of time and will revisit it tomorrow.

[abdullah]

Can you create tangency with a point? I mean, would not it be PoE? In that case the ellipses could intersect, wouldn't they?

No, I mean a special new solver constraint that takes two ellipses and a point. Me and Ulrich were proposing this here quite a while ago.

Ok. Here it is my opinion:
1. I prefer solver constraints over UI hacks (construction points and lines)
2. More solver constraints imply more specific code that gets debuged less over time. In other words, if you failed to see upon coding a border case common to several solver constraints, there is a high chance that it gets fixed for one and not for the other.
3. So it is effectively a trade off. I still favour solver constraints when we are able to create/formulate one.
4. Now if I have to choose between reusing a solver constraint by means of a construction element (line in this case) to apply a ui constraint that we are unable to formulate as a solver contraint, and creating a new solver constraint that relies in the creation of another construction element for it to work (point in this case), I really favour the first option.
5. Of course if you would tell me you have got a formulation without any construction elements, then I will go for it.

It is just my opinion. I may be wrong.

[DeepSOIC]

Sketch.cpp, line 2337
if (pos == mid | pos == none) {
should be boolean or "||", not bitwise or "|". Thanks goes to compiler warning. This doesn't actually affect anything