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

Add face reference modes (for angle and offset) to AddWall command #467

Merged
merged 35 commits into from
Feb 10, 2025

Conversation

sheetmetalman
Copy link
Contributor

Closes issue #307 .

Including new icons and additions in the taskpanel of the said command.

@shaise
Copy link
Owner

shaise commented Feb 4, 2025

Thank you for your great work!
I will do some tests and merge.

@sheetmetalman
Copy link
Contributor Author

You're welcome!

@karliss
Copy link

karliss commented Feb 5, 2025

Tried running the code from this PR and got missing icons and a bunch of errors like this:

14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2298: SyntaxWarning: invalid escape sequence '\F'
  selFaceIcon = icons_path + "\Face-selection.svg"
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2313: SyntaxWarning: invalid escape sequence '\I'
  iconRevWall = icons_path + "\Invert.svg" # Icon path
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2318: SyntaxWarning: invalid escape sequence '\S'
  iconUnfWall = icons_path + "\SheetMetal_Unfold.svg" # Icon path
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2323: SyntaxWarning: invalid escape sequence '\S'
  iconLenLeg = icons_path + "\SheetMetal_WallLenLeg.svg" # Icon path
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2324: SyntaxWarning: invalid escape sequence '\S'
  iconLenOut = icons_path + "\SheetMetal_WallLenOut.svg"
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2325: SyntaxWarning: invalid escape sequence '\S'
  iconLenInn = icons_path + "\SheetMetal_WallLenInn.svg"
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2326: SyntaxWarning: invalid escape sequence '\S'
  iconLenTang = icons_path + "\SheetMetal_WallLenTang.svg"
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2333: SyntaxWarning: invalid escape sequence '\S'
  iconPosMatOut = icons_path + "\SheetMetal_WallPosMatOut.svg" # Icon path
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2334: SyntaxWarning: invalid escape sequence '\S'
  iconPosIns = icons_path + "\SheetMetal_WallPosMatIns.svg"
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2335: SyntaxWarning: invalid escape sequence '\S'
  iconPosThkOut = icons_path + "\SheetMetal_WallPosThkOut.svg"
14:18:39  /home/karlis/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py:2336: SyntaxWarning: invalid escape sequence '\S'
  iconPosOffset = icons_path + "\SheetMetal_WallPosOffset.svg"

I am on Linux, with Python 3.13.1 and Freecad 1.0. Switching to / fixed it on my system not sure how well it would work on Windows. Windows and many libs has accepted '/' for a while, so it might be fine. If that's not an option will have to use appropriate python api for concatenating paths.

Love the look of new icons, they make each mode much easier to understand. Good job!

@karliss
Copy link

karliss commented Feb 5, 2025

Played with the feature a little bit, here are my comments from usability perspective:

Perfect touching, which the face mode makes very easy, often cause unfold to partially fail (touching parts not included in unfold). This somewhat limits the usefulness of the new feature. You end up using offset(plane)+offset mode to introduce gap in which case you might as well skip face reference and eyball it. Unfolding improvements are out of scope for this issue. It felt like whether it fails depended on which face was on top so maybe it's fixable. But I wouldn't be surprised if OCCT complained about such geometry even with unfolding fix.
One more thing to consider is making offset additive on top of other modes, to make it easier introducing a gap.

After selecting bend angle face, preview doesn't immediately refresh. I have to change back and forth one of the other settings like relative angle or wallpos to make it refresh.

