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

Revert: "Sketcher: Joint Line and Polyline in a command group." #14118

Closed
wants to merge 1 commit into from

Conversation

berberic2
Copy link
Contributor

This reverts commit cb0a2d2.

the commit 6bb7775, groups the line- and polyline-tool in a submenu, saving a tiny bit a space in the toolbar but clearly reduces the usability of the most-used sketcher tools considerably.
The main argument for this commit is, that sometimes in the future, the polyline-tool will be improved and replace the line-tool. None of this improvement can be seen now and there does not seem to be a clear understanding of how this new polyline-tool should work at all.
So, at the moment it is a degredation. The UI-change should be made when the polyline/line-improvement has happend.

For details see the discussion on
#13509
and
https://forum.freecad.org/viewtopic.php?p=759200 [german]

@github-actions github-actions bot added the WB Sketcher Related to the Sketcher Workbench label May 18, 2024
@maxwxyz
Copy link
Collaborator

maxwxyz commented May 18, 2024

I'm not in favor of a plain revert of this PR. If the current implementation is not in your personal favor, we should discuss a preference otherwise (although I'm not supporting a preference for every individual tool).
@FreeCAD/design-working-group FYI

@FEA-eng
Copy link
Contributor

FEA-eng commented May 18, 2024

I’m also against this plain revert. There was no agreement on this (quite opposite, most users want to keep the change) so the PR is out of place.

@wwmayer
Copy link
Contributor

wwmayer commented May 18, 2024

If the current implementation is not in your personal favor

The changes made to #13509 are not about personal preferences but are an obvious regression in usability because in certain situations it requires a lot of extra work to achieve what the user wants. Just watch the videos and read the explanation created by berberic2 and you will see why.

And in the past whenever it was about personal preferences things have always been kept flexible by allowing them to change a switch in the parameter editor. Not so with the mentioned PR where this possibility is not offered.

@maxwxyz
Copy link
Collaborator

maxwxyz commented May 18, 2024

That's why I'd propose a preference instead of reverting everything, same as with the dimension constraint tool.

@FEA-eng
Copy link
Contributor

FEA-eng commented May 18, 2024

Yes, a preference is definitely a good idea here. Maybe in the future preferences like this will be replaced by customizable toolbars but for now, one more preference shouldn’t hurt. FreeCAD doesn’t have many settings compared with other CAD software. If only it was possible to search in preferences…

@0penBrain
Copy link
Contributor

I'm not in favor of a plain revert of this PR. If the current implementation is not in your personal favor, we should discuss a preference otherwise (although I'm not supporting a preference for every individual tool). @FreeCAD/design-working-group FYI

I worry "It has been decided by the DWG" is an argument for everything. It isn't according current Contribution Process. DWG should have created a GH issue about the original change so a broader discussion may happen. Merged regressions may have been caught there before bothering users. It also tends to prove that DWG as it is today doesn't include enough real FC users to take only sensible decisions.
I have no problem doing a plain revert. This was part of the new merging motto. Refusing it will make the merging process lamous. According the current win/loss balance -- 100% users have 1 icon less in their toolbar ; 1% users get their workflow highly degraded --, plain revert seems reasonable.

I’m also against this plain revert. There was no agreement on this (quite opposite, most users want to keep the change) so the PR is out of place.

There was no agreement on the original PR, so not a valid argument.

That's why I'd propose a preference instead of reverting everything, same as with the dimension constraint tool.

I'd be in favor of plainly reverting and informing the original PR author that it should be resubmitted as a new PR including a user preference to enable the icon merge.

@obelisk79
Copy link
Contributor

an obvious regression in usability because in certain situations it requires a lot of extra work to achieve what the user wants

The obvious regression isn't clearly outlined by anyone here. Merely stated as an 'obvious regression'. Clear delineation of the nature of the regression should be defined. If the argument is that it takes an extra click to create a single line with the polyline tool, then that is not a particularly great point being made.

Previous points made which were relatively valid critiques of this change: PolyLine doesn't have OVP implemented. Too many discrete draw modes (six). Otherwise, while drawing lines, a single end to end line involves three mouse clicks instead of two.

