Sketcher_Dimension: Enable the user to select what angle he wants.

[PaddleStroke]

Fixes #10603
In Sketcher_Dimension and also in general moving the constraint : Enable the user to select the complementary angle he wants based on the position of the mouse. Similarly as how he can choose distance/DistanceX/Y.
Forum discussions :
https://forum.freecad.org/viewtopic.php?t=28321
https://forum.freecad.org/viewtopic.php?t=80994

[PaddleStroke]

@chennes this one is pretty cool :)

[chennes]

When I flip the angle to the "wrong" side, the constraint markers are mis-rendered:

[howie-j]

Hmm, i'm getting quite unexpected results:

pr_10657.webm

OS: Fedora Linux 38 (Workstation Edition) (GNOME/gnome)
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.34260 (Git)
Build type: Release
Branch: pr_10657
Hash: 4b69642
Python 3.11.5, Qt 5.15.10, Coin 4.0.0, Vtk 9.2.5, OCC 7.6.3
Locale: English/United States (en_US)

[howie-j]

Tried on X11 instead of wayland with a clean/default setup.
It works as expected with crossing lines, but not with coincident or separate lines:

(The cursor is hidden, sorry)

pr_10657_x11.webm

[PaddleStroke]

@chennes what did you meant by misrendered? Is it that the value is writen on top of the arc. Or is it that you expected the complementary angle (ie 360 - angle)? In this PR the goal was to make the supplementary angle (180-angle).

@howie-j you are right, I tested only on crossing lines. The two other cases don't work, I can reproduce the same as in your video. I will investigate

[chennes]

@chennes what did you meant by misrendered? Is it that the value is writen on top of the arc. Or is it that you expected the complementary angle (ie 360 - angle)? In this PR the goal was to make the supplementary angle (180-angle).

I think that without rendering the connecting lines, showing the supplementary angle doesn't make any sense -- in the animation shown in this post there are lines indicating what angle is being displayed. In this case, the angle is just floating out in space, with no clear reference.

[qewer33]

It might also make sense to implement this for the normal Angle tool as well when it's stable. For the users who prefer the separate toolset

[PaddleStroke]

You are right @chennes. I was planning to try to improve the connecting lines for angles. Though I don't know if this belong to this PR as it's quite a separate part of the code.

[PaddleStroke]

@qewer33 the normal angle tool does not offer a moment to do that. As it's a one shot command. No mouse move time to do it.
However what we can do is add to the datum edit dialog a 'Reverse angle' checkbox that would swap the angle.

[PaddleStroke]

@chennes I fixed the problem that @qewer33 mentioned and I added the brackets to the ifs.
I will start looking into how to improve the arc SoDatumLabel. But I think it's best to separate the PRs.

[howie-j]

Very nice, almost all bugs fixed in the last commit.

The only bug i could find now is when creating an angle between a line and X or Y axis:
When the mouse goes "negative" compared to the axis, the dimension snaps 180' opposite until the cursor passes 90' to the line:

dim_angle_bug.webm

(the blue construction line normal to the regular line is just for illustration)

[chennes]

I will start looking into how to improve the arc SoDatumLabel. But I think it's best to separate the PRs.

Fair enough -- once @howie-j is satisfied with the PR I'll merge it.

[PaddleStroke]

After a long fruitless investigation, I finally found that this bug is not due to my PR. It is in fact a bug of moving the constraint.
First it does not need to be an axis. This happen with any lines. Though in some cases, such as the axis, it's more visible. The axis is equivalent to a 1mm long line actually. If you try the same as you did with a random 1 mm horizontal line you'll have the same result.

But this is not due to sketcher_dimension : Make the angle and validate it first. Then try to click and drag the constraint as you did, the described behavior is achieved.

So I'm not sure what to do here. Do I have to fix it in this PR ?

The angle constraint 'drag and move' could benefit from a rework as we could work to enable the angle flipping during the mouse move rather than in sketcher dimension... Which would make this PR useless.

I will check tomorrow if I can move this code out of sketcher_dimension and into the moving of angle constraint

[chennes]

Do I have to fix it in this PR ?

No, certainly not -- if the fix were obvious and simple it would be fine for you to do so, but if it's not a regression due to this PR, there's no requirement that you fix it. You might consider submitting an Issue about it, though. Do you want to merge this as-is then?

[howie-j]

For me it's more than fine to merge as-is, as long as we create an issue we can point to regarding the constraint flipping.
The PR provides a very nice improvement to creating angles that has annoyed me and probably many other users for years. Merci beaucoup Pierre!

[PaddleStroke]

