PR 3669: Review and Expected behaviour

[abdullah]

Hi!

I am reviewing this PR:
https://github.com/FreeCAD/FreeCAD/pull/3669

I can reproduce the crash. The commit message is very well written (explaining what and why). The solution is codewise ok. After the commit the crash is not reproducible.

My question is whether the solution proposed as a fix is what a user would expect from the tool. So this is for community input.

Let's see the what:

FixCloneReferenceConstraints.gif (94.7 KiB) Viewed 1583 times

If you try to do this with FC master it will crash.

Observation 1: In the solution, an equality constraint is used when cloning the reference constraint (every time I write reference constraint I see Normandc twisting). The result is that the lines are set to maintain the length.

Observation 2: If I clone a line without a constraint on it, the resulting line does not maintain the length of the original.

For coherence, I tend to think that when cloning a reference constraint, what a user expects is either:
a) do not copy the reference constraint and do not add any other constraint.
b) copy a reference constraint of the same type on the clone.

I welcome input.

[tom]

Hi Abdullah,

I have to admit that I also sometimes struggle with the desired behaviour of the clone tool, but the longer I think about it the more logical looks the current behaviour, because:

a) do not copy the reference constraint and do not add any other constraint.

that would be the same as drawing a new line

b) copy a reference constraint of the same type on the clone.

and that would be the same as the Copy command already does.

But let's listen to the FreeCAD power users...

[abdullah]

I have to admit that I also sometimes struggle with the desired behaviour of the clone tool, but the longer I think about it the more logical looks the current behaviour,

I do understand your point. I have nothing against having the tool enhanced so that the result is more useful.

How the tool is coded today, is that constraints operating into the original geometry are modified so that when changing the (constrained) dimensions of the original, the "clone" changes accordingly. When the tool is explained this way, a user knows what to expect from the tool.

This explanation could be extended to say: Reference constraints are also modified so that the clone follows the original.

However, the general understanding should still be that unconstrained geometry is not cloned and it won't follow the original. There is not such guarantee.

It could work, let's grab some power user feedback here (others can also post, it is not restrictive, just to ensure I get an answer and can make a decision ) :

...

...

Could I have some input on this one?

[openBrain]

...

Could I have some input on this one?

As you didn't require useful input, here is mine.
First, I never use these clone/copy tools. Don't know why but my workflow doesn't need them.
Anyway, I think that reference constraint can be safely disregarded and ignored in the algorithm.
What I would expect from a 'Clone' command applied to a line is that it creates the clone with both an equality and a parallel constraint vs. the original one.

Then it has to be consistent.
If I currently apply a clone on a circle, it creates an equality constraint on the radius only when the original circle has its radius constrained. I find it inconsistent by itself, and also with any behavior proposal exposed on this thread about line clones.
Regarding circles, I'd expect that a radius equality constraint is always created between source & clone, being the first constrained or not.
I guess other geometry types has to be analyzed as well.

HTH.

[abdullah]

As you didn't require useful input, here is mine.
First, I never use these clone/copy tools. Don't know why but my workflow doesn't need them.
Anyway, I think that reference constraint can be safely disregarded and ignored in the algorithm.
What I would expect from a 'Clone' command applied to a line is that it creates the clone with both an equality and a parallel constraint vs. the original one.

Then it has to be consistent.
If I currently apply a clone on a circle, it creates an equality constraint on the radius only when the original circle has its radius constrained. I find it inconsistent by itself, and also with any behavior proposal exposed on this thread about line clones.
Regarding circles, I'd expect that a radius equality constraint is always created between source & clone, being the first constrained or not.
I guess other geometry types has to be analyzed as well.

HTH.

I will have to sharpen my request drafting skills with you

Point taken, neither a clone. So your proposal is to rename the tool, and call it instead of "clone", "DCF" (Discrete Fouri...wait no... Discounted Cash Flow.. wait, no, Dimensional-Constraint-Follower)?

If nobody posts here I am for just merging the bug-fix as it is. Then documentation will need to be updated

[chrisb]

Sorry for being late. I had the tab open because I wanted to think about it - and then forgot. So here we go:

1) In an ideal world the clone tool should create geometry that looks as its source, and if the source changes it should change in the same way.
This means that cloning is only cloning the geometric elements and not the constraints.

2) Thus as a first recommendation: any dimensional constraint, driven or driving, should be dropped.

3) Anything further may turn out to be difficult. I think that cloning should
- copy the elements
- add geometric constraints which can be fulfilled between the new elements, which means especially
- - coincidence
- - point-to-point tangency
- - point-to-point orthogonality
- add equality constraints as long as the sketch doesn't become overconstrained
- add parallel constraints for straight lines as long as the sketch doesn't become overconstrained

I'm not sure with the last two, if it may occur after all that the clone can be overconstrained.

I don't know how to clone arcs beyond equality of the radius.

[chrisb]

- add parallel constraints for straight lines as long as the sketch doesn't become overconstrained

This can indeed occur, even with a single line: If a line has already a vertical or horizontal constraint.
So either all aligning constraints, i.e. vertical and horizontal should be dropped, or parallel should not be added if an alignment constraint exists. The latter means less clone-like behaviour, but is probably closer to what a human would do.

[chrisb]

- add equality constraints as long as the sketch doesn't become overconstrained

This can indeed create overconstraints as well as this example shows:

Snip macro screenshot-805978.png (8.85 KiB) Viewed 1397 times

I have already applied two equality constraints. Adding another either to the arc or to the line at the right would make it overconstrained.

In fact the right side is in its current state what I would call a perfect clone: It looks always the same as its source except for the position.

[abdullah]

Sorry for being late. I had the tab open because I wanted to think about it - and then forgot. So here we go:

1) In an ideal world the clone tool should create geometry that looks as its source, and if the source changes it should change in the same way.
This means that cloning is only cloning the geometric elements and not the constraints.

2) Thus as a first recommendation: any dimensional constraint, driven or driving, should be dropped.

3) Anything further may turn out to be difficult. I think that cloning should
- copy the elements
- add geometric constraints which can be fulfilled between the new elements, which means especially
- - coincidence
- - point-to-point tangency
- - point-to-point orthogonality
- add equality constraints as long as the sketch doesn't become overconstrained
- add parallel constraints for straight lines as long as the sketch doesn't become overconstrained

I'm not sure with the last two, if it may occur after all that the clone can be overconstrained.

I don't know how to clone arcs beyond equality of the radius.

Thank you very much. I found your input very useful.

In view of your and openbrain input, I see that the clone tool is not very useful/coherent. No matter if we talk about before this PR or after. It simply needs a redesign.

So, I will go for a two step approach:
1) I will merge Tom's PR because it fixes a crash (crashes are a priority) and I do not see how it can lead to an overconstrained situation.
2) I report the following feature request for the 0.20 development cycle to remind me to come back here:
issue #4398

[chrisb]

In view of your and openbrain input, I see that the clone tool is not very useful/coherent.

It is not that bad. In the example above I used the clone tool and it created the geometry, coincidences and tangencies. I had to add only the two equality constraints.