[Resolved] PR #4942 new option to show/hide sketcher constraints

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Locked
chrisb
Veteran
Posts: 53930
Joined: Tue Mar 17, 2015 9:14 am

Re: PR #4942 new option to show/hide sketcher constraints

Post by chrisb »

abdullah wrote: Tue Sep 21, 2021 6:07 pm If it is not in the tracker, it would make sense.
Opened issue #4748.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
wsteffe
Posts: 461
Joined: Thu Aug 21, 2014 8:17 pm

Re: PR #4942 new option to show/hide sketcher constraints

Post by wsteffe »

abdullah wrote: Tue Sep 21, 2021 6:18 pm
The logic of the virtual spaces resides currently in the constraint and the sketch. There is no object "virtual space".

Multiple virtual spaces without mutually exclusivity would require that a constraint stores the plurality of virtual spaces in which it is visible. The sketch knows which virtual space is the one being shown.
You do not need to change the data structure in order to remove the mutually exclusivity.

I presume that now each constraint has an associated integer variable which tells you in which space it is visible:
1: visible in space 1
2: visible in space 2

A more general implementation without mutual exlusion would be

0: not visible in any space
1: visible in space 1
2: visible in space 2
3: visible in both space

Same numbers in a binary represantation with 2 digits (0 filled at the left side):

00: not visible in any space
01: visible in space 1
10: visible in space 2
11: visible in both spaces

With 3 virtual spaces:

000: not visible in any space
001: visible in space 1
...
111: visible in all spaces

So you may still keep a single integer variable for each constraint. You have just to look at its binary representation.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PR #4942 new option to show/hide sketcher constraints

Post by abdullah »

wsteffe wrote: Wed Sep 22, 2021 7:19 am
abdullah wrote: Tue Sep 21, 2021 6:18 pm I presume that now each constraint has an associated integer variable which tells you in which space it is visible:
It is a boolean and the visibility of the virtual space at sketch level is another boolean. Yes, the obvious modification is changing the constraint member to a set. This can be a set of bits or a set of integers (it is merely a matter of storage/memory space vs code speed). The integer set would probably result in clearer code. But that is an implementation detail.

In any case, one would need arrangements to create new spaces, name them and delete them. I do not favour creating a predetermined number of spaces called "user virtual space 1" "user virtual space 2" to be available always no matter if the sketch uses them or not without a clear indication of what they are being used for. A user cannot be left to try to remember how he or she were using the virtual spaces in that sketch of that project two years ago. This will need to come (maybe in yet another tab in the tabwidget).

In any case, when geometry layers are implemented, virtual spaces will need to be revisited. There may be implications from the layer implementation and we will be in a better position to shape the virtual space functionality.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PR #4942 new option to show/hide sketcher constraints

Post by abdullah »

Just added a commit fixing the long delay when operating hide/show on a specific selection (as described in the first post of this thread).
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PR #4942 new option to show/hide sketcher constraints

Post by abdullah »

uwestoehr wrote: Mon Sep 20, 2021 5:59 pm
abdullah wrote: Mon Sep 20, 2021 5:44 pm How do we shape it further?
Awesome!

I only miss a toggle button to change if the selected annotation type should be shown or hidden. In your screencast the filtered types will be shown all other not. There should be a way to say, "show everything but the filtered type" and there should also be a ways to hide everything.
I have just merged the work.

Please, let me know if it matches your use case.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #4942 new option to show/hide sketcher constraints

Post by uwestoehr »

abdullah wrote: Wed Sep 22, 2021 4:49 pm I have just merged the work.

Please, let me know if it matches your use case.
Oh, OK. I was already in compiling your fork. So in fact I had no chance in testing in advance. :|

I would have preferred that you make a PR first and let us compile, test and look at the code.

I hope tomorrow we will discuss this in the meeting - so please join us (last call ;) ):
I think having a pending PR helps people the most to test, check and giving feedback to the code. We see that this works well and for many PRs we get code reviews. This is good, since when another person understands the code, it is good enough that others can fix bugs in it in future, that there is sufficient documentation of the code etc.

I can only speak for my PRs, having them pending for some days helped me a lot to have better code quality and documentation since for me as creator everything is of course clear to me, but it must be clear for others too.


However, you made an amazing job to implement the annotation hiding/showing :D
I will test it out the next days with real-life uses cases (more than 1000 constraints) and report back.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PR #4942 new option to show/hide sketcher constraints

Post by abdullah »

uwestoehr wrote: Wed Sep 22, 2021 5:02 pm
abdullah wrote: Wed Sep 22, 2021 4:49 pm I have just merged the work.

Please, let me know if it matches your use case.
Oh, OK. I was already in compiling your fork. So in fact I had no chance in testing in advance. :|

I would have preferred that you make a PR first and let us compile, test and look at the code.

I hope tomorrow we will discuss this in the meeting - so please join us (last call ;) ):
I think having a pending PR helps people the most to test, check and giving feedback to the code. We see that this works well and for many PRs we get code reviews. This is good, since when another person understands the code, it is good enough that others can fix bugs in it in future, that there is sufficient documentation of the code etc.

