Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sketcher: Change rendering colors of points. #13098

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

PaddleStroke
Copy link
Contributor

@PaddleStroke PaddleStroke commented Mar 22, 2024

This PR changes the color of points in sketcher following DWG discussion.

image

1- edges (Line/arcs...) end/start points : Normal geo color (white)
2- Conic centers : construction geo color (blue)
3- Construction point : construction geo color (blue)
4- normal point : normal geo color (white)
5- When a point is coincident to another point : Constraint color (red) (other points colors render on top).
6 - Point tool now creates point that reflect construction mode. ie real point or construction point.
7 - Remove Vertex color from preferences as vertexes now use the geometry colors

Fixes #12582

@pierreporte let me know if you think it closes #11919

Partial fix of #11799, island would still be interesting.

@PaddleStroke PaddleStroke marked this pull request as draft March 22, 2024 14:22
@github-actions github-actions bot added Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB Sketcher Related to the Sketcher Workbench labels Mar 22, 2024
@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 22, 2024

nice! Is it also switching default behavior (defaul point is normal point, toggle to construction geo is construction point)?
#12582

@PaddleStroke
Copy link
Contributor Author

Yes Fixes #12582
I have not changed the icons. I will let you see if it's needed.

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 22, 2024

Top! Yes let's leave icons for now.

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 22, 2024

@PaddleStroke I've tested this, it works really great on all tools, two minor issues.

One thing I've noticed with internal geometry:
If you constrain internal geometry it won't appear red like with normal or construction geometry.
grafik

If referencing external geometry:
External points are green (as maybe fully constrained). If you make another point coincident, all the other point of an external line become blue, like construction. If you delete the coincident, its green again. When referencing connected external geometries, the coincident point gets red on the endpoints (all other still get blue) and if you pick a point in the middle, all points, incl the coincident in the middle is blue.
Demo video

point-color.mp4

Otherwise it works great, thanks!

@AjinkyaDahale
Copy link
Contributor

External geometry from outside the sketch as well as geometry internal to other geometry (focus, center, etc) are treated differently within sketcher. Do you plan to have different colors for both, or just construction?

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 22, 2024

I don't need another one. It's great how its implemented, but the two issues behave differently. Should be treated like the rest.

@PaddleStroke
Copy link
Contributor Author

Can you try again @maxwxyz please?

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 22, 2024

@PaddleStroke yes.
Internal still is blue:
grafik
External still same behavior:
grafik

@PaddleStroke
Copy link
Contributor Author

@maxwxyz That's weird. Have you rebased ? I have force pushed.
To make sure check the commits # numbers, so that they match with the one in the PR.

@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Mar 22, 2024

image
works on my side. Well the point does not turn red on external. But at least they are external color.

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 22, 2024

ah that was my issue. Yes on external it should be red too otherwise perfect!

@pierreporte
Copy link

It doesn’t fix #11919. It’s even worse IMO because now there are construction points that appear white, and fully-constrained points will still appear green.

Though I do like the red points for the coincidence constrain but I have no idea about how it’s going to feel for actual work. In any case, turning construction points in blue is good: they should be the same color as construction geometry.

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 23, 2024

@pierreporte have you build it? Construction points are never white. Points are same as the line type (real/constr) and only change when constrained.

@pierreporte
Copy link

@maxwxyz I never succeeded to build FreeCAD, even using Conda with help.

It was never clear that points at the end of geometry was of the same type of the geometry. It could explains #11920 and I think that it is a bug in itself, or more precisely that it would block resolving #6884.

Also, I find strange that vertex of rectangles and shapes in general are red, that is there is actually two coincident points, when they were not created explicitly. When creating a polyline, rectangle, or other closed shapes, the same point should be reused. There shouldn’t be several points. It would make the coincident constrain stand out more. Actually, in this case, real coincident constrains could be indicated by an icon like other constrains because it will be much less common (similar to Catia).

@pierreporte
Copy link

By the way, #11919 is still in the list of issues closed by this PR. How to remove it?

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 23, 2024