I found the the double face selection workflow a bit clunky. Even though I understand the reasoning behind it, half the time after I selected face for bend angle and saw that "wall pos" is set to "material outside" or "material inside" inside but actual position is nowhere close it made stop for a few seconds until I remember that I need to set mode to "offset" select face again, and then select "material inside" in second dropdown. Not sure how to be better solve this. I somewhat like the idea of wall position face choice being separate from angle choice. I guess it might depend on the situation where you use this feature. If you are referencing some global datum plane, I guess it makes sense to use only angle, but for the types of bends I tried to make relative to other faces of shape most of the time I also wanted to use it for wall pos. Does it even make sense to use to use 2 different faces here instead of single face for both or single face only for angle? I guess the double selection might be more more useful if second reference feature could be a vertice not just face.
Potential approaches:

  • Simplify UI so that there is single face selection, with face relative wall pos being activated by special wallpos mode or checkbox available only when angle uses face.
  • Keep UI as is, but after selecting face for angle automatically assign it for wall pos as well. User can still switch it if they want wall position to be based on previous segment end instead of reference face.
  • Flatten wall position choice so that there aren't two levels of it

Is there any way to change the reference face without toggling face relative angle/offset off and back on?

Face relative angle worked even when selecting multiple edges for symmetrical bends. I was able to make hexagonal box with overlapping flaps using only 2 bend operations and no additional Sheetmetal or PD operations. No idea how, but it worked. Does it do the face relative calculations only for the first edge in list? Since it's not obvious which bend will be used as reference, it was easier to set it up for single edge first and then add rest of them afterwards.

@sheetmetalman
Copy link
Contributor Author

sheetmetalman commented Feb 6, 2025

@karliss Thanks for the catch!

Tried running the code from this PR and got missing icons and a bunch of errors like this:

@shaise I think is needed to fix these errors before merging.

@karliss I glad you like the icons!

I think is very good to have someone testing on another OS. SheetMetal WB need it. Thanks! 👍
I also take a little look at your PR #452 . I'm happy to see more people helping with SheetMetal WB. The FreeCAD community need it! Thanks! Keep going! 👍 💪

_

Perfect touching, which the face mode makes very easy, often cause unfold to partially fail (touching parts not included in unfold). This somewhat limits the usefulness of the new feature. You end up using offset(plane)+offset mode to introduce gap in which case you might as well skip face reference and eyball it. Unfolding improvements are out of scope for this issue. It felt like whether it fails depended on which face was on top so maybe it's fixable. But I wouldn't be surprised if OCCT complained about such geometry even with unfolding fix.
One more thing to consider is making offset additive on top of other modes, to make it easier introducing a gap.