Since it has been so controversial (with 3 or 4 vocal users), I agree that adding a simple preference to disable this change seems to be the best course of action until the PolyLine tool can have its deficiencies addressed.

@0penBrain
Copy link
Contributor

The obvious regression isn't clearly outlined by anyone here.

Be serious please. What is unclear here: #13509 (comment) ?

@maxwxyz
Copy link
Collaborator

maxwxyz commented May 18, 2024

That's no change that was introduced with the original PR, it's the current behavior of the polyline tool. Both tools are still available.

@0penBrain
Copy link
Contributor

That's no change that was introduced with the original PR, it's the current behavior of the polyline tool. Both tools are still available.

Guys, really looks like you're not using FC at all... 😕 The original PR is a PITA for one frequently using both Line and Polyline tools for what they each are targeting. As it is now, it is like if Line and Arc would have been merged in same icon. This is a major user experience crap. Is it what DWG is aiming at ?

@kadet1090
Copy link
Contributor

kadet1090 commented May 18, 2024

I'd also say that this is problematic and changes that affect user experience so much should not be pushed. Let's back off, preserve this as optional behavior, personally I'd lean towards it being opt-in not opt-out. When polyline would be improved to a point where the line tool is not needed we can think about revisiting it.

Also, no body is deciding about anything, things are opinionated nothing more.

@obelisk79
Copy link
Contributor

obelisk79 commented May 18, 2024

I worry "It has been decided by the DWG" is an argument for everything. It isn't according current Contribution Process. DWG should have created a GH issue about the original change so a broader discussion may happen. Merged regressions may have been caught there before bothering users. It also tends to prove that DWG as it is today doesn't include enough real FC users to take only sensible decisions. I have no problem doing a plain revert. This was part of the new merging motto. Refusing it will make the merging process lamous. According the current win/loss balance -- 100% users have 1 icon less in their toolbar ; 1% users get their workflow highly degraded --, plain revert seems reasonable.

  • I didn't see the DWG invoked here as the 'holy authority'. This particular change was part of a larger proposal I made many months ago, which I had intended be implemented as a wholesale expanded vs compact toolbar setting in sketcher, because I had anticipated this very controversy.

  • It is extremely difficult to make meaningful changes without 'bothering' a handful of users. This is a well studied phenomenon in the area of UX.

  • Since all participants of the DWG use freecad, and several work with CAD as part of their profession. I'd like to better understand what you consider to be a real FreeCAD user.

  • Reverting a change because it alters what some users are accustomed to and are upset 1% by your estimate, isn't necessarily a great way to conduct business.

I'm just not interested in petty infighting and circular arguments over this change. Revert it and once polyline has improvements similar to what I proposed in the original PR's comments we need to revisit this change with a closer eye towards just removing the line tool completely and renaming polyline.

@0penBrain
Copy link
Contributor

  • I didn't see the DWG invoked here as the 'holy authority'.

#13509 (comment)
#13509 (comment)

  • It is extremely difficult to make meaningful changes without 'bothering' a handful of users. This is a well studied phenomenon in the area of UX.

Only true in the case of meaningful change. Which it isn't.

  • Since all participants of the DWG use freecad, and several work with CAD as part of their profession. I'd like to better understand what you consider to be a real FreeCAD user.

Someone that would both frequently use Polyline and Line -- with automatic coincidence on both endpoints --. Looks pretty basic, but not covered in DWG yet. Maybe professional there only machine/print/... cylinders. 😕

  • Reverting a change because it alters what some users are accustomed to and are upset 1% by your estimate, isn't necessarily a great way to conduct business.

Same thing as assessing risks only based on likelihood completely discarding severity. Will fail soon.
If every human being gets a free glass of water at the price 1% has severe disease, is it a great way to conduct business ?

I'm just not interested in petty infighting and circular arguments over this change. Revert it and once polyline has improvements similar to what I proposed in the original PR's comments we need to revisit this change with a closer eye towards just removing the line tool completely and renaming polyline.

Sounds wise.