@pierreporte the are real coincident constraint in all 4 edges of a rectangle. You can delete them and then move all lines individually, so the behavior is correct.
I think it solves #11919 Colors are same as the lines (so all geometry is represented the same way) and can be set with the geometry color in the preferences. Vertex display size outside of the sketch can also be individually set, so there is no need to fix something.
For selecting outside the sketch there is the general problem: #9205
The visibility with dimension lines on top is another issue which was also brought up here: #12756 (comment) so maybe this is an own issue for the rendering order (#11603).
Then regarding color is still this issue open: #10100

@PaddleStroke could you integrate #11920 as well in this?

@pierreporte
Copy link

@pierreporte the are real coincident constraint in all 4 edges of a rectangle. You can delete them and then move all lines individually, so the behavior is correct.

I get the idea. Maybe there should be a tool to transform a point that is common to two entities into two separate points. It exists in other CAD programs so it wouldn’t be entirely new.

I think it solves #11919 Colors are same as the lines (so all geometry is represented the same way) and can be set with the geometry color in the preferences.

We agree that the color should be the same for everything that has the same type. This is precisely why points styles are needed.

Vertex display size outside of the sketch can also be individually set, so there is no need to fix something.

But then the points between lines can be too big. Their size is good at the moment. The problem is only for normal points.

For selecting outside the sketch there is the general problem: #9205
The visibility with dimension lines on top is another issue which was also brought up here: #12756 (comment) so maybe this needs an own issue for the rendering order.

This is unrelated. To select something, you have to see it first, which is difficult for normal points.

Then regarding color is still this issue open: #10100

I created #11919 to propose a partial solution to #10100. The complete solution is in 3 steps:

  1. implement line styles (done)
  2. implement point styles (in discussion): this is [Problem] Non-construction points are hard to distinguish #11919
  3. improve dimensions (in discussion): this is [Feature request] Sketcher: more visual differences for dimensions #12036

As explained in other issues, color is not sufficient to distinguish type and constrain status. They are orthogonal things and as such should be displayed by two different visual variations. Currently, there is still a problem of colors being too similar when points of different types are fully constrained, even more in not-so-complex sketches. If you add colored error hints (like for overconstrained geometry), points styles are a need.

If points 2 and 3 are implemented, you can have just two colors: green for fully-constrained geometry and dimensions, and while for everything else. (Personally I wouldn’t be against a slightly muted color for construction geometry but it would be a bonus not a necessity.)

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 23, 2024

@pierreporte the are real coincident constraint in all 4 edges of a rectangle. You can delete them and then move all lines individually, so the behavior is correct.

I get the idea. Maybe there should be a tool to transform a point that is common to two entities into two separate points. It exists in other CAD programs so it wouldn’t be entirely new.

There is, just delete the coincident constraint.

Vertex display size outside of the sketch can also be individually set, so there is no need to fix something.

But then the points between lines can be too big. Their size is good at the moment. The problem is only for normal points.

I'm not sure if they are different.

The visibility with dimension lines on top is another issue which was also brought up here: #12756 (comment) so maybe this needs an own issue for the rendering order.

This is unrelated. To select something, you have to see it first, which is difficult for normal points.

That's what was mentioned there. I'll add it as comment to the next issue:

  1. improve dimensions (in discussion): this is [Feature request] Sketcher: more visual differences for dimensions #12036

@pierreporte
Copy link

Vertex display size outside of the sketch can also be individually set, so there is no need to fix something.

But then the points between lines can be too big. Their size is good at the moment. The problem is only for normal points.

I'm not sure if they are different.

It’s rather semantic. My point of view is that normal points are closer to datum points than to construction points. From outside the sketch, construction points are only helper and users should avoid attaching things to them. It it why I believe that normal points should look identical to datum points, and that construction points should remain small.

The visibility with dimension lines on top is another issue which was also brought up here: #12756 (comment) so maybe this needs an own issue for the rendering order.

This is unrelated. To select something, you have to see it first, which is difficult for normal points.

That's what was mentioned there. I'll add it as comment to the next issue:

It’s still only one part of the issue. The screenshot in #6884 shows the problem better: the points are barely visible already. This is because the situation is good: the contrast is large enough and the picture was taken so that they should be quite obvious. In real live, they could blend in the shading of the part, be nearly hidden or just viewed from a bad angle. A more obvious shape and size, just for them, would improve the ability of the user to spot them at first sight.

@PaddleStroke PaddleStroke marked this pull request as ready for review March 25, 2024 08:58
@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Mar 25, 2024

I fixed the external point color when coincident @maxwxyz.

#11920 is going to be (I think) very hard to implement. I cannot handle it now.

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 25, 2024

@PaddleStroke nice, looks good now!

@FlachyJoe
Copy link
Contributor

Select 2 ends
image
Click constraint perpendicular
image

@pierreporte
Copy link

This shouldn’t be working.

@maxwxyz
Copy link
Collaborator

maxwxyz commented Apr 9, 2024

yes, should be red. You can do the same with tangent I guess.
@PaddleStroke FYI.

@obelisk79
Copy link
Contributor

I'm losing track at this point.

How many visual states are possible for points now? How many is reasonable to expect a user to recognize and remember? Again, I do not like the trend this is going. It is beginning to seem like a good idea gone bad.

@FlachyJoe
Copy link
Contributor

@obelisk79

  • construction → blue
  • normal w/o coincidence → white
  • normal with coincidence → red
  • highlight (mouse over) → blue
  • selected → green
  • end of a constrained segment → light green
  • fully constrained sketch → green
    😰

@DrD4ffy
Copy link

DrD4ffy commented Apr 9, 2024

Two lines can be perpendicular without being connected. It’s a coincidence that the two points ended up in the same place. They are not constrained and not red. Should be correct.

@FlachyJoe
Copy link
Contributor

@DrD4ffy the points ARE coincident if you create the perpendicular as I shown.

@maxwxyz
Copy link
Collaborator

maxwxyz commented Apr 9, 2024

@DrD4ffy no only if you apply the constraint with selected lines. If you select points and apply perpendicular or tangent they also get coincident. You can try it in sketcher.

@obelisk79
Copy link
Contributor

obelisk79 commented Apr 9, 2024

I think this feature has grown in complexity too much. There are too many states.

It should be something more along these rules:

  1. Points should match the same coloring rules as lines.

Done. That's it.

Anything else and things get overly complicated with edge cases and the users will have to bear the cognitive burden of learning, remembering and processing the various conditions. In this state, this PR results in a worse user experience, not a better one.

@FlachyJoe
Copy link
Contributor

@pierreporte the points are constrained if the segment is.

@pierreporte
Copy link

@FlachyJoe Yes but actually I messed up. What I wanted to say is that a fully-constrained vertex of a line never turns green if the whole line isn’t fully constrained. Similarly, you can’t have a fully-defined orientation and position with white or blue points at the end of a line. It’s a different subject.

@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Apr 9, 2024 via email

@DrD4ffy
Copy link

DrD4ffy commented Apr 9, 2024

@maxwxyz Oh sorry, I am too new at FreeCAD! I didn’t expect that you can use point for a perpendicular constraint, since points can not be perpendicular - only the lines they belong to. So the perpendicular constraint is in fact a “perpendicular and sometimes also coincidence” constraint. Confusing. Who can predict what will happen in this case? Which lines will be perpendicular to the left one? The lower or the upper?
image
For me the issue right now is not the wrong color of the dot but the unexpected implicit coincidence constraint. But OK, now I know.

@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Apr 10, 2024

@obelisk79 removing the red color is possible, the view would indeed be simplified without it. But they provide information that could prevent many open wire situations.
I'm ok with both situations, I just implemented what was validated when I asked on the issue. If majority changes and want to remove the red then we can simply remove it.

Perhaps I would suggest trying it out for a few days to see how you guys feel about it. Then rediscuss it.

@FEA-eng
Copy link
Contributor

FEA-eng commented Apr 10, 2024

If the red color is to be removed then it should be replaced with another way of showing whether there is a coincidence or not. Why would we want to lose this feature?

@obelisk79
Copy link
Contributor

obelisk79 commented Apr 10, 2024

What happens when 4 or 5 points are on the same spot but only 3 are constrained coincident? But drawn red because the last one drawn is coincident?

@maxwxyz
Copy link
Collaborator

maxwxyz commented Apr 10, 2024