Ok so the good news is that it was fairly easy to move to 'moveConstraint' so that it now works not only on angle creation, but also when you move an angle around.
The bad news is that there is now an issue when the constraint has an expression. It should be easy and just adding/removing 180- from the expression. The issue is how to access the expression.
Cf my pseudo code here :

                    // Edit the expression if any
                    /*
                    if (Constr->hasExpression()) {
                        std::string expression = Constr->getExpression();
                        if (expression.substr(0, 3) == "180-") {
                            expression = expression.substr(3);
                        }
                        else {
                            expression = "180-" + expression;
                        }
                        Constr->setExpression(expression);
                    }*/

@wwmayer do you perhaps know if there is an easy way to access the constraint expression? I tried to follow the logic in EditDatumDialog but I'm having trouble as it seems it use an expression binding to the widget there.

[wwmayer]

do you perhaps know if there is an easy way to access the constraint expression?

@abdullahtahiriyo is there no method to get the index (int) for a given constraint?

Assuming you know the index of the constraint then you can access a possible associated expression via the sketch object:

int index = ...
SketchObject* sketch = ...
App::ObjectIdentifier path = sketch->Constraints.createPath(index);

auto info = sketch->getExpression(path);
if (info.expression) {
    std::string expression = info.expression->toString();
    expression = ...;
    try {
        std::shared_ptr<App::Expression> expr(App::Expression::parse(sketch, text));
        // there is a bug in the SketchObject API because setExpression() is protected but public in DocumentObject
        App::DocumentObject* base = sketch;
        base->setExpression(path, expr);
    }
    catch (const Base::Exception&) {
    }
}

I don't know if there is a better way than this but it works.

expression = "180-" + expression;

Manipulating the expression this way is fragile because you don't know how the expression looks like. Better do
expression = "180 - (" + expression + ")";
The parentheses are important because the old expression could be "75 - 15". Without them you would get 180 - 75 - 15 = 90 instead of 180 - (75 - 15) = 120

Another thing to check is whether the old expression uses the degree symbol to avoid a unit mismatch when changing the expression.

[PaddleStroke]

Yes I have the index. Thanks very much, I was in down the rabbit hole and came up with the following but I was stuck then

                    App::Property* expressionEngineProperty = getSketchObject()->getPropertyByName("ExpressionEngine");
                    if (expressionEngineProperty) {
                        auto* expressionEngine = static_cast<App::PropertyExpressionEngine*>(expressionEngineProperty);

                        // Access and manipulate the expressions
                        for (auto& expressionPair : expressionEngine->getExpressions()) {
                            if (expressionPair.first == getSketchObject()->Constraints.createPath(constNum)) {
                                const App::Expression* expression = expressionPair.second;
                                std::string expressionString = expression->getExpressionString();

                            }

                        }
                    }

Yes you are right, it needs paranthesis indeed.

[PaddleStroke]

App::ObjectIdentifier path = getSketchObject()->Constraints.createPath(constNum);
Is giving me a 'incomplete class is not allowed' error. What includes I need for it to work? I'm in ViewProviderSketch.cpp

[wwmayer]

Some general remarks:

• You undid the auto-formatting done by the pre-commit hook. If you want to make sure that the changes persist you should put // clang-format off and // clang-format on around the code. But make sure that the lines won't be much longer than 100 characters.
• ViewProviderSketch::moveConstraint() is one of these huge functions with more than 250 lines of code. Take the opportunity and at least refactor the else-branch you are working at. This bad practice to bloat functions more and more must be stopped.

[wwmayer]

#include <App/ObjectIdentifier.h>
#include <App/Expression.h>

[PaddleStroke]

Do you think I should rather make a 'hasExpression' 'getExpression' and 'setExpression' into Sketcher::Constraint ? The issue I see is that the constraint object currently doesn't have access to sketch object.

[wwmayer]

Sketcher::Constraint looks like the wrong place to me. Better put it into SketchObject or where it makes sense to DocumentObject.

[sliptonic]

Moving to Draft. Please reset when ready.

[PaddleStroke]

Ok so I have split the move constraint function. And moved some code into SketchObject.
Is that all good now?

Side note: The problem raised by @howie-j is solved.

[howie-j]

Tested, could not detect any misbehavior. Very nice work 🥇

[howie-j]

Damn this is cool

movable.webm

[PaddleStroke]

Typo fixed + typeId test replaced by isLineSegment(*geo1) (line 1779, 1850, 1854).
And rebased.

[Syres916]

@PaddleStroke shouldn't errors presented to the user be made available for translation?

[PaddleStroke]

This error is not a user-facing error. It's not supposed to ever appear so I think it's not worth translating.