@obelisk79
Copy link
Contributor

obelisk79 commented May 18, 2024

If every human being gets a free glass of water at the price 1% has severe disease, is it a great way to conduct business ?

If it prevents 99% of human beings from dying of thirst, yes. Your provided anecdote is a poor example, neither your example or my response really apply here.

Someone that would both frequently use Polyline and Line -- with automatic coincidence on both endpoints --.

PolyLine does add automatic coincidence/point-on-object on both endpoints, just like the line tool. This is why I keep asking for an itemized list of lost functionality/regressions. From my perspective, I've seen the following complaints filed:

  • I have to make an extra click for when I need to draw many dozens of single lines in one sketch (trivial)
  • I don't get automatic coincident constraints using polyline (which is incorrect)
  • Polyline lacks the quick dimension function right now (valid)
  • Polyline's modes are too numerous for efficient cycling while drawing complex wires (my point of frustration with the tool)

Do you have more points to add to the list so I can track them as well?

@0penBrain
Copy link
Contributor

PolyLine does add automatic coincidence/point-on-object on both endpoints, just like the line tool. This is why I keep asking for an itemized list of lost functionality/regressions. From my perspective, I've seen the following complaints filed:

  • I have to make an extra click for when I need to draw many dozens of single lines in one sketch (trivial)

I don't understand how the hell the DWG can be pushing hours to defend a change that will save 1 click a day, and then take decisions that add so much ones...

  • I don't get automatic coincident constraints using polyline (which is incorrect)

Did you look at the video I pointed above ? The main problem is that when clicking arc endpoint with polyline, it goes into tangent mode and you have to enter 'M' and again to get the correct mode. This is highly inefficient.

@obelisk79
Copy link
Contributor

I don't understand how the hell the DWG can be pushing hours to defend a change that will save 1 click a day, and then take decisions that add so much ones...

As with most things in life a balance has to be made between clicks, options, efficiency, what we expect users to mentally track, etc etc. Sometimes a click should be conserved, other times concessions need to be made for greater impact in other facets of UX.

Did you look at the video I pointed above ? The main problem is that when clicking arc endpoint with polyline, it goes into tangent mode and you have to enter 'M' and again to get the correct mode. This is highly inefficient.

I missed the link to the video, I had looked at the other two links you provided. This also goes back to my problem with the many discrete modes of polyline. Forced tangential constraint from the endpoint of an arc is indeed a serious problem, switching tools modes for a user like this is yet another problem. That is a bit different that 'end to end lines don't get auto constraints' that was a broader statement for a specific identifiable issue which I was already aware of, but now better understand what you were trying to communicate.

@PaddleStroke
Copy link
Contributor

PaddleStroke commented May 20, 2024

Doing a plain revert as it is done in this PR is not necessary. If you want to revert you can just modify the toolbar instead of removing the command group and all. All you need is

-    geom << "Sketcher_CreatePoint";
-    SketcherAddWorkspaceLines(geom);
+    geom << "Sketcher_CreatePoint"
+         << "Sketcher_CreateLine";
    SketcherAddWorkspaceArcs(geom);
+    geom << "Separator"
         << "Sketcher_CreatePolyline";

As said before I am not against the reversal for now as the polyline tool has apparantly some way to go before being good enough to warrant the removal of the line tool.

However I will say that the position of those in favor of reversal are very conservative, anti-change and not constructive. Because they fail to see that there is value in the proposal of merging these tools : It promotes the use of polyline which reduce the probability of open wires.

The best course of action imo is to improve the polyline tool to solve the annoying case described in the video. The tangent mode after an arc is not necessary. It could just be an auto-constraint when close enough to tangent.

@berberic2
Copy link
Contributor Author

However I will say that the position of those in favor of reversal are very conservative, anti-change and not constructive. Because they fail to see that there is value in the proposal of merging these tools : It promotes the use of polyline which reduce the probability of open wires.

No one has said something like this – ever. Read the comments.

The problem is not the merging of the tools, the problem is that there is no progress on the tools at all, only the UI has changed with a degregation in usability.