This happens even if the user has set a value for the "angle" manually. The code works like a user who inserts a value in the "angle" property.
The new tools don't really create new thing in the AddWall command. It's only make easy to define the value for the "angle" and for the "offset" (when it's "on" of course).
In fact is this what happen in the code: it applies a calculated value in the "angle" and in the "offset". This value is the result of the calculation based on the reference face.

_

After selecting bend angle face, preview doesn't immediately refresh. I have to change back and forth one of the other settings like relative angle or wallpos to make it refresh.

If you change the angle mannualy, happens the same: the model only updates on the second change of the angle. If you change the wall length it updates too. I try fix it on the past, but I couldn't. I'll try again when I fix that thing about the icons path.

_

I found the the double face selection workflow a bit clunky. Even though I understand the reasoning behind it, half the time after I selected face for bend angle and saw that "wall pos" is set to "material outside" or "material inside" inside but actual position is nowhere close it made stop for a few seconds until I remember that I need to set mode to "offset" select face again, and then select "material inside" in second dropdown

If the user select a face for "angle" reference, it will works only for angle. The wall position is relative to the selected edge on the sheet metal part, not the angle face reference. The wall position need to be defined even if the angle is manually inserted value.
The offset position (which has the same names and icons) is relative to the offset face reference.

_

I somewhat like the idea of wall position face choice being separate from angle choice. I guess it might depend on the situation where you use this feature. If you are referencing some global datum plane, I guess it makes sense to use only angle, but for the types of bends I tried to make relative to other faces of shape most of the time I also wanted to use it for wall pos. Does it even make sense to use to use 2 different faces here instead of single face for both or single face only for angle? I guess the double selection might be more more useful if second reference feature could be a vertice not just face.

I agree about the offset reference, maybe will be nice if the user can select another type of geometry as reference to it (like vertex).
The idea is to make life easier for the user by inserting angle and offset values when it's easier for him use a geometry for reference. Before this the user would need to calculated himself the value for the angle or for offset and insert it manually (and that would not be parametric). With these new functions, the value will be calculated automatically and will still be parametric.
Another fields can has this type of mode (where the user can insert a value or use a geometry as reference), for exemple maybe in the future will be added something like this to the length value.

Just a little trick: as asked in the said issue, was inserted too a new "shortcut" type in the code. If the user select the face reference (together with the edges selections) before click on the AddWall command it will use this face for angle and offset references.

_

Potential approaches:
Simplify UI so that there is single face selection, with face relative wall pos being activated by special wallpos mode or checkbox available only when angle uses face.
Keep UI as is, but after selecting face for angle automatically assign it for wall pos as well. User can still switch it if they want wall position to be based on previous segment end instead of reference face.
Flatten wall position choice so that there aren't two levels of it

Well, I think something like that was discussed on the refereced issue #307 , but the conclusion for that to make the current UI in this PR. I think is better to keep this taskpanel for this moment. Maybe in the future a new UI can be discussed and change it again.
If you want, you can design a proposal, a draft, about the taskpanel on the way you want it. Perhaps with QtCreator Designer or just a drawing. Maybe community will like it and soon a new UI is there.

_

Is there any way to change the reference face without toggling face relative angle/offset off and back on?

I was thinking about that. I think it can be done if instead of a LineEdit field for the face references properties we use a button. But then I would get into another long debate about UI and consistency across FreeCAD (consistent user experience and similar interfaces for similar tasks). And I have enough about this on this issue. I will let this for my next issue. 😅

_

Face relative angle worked even when selecting multiple edges for symmetrical bends. I was able to make hexagonal box with overlapping flaps using only 2 bend operations and no additional Sheetmetal or PD operations. No idea how, but it worked. Does it do the face relative calculations only for the first edge in list? Since it's not obvious which bend will be used as reference, it was easier to set it up for single edge first and then add rest of them afterwards.

Yes, the calculations for "angle" and "offset" use the first edge in the list.

_
Now specifically about that part at the beginning.

Perfect touching, which the face mode makes very easy, often cause unfold to partially fail (touching parts not included in unfold).

I tried this here on a elevator part project, and unfold works well. I will attach a file where I used this. But I agree we will need some sort of corner gap tool on the future. Can you see it unfold well on Linux? elevatorPart.zip *PS.: I used the new unfolder code.

_
Summarizing the actions I will take:

  • Correct the icon directory string;
  • And try again to fix this delay in updating the model when the angle value is changed in the taskpanel.

@karliss
Copy link

karliss commented Feb 6, 2025

Elevator part unfolds fine when I select bottom surface surface. It fails with "FreeCAD exception thrown (Wire is not closed.)" when selecting bottom surface.

The kind of failure I was talking about happens when you have overlapping flaps like this:
image
image

I haven't looked at how the unfolder works, but I am guessing it functions by traversing one side of the part. That would match with the observation of failure place being dependent on overlap order and which face was selected as starting point for unfolding. Hard to unfold based on faces which don't exist. This somewhat ruins my hopes that there might be an easy fix in unfolder.

@sheetmetalman
Copy link
Contributor Author

sheetmetalman commented Feb 6, 2025

Thanks for test! 👍

Got it!

This is why there's a "offset position" for face reference. Sometimes is better to has some clearance between the reference face and the wall (even if so small that it can be considered negligible of manufactoring perspective). And the good thing here is: this clearance is parametric (even if some length is changed the clearance size will be the same). By the way, the distance of the offset for "offset position" is measured perpendicular to the face reference.

I haven't looked much at the new unfolder code either. I'm looking to help with other things on SheetMetalWB. But of course, I'm very excited about the new possibilities of the new code and its speed!

* Fix the directory path for better compatibility with other OS;
* Fix model angle update when changed in taskpanel;
* Fix angle and offset form fields when using face reference modes.
@sheetmetalman
Copy link
Contributor Author

sheetmetalman commented Feb 6, 2025

Done!

  • Fixed the directory path for better compatibility with others OS;
  • Fixed model angle update when changed in taskpanel;
  • Fixed angle and offset form fields updates when using face reference modes.

@shaise
Copy link
Owner

shaise commented Feb 6, 2025

Love the look of new icons, they make each mode much easier to understand. Good job!

+1 !!

@shaise
Copy link
Owner

shaise commented Feb 6, 2025

I'm still testing but here are some initial remarks you can work with:

  1. I get a lot of errors when I try to open the UI file with my qt designer. What version do you use?
  2. Icons are missing:
    (why not use the icons directly in the ui files? much simpler. See other sheetmetal task panels using icons.)

image

  1. Unfold is a parameter and was initially a checkbox. There is no way to see now if the unfold is pressed or not because its a button now.
  2. if load old file and double click on a bend, the task panel is not opened because there is an error. (old object is missing properties). You need to activate the _addProperties function before updateForm:
    image

here is a sample file to try:

test-compat.zip

@sheetmetalman
Copy link
Contributor Author

sheetmetalman commented Feb 6, 2025

I'm still testing but here are some initial remarks you can work with:

  1. I get a lot of errors when I try to open the UI file with my qt designer. What version do you use?

QtCreator 15.0.1 (community)

  1. Icons are missing:
    (why not use the icons directly in the ui files? much simpler. See other sheetmetal task panels using icons.)

Thanks for the tip! I will look at that and use the icons through the UI files.

image

  1. Unfold is a parameter and was initially a checkbox. There is no way to see now if the unfold is pressed or not because its a button now.

It's set checked when is the unfold is on - like the face reference mode buttons.
But I made that as icon in the begining because I wanted to use less space. But after the UI changed, it don't need to be a button anymore, because use the same space. What do you preferr a button or the checkbox? I'll change, no problem.

  1. if load old file and double click on a bend, the task panel is not opened because there is an error. (old object is missing properties). You need to activate the _addProperties function before updateForm:
    image

Thanks! I added that fix in the code (but not in github yet - I will change the UI file before put all code into github).
Good to know this is need in the taskpanel part of the code too.

here is a sample file to try:

test-compat.zip

I test the file here and no problem when open the bend feature.

But get another error when opening the file:

20:47:35  Cannot create object 'Origin001': ('App::Point' is not a document object type)
20:47:35  <PropertyLinks> PropertyLinks.cpp(1013): Lost link to test_compat Origin001 while loading, maybe an object was not loaded correctly
20:47:35  Extension is not a python addable version: 'Gui::ViewProviderGeoFeatureGroupExtension'

Other fixes include:
* Add a line to make sure all properties ared added - very needed for backward compatibility;
* Simplification of some lines of taskpanel updatesForm function;
* Remove some unused code lines.
@sheetmetalman
Copy link
Contributor Author

Done!

  • Fixed that thing about backward compatibility;
  • Changed the file where the icons are set.

Waiting for the answer about the unfold option.
The reverse/invert bend side works on the same (set checked when it's on).
Here's a video about that:

buttonsUnfoldFlip01.mp4

@shaise
Copy link
Owner

shaise commented Feb 7, 2025

Thanks @sheetmetalman !
I will continue my tests tomorrow.
Regarding unfold, yes, make it a checkbox. thank you.

@sheetmetalman
Copy link
Contributor Author

Done!

@shaise
Copy link
Owner

shaise commented Feb 8, 2025

2'nd test remarks:

  1. Important:
    I still got empty icons like above. To fix it, I changed the button type from QToolButton, to QPushButton. (in designer right click on the button and "morph into" QPushButton.
    All other fixes worked. Thanks!

  2. Less critical, but more compatible with other tools:
    Right now if I already created a simple fold (without selecting a face) I do not have any option to select a face for it, only for the angle feature. It will be nice if you can make the fold up to a face after creating a simple fold (same like pad option), I would suggest moving the select face to be near the length parameter. something like this:
    image
    and when a face is selected, it will switch to "up to face" mode and make the length gray as it is not used now.
    We also need the opposite function: remove the "up to face" for angle and length. I suggest to do it by placing an X button to the right of the Reference fields.

@sheetmetalman
Copy link
Contributor Author

sheetmetalman commented Feb 8, 2025

  1. I can make the ToolButtons as PushButtons. No problem. But let me explain, please. I had made the buttons as ToolButtons because in the DarkTheme and LigthTheme the PushButtons have a minimal size and don't keep the size of the icon. Only ClassicTheme keep the PushButton as the icon size. At least at FreeCAD 1.0.0. Do the test.

In the below images I use the buttons as PushButtons in all themes. Notice the buttons.

image
image
image

  1. I agree it's a needed function and I will be glad to help with this on future. And the button position is ok to me too. But let me explain, that "shortcut" (selecting a face before click AddWall command) only use the face as reference for angle and offset, no for wall legth. Don't has a function to make the wall length UpToFace yet.
    Yes, indeed, the future function for wall length with geometry reference mode need to be like that (the length value greyed). But I think for offset and wall length geometry reference mode can have another geometry types as reference (not only just face, but vertex and edge) as suggested by @karliss.
    About turning off geometry reference mode, I think can works like the angle and offset today: only click the button again. Because when it's on the button is set as checked, when it's off no.

@sheetmetalman
Copy link
Contributor Author

@shaise The icons got empty in FreeCAD on all buttons? Can you send a image (with the buttons as ToolButtons)?

@shaise
Copy link
Owner

shaise commented Feb 8, 2025

  1. only the toolbuttons:
    EDIT: I'm using open theme light
    image

  2. Regarding up-to-face, Sorry it was my mistake, The length is a separate parameter. What I meant that if you make a simple bend, the up-to-geometry is now hidden and can not be used:

image

@shaise
Copy link
Owner

shaise commented Feb 8, 2025

Ah, and regarding the usage of the same button for select-geometry / remove-geometry-selection, I'm OK with that. But for better clarity, you can set the Normal-off Icon to the same icon you have now, and for the Normal-On add a small red X on the icon. This way the icon will toggle the image and the user can understand that pressing again deletes the selection.

@shaise
Copy link
Owner

shaise commented Feb 10, 2025

Also: to fix the button sizing for all icon button, and for all themes, add the following stylesheet to the button:

margin: 0px;
padding: 0px;
min-width: 28px;

(In designer, right click on the button -> Change style sheet)

This will make sure the icon button will work on all themes, and no matter if you use QToolButton or QPushButton.
FreeCAD Dark:
image

OpenTheme Light:
image

@sheetmetalman
Copy link
Contributor Author

@shaise Thanks for tip about the icons size! 👍 Exactly what I needed!

The buttons are fixed! 🥳

About the face selection mode for offset: it only appears if the wall position is set to "offset", otherwise the offset SpinBox (to manually enter the offset value) and the face selection mode button do not appear.

The icon for the "on" and "off" button mode is also done. I did it a little different.

Also fixed the HorizontalLine that did not appear in OpenTheme nor in the default FreeCADTheme Dark or Light.

Take a look:

task.panel.06.mp4

image

@shaise
Copy link
Owner

shaise commented Feb 10, 2025

Hey @sheetmetalman , this looks great!
Thank you for this wonderful feature, I know it took lots of work hours. Also the Icons adds a lot to understanding the types of bents, so thanks for that as well!

I have squashed the merge to a single change.

@shaise shaise merged commit 889cd40 into shaise:master Feb 10, 2025
@sheetmetalman
Copy link
Contributor Author

I glad you like it! 🤝
I'm happy to contribute to SheetMetalWB and FreeCAD!

shaise added a commit that referenced this pull request Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants