Request for testing: sketcher fix

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
eivindkvedalen
Posts: 598
Joined: Tue Jan 29, 2013 10:35 pm

Request for testing: sketcher fix

Postby eivindkvedalen » Mon Oct 05, 2015 6:24 am

Hi,

https://github.com/eivindkv/free-cad-co ... ions_fixes (rebased on master, forced update) should fix issue #2286. Could some of you guys please test before I make a pull request?

There are still issues if you define an expression that returns a negative value; I'm working on that.

PS! This branch also contains another thing I'm working on, consider it work in progress: An easier way to create aliases. I have now added an Alias command that takes you directly to the alias property page. This command can then easily be bound to a keyboard short-cut. Cells with an alias gets a light yellow background by default, and also gets a tooltip set to the chosen alias.

Eivind
User avatar
DeepSOIC
Posts: 7145
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Request for testing: sketcher fix

Postby DeepSOIC » Mon Oct 05, 2015 9:09 am

Hi!
Sorry, I didn't test anything, but I think that all that sign-absorption convenience is becoming an inconvenience. I think signs should never be hidden from user anymore.
cox
Posts: 963
Joined: Wed Nov 26, 2014 11:37 pm

Re: Request for testing: sketcher fix

Postby cox » Mon Oct 05, 2015 9:15 am

eivindkvedalen wrote:Could some of you guys please test before I make a pull request?
Works perfectly for me, the editing in sketcher is now working.
eivindkvedalen wrote:There are still issues if you define an expression that returns a negative value; I'm working on that.
I am also experiencing the same when positive numbers are returned from expression, when driving constraint in second quadrant.
This could be the same problem you are experiencing.

To reproduce:
Open test1.fcstd
Assign Sketch.Constraint9 = Spreadsheet.Va

Observation:
Point in quadrant 2 jumps to quadrant 1
eivindkvedalen wrote:PS! This branch also contains another thing I'm working on, consider it work in progress: An easier way to create aliases. I have now added an Alias command that takes you directly to the alias property page. This command can then easily be bound to a keyboard short-cut. Cells with an alias gets a light yellow background by default, and also gets a tooltip set to the chosen alias.
I could not figure out how to test this.
Attachments
test1.fcstd
(2.92 KiB) Downloaded 11 times
Need help? Feel free to ask, but please read the guidelines first
abdullah
Posts: 3174
Joined: Sun May 04, 2014 3:16 pm

Re: Request for testing: sketcher fix

Postby abdullah » Mon Oct 05, 2015 12:31 pm

I was thinking of handling this bug, so I assigned it to me in Mantis. Thanks for starting to fix the issue (it was driving me crazy last week) :) I am not handling it any more so not to interfere.

I have found no general issues regarding the code. Some considerations:

Aside from the bug. I have been asking myself partly the same question DeepSOIC does for a while.

If we fully disregard the sign, the problem then is that when you set it to be a length of a line (not the position of a point with respect to the other), you will see the negative sign. This, I think, makes little sense for a length (Distance). I never realised that the radius could actually be negative in any calculation in the sketcher (can it really be?). Anyway, in those cases, IMO, the solution below is the only one that makes sense. Of course, we should take care that whatever we calculate is not negative. If a radius or a distance is calculated or otherwise entered as negative, a warning/error may be shown.

However, DistanceX and DistanceY, may not be considered "lengths", but "distances" with respect to a reference. Not only in the cases considered below (reference is an axis of the sketch), but also on the others (when the reference is applied on a line instead of on its two points OR when the reference is not an axis of the sketch).

Code: Select all

double Constraint::getPresentationValue() const
{
    switch (Type) {
    case Distance:
    case Radius:
        return std::abs(Value);
    case DistanceX:
    case DistanceY:
        if (FirstPos == Sketcher::none || Second != Sketcher::Constraint::GeoUndef)
            return std::abs(Value);
        else
            return Value;
    case Angle:
    case SnellsLaw:
        return Value;
    default:
        return Value;
    }
} 
It may be possible that this "obscuring" of the sign in these cases will be confusing for the user, when working with the spreedsheet and references between sketches... But it may also be that showing the sign confuses the user... anybody has an opinion on this?

English translation: When using horizontal or vertical distance constraints (not distance constraint), anybody sees a problem if instead of showing the value in absolute value, the value shown on the screen is signed (e.g. -30 mm), meaning that the point to the right (in horizontal distance constraint) is the one taken as reference for then definition of the "distance"? Anybody thinks that there is an advantage for the user if the sign is shown?
cox wrote:I am also experiencing the same when positive numbers are returned from expression, when driving constraint in second quadrant.
This could be the same problem you are experiencing.