Just a quote:

So, at the moment it is a degredation. The UI-change should be made when the polyline/line-improvement has happend.

@PaddleStroke
Copy link
Contributor

PaddleStroke commented May 20, 2024

Indeed I stand corrected then. I read in diagonal and stopped at openBrain comment

Guys, really looks like you're not using FC at all... 😕 The original PR is a PITA for one frequently using both Line and Polyline tools for what they each are targeting. As it is now, it is like if Line and Arc would have been merged in same icon. This is a major user experience crap. Is it what DWG is aiming at ?

But he indeed says 'As it is now'

@0penBrain
Copy link
Contributor

However I will say that the position of those in favor of reversal are very conservative, anti-change and not constructive.

Surely not. Even absolutely opposite. This is probably the most essential rule of the merging process. Being easy to revert is what allows to easier merge PR too. The process is fully lamous if not all rules apply at same level. 😉

@MisterMakerNL
Copy link
Contributor

As a long time CAD professional I really like the change, I never use normal line and really like how it works now.

@berberic2
Copy link
Contributor Author

As a long time CAD professional I really like the change, I never use normal line and really like how it works now.

What change?! There is absolutely no change in the way the tools work, yet.
The only change is in the UI and for the ones that only use one of the tools, the UI-change it has no impact at all. It’s an unnecessary usability-degredation for people using both tools.

@PaddleStroke
Copy link
Contributor

PaddleStroke commented May 21, 2024

@berberic2 Instead of this reversal (which again I'm not against at this time), I can offer the following :

  • Add a tangent autoconstraint detection for the 'free' line mode. ie when close enough from tangent (after an arc), a tangent auto-constraint is offered.
  • After an arc: change the mode from the tangent line to the free line mode.

This would solve the situation described in your video.

@MisterMakerNL
Copy link
Contributor

MisterMakerNL commented May 21, 2024

As a long time CAD professional I really like the change, I never use normal line and really like how it works now.

What change?! There is absolutely no change in the way the tools work, yet. The only change is in the UI and for the ones that only use one of the tools, the UI-change it has no impact at all. It’s an unnecessary usability-degredation for people using both tools.

You can just exit the feature if you want a single line. You just have to get used to it.
Further more it's a 3D CAD program I see no real use case for single lines except split lines and center-lines but you hardly need those. Everything needs closed loops which means a minimum of 3 straight lines. I'm not sure what all these users that need this do in Freecad with single lines?

@wwmayer
Copy link
Contributor

wwmayer commented May 21, 2024

You can just exit the feature if you want a single line. You just have to get used to it.

I wonder whether you have ever watched the video. This discussion is not about to quit the polyline command but about the extra work that must be done because of automatically added but unwanted constraints.

Everything needs closed loops which means a minimum of 3 straight lines. I'm not sure what all these users that need this do in Freecad with single lines?

A single line + a semi-circle.

@0penBrain
Copy link
Contributor

@berberic2 Instead of this reversal (which again I'm not against at this time), I can offer the following :

Your proposals seems OK but:

  • We aren't sure we catch every drawback of the change
  • We are depending on the delay it will take

To me, the sensible decision is to revert the PR (or at least its effects in the toolbar, as you proposed earlier).
Then a global discussion about the Polyline shall happen to see how it could make Line unnecessary in a vast majority of cases.
Probably Polyline could also take opportunity of Tool Setting to better indicate its current working mode.
Gonna say it again: I have no issue with easily merging things so users can test faster. But everyone shall be aware that sometimes it will fail and we have to revert easily to. This doesn't necessarily mean a complete withdrawal of the change, that may be submitted again later with appropriate modifications. But not being able to revert promptly is shooting in the legs of the contribution process.

@FEA-eng
Copy link
Contributor

FEA-eng commented May 27, 2024

It seems that this PR can be closed.

@chennes
Copy link
Member

chennes commented May 27, 2024

Superseded by #14299

@chennes chennes closed this May 27, 2024
@berberic2 berberic2 deleted the revert-linetool branch May 28, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB Sketcher Related to the Sketcher Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants