[Sketcher] Tweaking the Constraint label

[chennes]

Also, to make sure I'm understanding: it is possible for "redundant", "partially redundant", and "fully constrained" to all be displayed at the same time, right?

[chrisb]

Also, to make sure I'm understanding: it is possible for "redundant", "partially redundant", and "fully constrained" to all be displayed at the same time, right?

In theory yes, and even more is possible, but it would be sufficient to show only one. I think it should be the most severe, i.e. "redundant" before "partially redundant" before "fully constrained".
Currently "partially redundant" is shown before "redundant", which is for me equally severe: all of these have to be fixed. However, partially redundant is not seen as an error and so the real error, the redundancy, slips through.
This is a good opportunity to ask another time to make partially redundancy a real error shown with a red sketch .

The attached sketch contains such constraints. If you add a vertical distance it is additionally fully constrained.

[chennes]

It would certainly make the code simpler to just have a simple cascade of error-type conditions, leading ultimately to an "else..." block that displays the "Fully constrained" message. And for most cases that's what it does now. The only exception is those redundancies, which are treated within that final else block. I am happy to make the change you suggest, but hesitant to do it because it's clear @abdullah coded it like this with some specific thought in mind. Abdullah, how do you feel about making the change that @chrisb suggests?

.

[abdullah]

Proposed wording.png

Looks good to me. How about making "Unsolved" a solver message? Abdullah may know why it is not.

ping

Caveats

If solving time was ever relevant, I think that time is long gone. The way solver time is calculated is not representative of actual overall solving time either (does not include QR decomposition time, if several solves are involved only last is reported). So dropping it is appropriate.

"Solving" is divided between "setup" (i.e. SparseQR) and actual solver time (e.g. DogLeg). To make it a little bit more complicated, there is a DogLeg execution during SetUp on a small temporal equation system to decide whether a constraint is redundant or conflicting. But let's obviate this. The first line in the solver messages comes from the "setup" as a consequence of the result of the SparseQR. The second line "Solved"/"Unsolved" comes from the actual solver (e.g. DogLeg). So the first message is about an analysis of ranks of the system, whereas the second message relates to the convergence of the actual solver.

Then, there might be an additional caveat about making the "Unsolved" a "solver message", it is about dragging. During dragging only the "Solved ()" green string is updated. This has to do with the fact that dragging uses a modified equation system to "attach" the pointer position to the system.

So, better than deciding for myself whether it makes sense or not, I explain the underlying logic, as you (and the Community) might come with a better way to represent it (given that Chennes was nice enough to ask ).

There are three blocks of code by priority:

A. The Sketch is empty => so "Empty" state

B. The Sketch has real problems => There is a "setup", but there is no "solver" execution [save for that small system deciding if constraints are conflicting or redundant]. This is the case of dofs<0 (aka "overconstrained"), malformed constraints and conflicting constraints. => In this cases there is no "solved" or "unsolved" string. There was no actual solving. So currently only the first line is shown.

C. The Sketch might have minor problems or not that anyway allowed to run the solver (e.g. DogLeg). This is the case of "redundant constraints", "partially redundant constraints", "no problems" and "no apparent problems but DogLeg does not converge". In this case there are always two strings, because the solver did run.

Some ideas

Without giving it much of a big thought:
- The information "solved" "unsolved" is only present in Block C.
- In Block C, it may be important to know whether the geometry converged to the right position. However, there is additional information to show (what is now the first line). Therefore, rather than a state on its own, it is like an additional attribute of a state. However, I guess this extra information could find its way to the first line.

Hope this helps.

[abdullah]

Also, to make sure I'm understanding: it is possible for "redundant", "partially redundant", and "fully constrained" to all be displayed at the same time, right?

To reply to the previous question I went to the code and realised this issue too.

It is theoretically possible to have "redundant" and "partially redundant" constraints at the same time. However, it is not right to "overwrite" a redundant message with a "partially redundant" one, as the code currently does. In my opinion, this should be changed to reflect that the user should first fix the "redundant" issue and then the "partially redundant issue".

Similarly, a sketch that is "full constrained" and converges (and this is what is found via the solver status message), may still have the aforementioned problems, which IMO should be shown with higher priority, enabling the user to actually fix them.

At the time, I added cases to the preexisting structure following the same logic. However, I think the time has come to fix this situation. I cannot think a better moment than now.

[abdullah]

Also, to make sure I'm understanding: it is possible for "redundant", "partially redundant", and "fully constrained" to all be displayed at the same time, right?

In theory yes, and even more is possible, but it would be sufficient to show only one. I think it should be the most severe, i.e. "redundant" before "partially redundant" before "fully constrained".
Currently "partially redundant" is shown before "redundant", which is for me equally severe: all of these have to be fixed. However, partially redundant is not seen as an error and so the real error, the redundancy, slips through.
This is a good opportunity to ask another time to make partially redundancy a real error shown with a red sketch .

The attached sketch contains such constraints. If you add a vertical distance it is additionally fully constrained.

Ok, I replied without look at your post (I am round robin obsessed). Nice that we arrived to the same conclusion.

Partial redundancies as a blocking error might be too much for many users. It reminds me of those teachers that would hit you with the ruler on the point of the fingers for minor deviations from the right path

[chennes]

Sounds great, and is much easier to code than the crazy alternatives I was thinking about to try to support the multiple overlapping messages! Let me get a PR put together and then people can play around with a real implementation to see how they like it.

[chrisb]

It reminds me of those teachers that would hit you with the ruler on the point of the fingers for minor deviations from the right path

I want one of these rulers! For the forum!

[chrisb]

Thanks for the input and for working on it!

[chennes]

OK, here goes! I've marked this PR as a draft to make sure it doesn't get merged accidentally before you have all gotten a chance to test it out and give your feedback. Please let me know what you think. I included some styling in the Behave-dark stylesheet for testing purposes. If you want to try setting some custom colors in your user.cfg file, the variables to set (in Mod/Sketcher/General) are:

• EmptySketchMessageColor
• UnderconstrainedMessageColor
• MalformedConstraintMessageColor
• ConflictingConstraintMessageColor
• RedundantConstraintMessageColor
• PartiallyRedundantConstraintMessageColor
• SolverFailedMessageColor
• FullyConstrainedMessageColor

https://github.com/FreeCAD/FreeCAD/pull/5091