To reproduce:
Open test1.fcstd
Assign Sketch.Constraint9 = Spreadsheet.Va

Observation:
Point in quadrant 2 jumps to quadrant 1
Additional info: In master it does not change quadrant.
User avatar
DeepSOIC
Posts: 7145
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Request for testing: sketcher fix

Postby DeepSOIC » Mon Oct 05, 2015 12:50 pm

Negative signs can be dealt with in constraint adding commands, which are to swap the elements order to get the sign positive. That will not bore the user with random negative signs that user has to take care of keeping, yet keep the signs consistent if a negative value is entered later.

IMO, there is no sense in hiding the sign of radii either, because if that accidentally becomes calculated negative, the sign will show up and clue the user on why the sketch fails to be solved.
abdullah
Posts: 3174
Joined: Sun May 04, 2014 3:16 pm

Re: Request for testing: sketcher fix

Postby abdullah » Mon Oct 05, 2015 5:02 pm

DeepSOIC wrote:Negative signs can be dealt with in constraint adding commands, which are to swap the elements order to get the sign positive. That will not bore the user with random negative signs that user has to take care of keeping, yet keep the signs consistent if a negative value is entered later.
I agree. This could potentially simplify the whole sign issues.
DeepSOIC wrote:IMO, there is no sense in hiding the sign of radii either, because if that accidentally becomes calculated negative, the sign will show up and clue the user on why the sketch fails to be solved.
Well, I still believe it can only be negative by user error. But in that case, I think there should be a warning (the solver will probably not converge if allowed).
eivindkvedalen
Posts: 598
Joined: Tue Jan 29, 2013 10:35 pm

Re: Request for testing: sketcher fix

Postby eivindkvedalen » Mon Oct 05, 2015 7:17 pm

abdullah wrote:
DeepSOIC wrote:Negative signs can be dealt with in constraint adding commands, which are to swap the elements order to get the sign positive. That will not bore the user with random negative signs that user has to take care of keeping, yet keep the signs consistent if a negative value is entered later.
I agree. This could potentially simplify the whole sign issues.
Ok. So, should I make this pull request as a short-term solution, then you two could work on a better long-term solution? I'm not more familiar with the Sketcher module than being able to add expression support to it. Also, expressions would definitely benefit from a different sign logic; it still has issues even after my fix.

PS! During my work with expressions, I refactored some of the sign code, so now at least most "magic" happens in Constraint::getPresentationValue(). The sign changing logic had to remain unchanged and duplicated in a couple of places, if I remember correctly.

Eivind
abdullah
Posts: 3174
Joined: Sun May 04, 2014 3:16 pm

Re: Request for testing: sketcher fix

Postby abdullah » Tue Oct 06, 2015 5:39 am

By all means Eric. Do the pull request.

As I still have the ticket to my name, I will then open a new thread so that we can discuss and test the new way.

Thanks
triplus
Posts: 8786
Joined: Mon Dec 12, 2011 4:45 pm

Re: Request for testing: sketcher fix

Postby triplus » Tue Oct 06, 2015 7:45 pm

I can confirm issue:

viewtopic.php?f=27&t=12533&start=50#p101615

Is fixed. If i will notice similar behavior in other use cases in the future i will report it.
eivindkvedalen
Posts: 598
Joined: Tue Jan 29, 2013 10:35 pm

Re: Request for testing: sketcher fix

Postby eivindkvedalen » Tue Oct 06, 2015 11:24 pm

triplus wrote:I can confirm issue:

viewtopic.php?f=27&t=12533&start=50#p101615

Is fixed. If i will notice similar behavior in other use cases in the future i will report it.
Thanks for testing and reporting back!

Could you also please play a bit with current https://github.com/eivindkv/free-cad-co ... ions_fixes (rebase and forced update)?

There are three things there:
1) Fix: Sketcher constraints assigned by an expression will now keep its original sign. This makes using expressions much more predictable and nicer, imo.
2) Feature: A new spreadsheet command has been added, accessible with Ctrl+Shift+A by default. This should bring up the alias page of a cell's properties directly with focus on the line edit, making it more convenient to add aliases.
3) Feature: Aliased cells will change their default background to light yellow, and get a tooltip when the mouse is over them.

Eivind