White is always on top to see unconnected / open profiles

@PaddleStroke
Copy link
Contributor Author

red points are drawn underneath other points. So if one of the point is not coincident then it's not red.

@obelisk79
Copy link
Contributor

Well, that is at least better behaved than I expected ;)

@obelisk79
Copy link
Contributor

obelisk79 commented Apr 10, 2024

Ok, so theorizing problems aside, things seem to behave rather well for the most part, which is good.

Something I noticed also, is that points actually have a bit of a snap to each other effect that I never noticed before when just moving things around. Was that part of this PR @PaddleStroke or have I just never noticed it before? Either way, bravo.

What I was eventually trying to drive the conversation towards is that drawing coincident with colors wouldn't be necessary if point-coincident was made more reliably with the auto-constraints (ie snapping). Because I run linux, my options for native software I can compare against are limited. However, when compared against OnShape, sketching never has an overt indication if points are coincident. When I was using that software regularly I never felt the need, or experienced issues due to missed coincidence constraints.

Improved auto-constraining (and better promoting use of the polyline tool) would better address the underlying problem rather than needing color or symbol to inform the user that the software didn't meet their expectations. Both features may coexist, but my gut feeling about the red points strikes me as a small bandage applied to a larger wound.

In any case, I commend the quality of this functionality. It works well as it was intended!

@PaddleStroke
Copy link
Contributor Author

@obelisk79 the snap functionality has been introduced some time ago (maybe about 1 year).

I see what you mean. Maybe red points are superfluous. With snapping I think open wires are less frequent already.
Anyhow this feature can be disabled pretty easily (or put under a preference). Perhaps we can wait a little bit to see how people like it or not and act accordingly.

@FEA-eng
Copy link
Contributor

FEA-eng commented Apr 10, 2024

I think that red color for constrained points is a good idea. Missing coincidences are still very common, especially when sketches are obtained from drafts (e.g. imported dwg/dxf files - example) or even subjected to small modifications like trimming which can lead to the loss of coincident constraints. We've seen lots of forum threads about open wires that could be quickly solved without having to use the Validate Sketch tool if there was some visual feedback about coincidences.

However, I'm also a fan of customizability and I think that the best way would be to offer an option to disable this or change the colors as desired.

@obelisk79
Copy link
Contributor

@FEA-eng that all seems fine but let's not overlook the underlying issue that it is too easy to get an open wire in the first place. Fix that, and support requests will dramatically drop even without color states.

@Roy-043
Copy link
Contributor

Roy-043 commented Apr 16, 2024

There is an issue when you hide a coincident constraint. The color of the constrained point then stays red.

@Roy-043
Copy link
Contributor

Roy-043 commented Apr 16, 2024

An example where a coincident point receives the wrong color:

  1. Create a new sketch.
  2. Create a unconstrained line.
  3. Create a circle with the center at the origin.
  4. The center point is marked as coincident (red). OK.
  5. Apply a radial constraint to the circle.
  6. The center point is now blue.
OS: Windows 8 build 9600
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.36807 (Git)
Build type: Release
Branch: main
Hash: cc96f2718e139d670e08c4226bdea9955a102ed6
Python 3.11.8, Qt 5.15.13, Coin 4.0.2, Vtk 9.2.6, OCC 7.7.2
Locale: Dutch/Netherlands (nl_NL)
Installed mods:

@maxwxyz
Copy link
Collaborator

maxwxyz commented Apr 20, 2024

@PaddleStroke when creating geometry using the symmetry tool and creating symmetry constraints the new points are white, not red, although the wire is closed:
grafik

should I create a new issue?

@PaddleStroke
Copy link
Contributor Author

Well yes it is because the shape is actually not constrained with coincidence but with symmetries. Which kind of sucks I agree. But it was a bit difficult to add coincidences and symmetric and not have the sketch overconstrained.
But now that I think about it, I think I see a way to handle this. We can create the coincidences then remove duplicated symmetries. I can feel it's going to be painful to get it right, with closed wires and tangencies there are going to be a lot of overconstraining edge cases.
It's closely related to #13551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB Sketcher Related to the Sketcher Workbench
Projects
None yet