I can only speak for my PRs, having them pending for some days helped me a lot to have better code quality and documentation since for me as creator everything is of course clear to me, but it must be clear for others too.

However, you made an amazing job to implement the annotation hiding/showing :D
I will test it out the next days with real-life uses cases (more than 1000 constraints) and report back.
I tend to merge minor features where I did not encounter a major point requiring discussion directly to master. This is efficient for me and saves resources. I am generally reviewing sketcher PRs. In that, I try to ensure consistency, readability and intend of the code. I try to apply the same standard to me. However, I am certainly not flawless. The good part is that the code is open source and open to public scrutiny. Feel free to look at it. I can benefit from code reviews of my merged code too. Let me know if you find inconsistencies, mistakes or meaningful ways to improve it. The fact that is merged does not mean that it cannot be improved, or that it should not be fixed or even reverted if appropriate. I am always learning.

As for additional testing, I was not expecting to have additional testing before merge. It is generally the case for the community discussed features where the working is shown (animations) and repetitive feedback is given, that real testing begins after merge once the initial feedback is positive. I am happy to see that you are going to give it some heavy testing. This is what the functionality needs now. The fact that is merged does not prevent improving it. Rather the fact that is merged will set the functionality under the magnifier glass as all users will have access to it and will be the drive to improve it where necessary. This has been the traditional way of improving the Sketcher.

As you will be testing the code with a very large amount of constraints, I am interested if you could repeat the test that prompted you to tackle this (the minutes long delay when group selecting in the widget list and hiding/showing using the contextual menu). This long delay should be gone (it is gone in my around 100 constraint tests). This is one point I wanted to tackle. No normal operation should take these long delays. I wait for your feeback :D
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #4942 new option to show/hide sketcher constraints

Post by uwestoehr »

abdullah wrote: Wed Sep 22, 2021 5:58 pm I tend to merge minor features where I did not encounter a major point requiring discussion directly to master.
I can understand this, but it is error-prone. E.g. what compiles for you might not compile on other computers. We had this case this week with a merge from Yorik and since we don't have a running CI at the moment that slips through.

Moreover, it is clear that one as creator thinks the code it is fine, otherwise one would not have written the it that way. But also the most experienced coder cannot document code out of the box in all cases that others understand it. And what I miss the most in FC's code is documentation. I mean not WHAT is done by the code but WHY, why this way, what are the corner cases etc. A review by others would uncover this. When it is already merged, nobody looks into if it is understandable. But then a year later an issue arises and someone wants to fix it, he then looks into the code and don't understand it. So when it is already merged, in most cases the code documentation slips through.

Personally have this issue with code of TechDraw. We lost Wandererfan and I thought I could fix some issues and often see that I cannot understand why the code is as it is.

Back to the content:
I had a simple real-life use case with a deadline today. So here is my example:
FreeCAD_kjISXiHJ3b.png
FreeCAD_kjISXiHJ3b.png (70.4 KiB) Viewed 2419 times

I need everything to be shown, but not the datums. So I thought, I select the datums and then an option to hide (or later show them again). But I cannot find a way to do this.

The option named" Track filter selection" is not clear to me. What is tracked?
Here is a screencast, so no matter what I do, nothing changes. So the option has no effect and I cannot show everything but not the datums:
XlwgSb2iTx.gif
XlwgSb2iTx.gif (76.95 KiB) Viewed 2419 times
Attachments
Spiel-Schrauben.FCStd
(7.23 KiB) Downloaded 34 times
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #4942 new option to show/hide sketcher constraints

Post by uwestoehr »

Maybe It is not clear what I want/need:
- select a annotation type in the combobox
- then use an option or button, to hide all annotations of the selected type
- if the annotations of one type are _all_ already hidden, I need a quick way to show them
- if the annotations of one type are _all_ already shown, I need a quick way to hide them

In my example I needed to investigate the maximal displacement angle. So constraints like rectangular etc. are important, the dimensions are not. While we discussed it, I hide the dimensions since I realized one colleague went into the wrong direction. However later someone else asked to show the dimension again... In my real-life I often discuss sketches/designs, so I need to be able to quickly show/hide some annotations in a sketch.
GeneFC
Veteran
Posts: 5373
Joined: Sat Mar 19, 2016 3:36 pm
Location: Punta Gorda, FL

Re: PR #4942 new option to show/hide sketcher constraints

Post by GeneFC »

uwestoehr wrote: Wed Sep 22, 2021 9:26 pm The option named" Track filter selection" is not clear to me. What is tracked?
As I said yesterday, the track filter selection works perfectly. Your screencast shows you are not using it.

Click the Automation tab and the Track box. Then select any item in the dropdown menu (starts with All), and only those constraint items will show in the viewing window. If you click only "All" as your screencast shows then it will appear that nothing happens.

It may be that the exact wording could be tweaked a bit.

Gene
Locked