Request for testing: sketcher fix
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Be nice to others! Respect the FreeCAD code of conduct!
-
- Posts: 602
- Joined: Tue Jan 29, 2013 10:35 pm
Request for testing: sketcher fix
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
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
- DeepSOIC
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: Request for testing: sketcher fix
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.
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.
Re: Request for testing: sketcher fix
Works perfectly for me, the editing in sketcher is now working.eivindkvedalen wrote:Could some of you guys please test before I make a pull request?
I am also experiencing the same when positive numbers are returned from expression, when driving constraint in second quadrant.eivindkvedalen wrote:There are still issues if you define an expression that returns a negative value; I'm working on that.
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
I could not figure out how to test this.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.
- Attachments
-
- test1.fcstd
- (2.92 KiB) Downloaded 40 times
Need help? Feel free to ask, but please read the guidelines first
Re: Request for testing: sketcher fix
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).
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?
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;
}
}
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?
Additional info: In master it does not change quadrant.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
- DeepSOIC
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: Request for testing: sketcher fix
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.
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.
Re: Request for testing: sketcher fix
I agree. This could potentially simplify the whole sign issues.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.
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).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.
-
- Posts: 602
- Joined: Tue Jan 29, 2013 10:35 pm
Re: Request for testing: sketcher fix
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.abdullah wrote:I agree. This could potentially simplify the whole sign issues.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.
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
Re: Request for testing: sketcher fix
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
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
Re: Request for testing: sketcher fix
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.
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.
-
- Posts: 602
- Joined: Tue Jan 29, 2013 10:35 pm
Re: Request for testing: sketcher fix
Thanks for testing and reporting back!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.
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