FreeCAD_SheetMetal icon indicating copy to clipboard operation
FreeCAD_SheetMetal copied to clipboard

Bend: Invalid subelement name (due to Topoaming)

Open JaapStruyk opened this issue 1 year ago • 34 comments

most of the workbench not working due to mallformed names of subelement, subelement has a name like: ;g543;SKT;:Hc32,E;:G;XTR;:7,F.Face3 when selecting Face3

OS: Ubuntu 24.04 LTS (ubuntu:GNOME/ubuntu) Word size of FreeCAD: 64-bit Version: 0.22.0dev.37543 (Git) AppImage Build type: Release Branch: main Hash: 3eb45b045cdda9d5bfe11baba2567a1df90f064c Python 3.11.9, Qt 5.15.13, Coin 4.0.2, Vtk 9.2.6, OCC 7.7.2 Locale: Dutch/Netherlands (nl_NL) Installed mods:

  • fcgear
  • fasteners
  • dxf-library
  • CfdOF 1.25.16
  • sheetmetal 0.4.15
  • Curves 0.3.0

JaapStruyk avatar May 30 '24 14:05 JaapStruyk

Is this freecad version with the new topological naming enabled?

shaise avatar May 30 '24 21:05 shaise

No idea, stock appimage from weekly builds so presume not, btw starting with a sheetmetal base shape works fine, just found out then when saving, closing and reopening the drawing it's working as it should so think it's FreeCAD related not the sheetmetal workbench.

JaapStruyk avatar May 31 '24 06:05 JaapStruyk

The ;g543;SKT;:Hc32,E;:G;XTR;:7,F.Face3 instead of just Face3 hints that the TNP is already enabled. I never tested it with TNP. I will investigate. Thanks!

shaise avatar May 31 '24 07:05 shaise

It looks that TNP is used while working but gets lost when saving/reopening, on re-edit it's TNP again. Looks like a freecad bug in this release but does show a tiny bottleneck with sheetmetal if TNP is applied defenatly. Thanks for helping!

JaapStruyk avatar May 31 '24 11:05 JaapStruyk

I will investigate if sheetmetal might collide with TNP.

shaise avatar May 31 '24 12:05 shaise

@JaapStruyk , I have added some code to bypass the TNP issue. please update and test

shaise avatar Jun 01 '24 06:06 shaise

Looks like you nailed it ;-) tried a couple of things but couldn't find trouble, thanks for the quick responce.

JaapStruyk avatar Jun 01 '24 10:06 JaapStruyk

Hello, Unfortunately, after the TNP problem was fixed, the models of sheet metal parts stopped working correctly. Errors appear after saving and reopening files.

FreeCAD info (latest weekly-build):

OS: Ubuntu 22.04.4 LTS (XFCE/xfce)
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.37668 (Git) AppImage
Build type: Release
Branch: main
Hash: ea68c5e88cdc80044ede482f6ad908820f4c1c82
Python 3.11.9, Qt 5.15.13, Coin 4.0.2, Vtk 9.2.6, OCC 7.7.2
Locale: English/United States (en_US)
Installed mods: 
  * Assembly4 0.50.13
  * Plot 2022.4.17
  * sheetmetal 0.4.17
  * addFC 0.3.1
  * CfdOF 1.25.17
  * fasteners 0.5.21

Report:

09:59:47  <PropertyLinks> PropertyLinks.cpp(453): Error#Bend.baseObject missing element reference Error#BaseBend ;g5v2;SKT;:Had6,V;:G;OFS;:Had6:7,V;:G;OFS;:Had6:7,V;WIR;:Had6:4,V;:G;XTR;:Had6:7,E;:H,E.Edge2
09:59:47  <PropertyLinks> PropertyLinks.cpp(453): Error#Bend.baseObject missing element reference Error#BaseBend ;g3v1;SKT;:Had6,V;:G;OFS;:Had6:7,V;:G;OFS;:Had6:7,V;WIR;:Had6:4,V;:G;XTR;:Had6:7,E;:G;OFS;:Had6:7,E;:H,E.Edge40
09:59:47  pyException: Traceback (most recent call last):
  File "/home/***/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py", line 1551, in execute
    thk, thkDir = sheet_thk(Main_Object, face[0])
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/***/.local/share/FreeCAD/Mod/sheetmetal/./SheetMetalCmd.py", line 437, in sheet_thk
    selItem = MainObject.getElement(SheetMetalBaseCmd.getElementFromTNP(selFaceName))
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
<class 'ValueError'>: Invalid subelement name
09:59:47  Bend: Invalid subelement name

https://github.com/shaise/FreeCAD_SheetMetal/assets/25671299/07c01fa3-5572-440e-a4c3-7b846401d707

Error.zip

GS90 avatar Jun 08 '24 07:06 GS90

ok. I will investigate further more.

shaise avatar Jun 08 '24 08:06 shaise

@GS90 , Thank you for the catch. This is indeed another TNP use case. Please check my fix.

I still think what I do is a workaround. I think .getElement should support TNP, perhaps @bgbsww can way in.

shaise avatar Jun 08 '24 09:06 shaise

So the long subelement name is the correct full history name of the element, and as you can see, the tail portion after the dot is the traditional short name. Can you give a specific pointer to where in the FC API you are getting these names, so that I can confirm whether or not they should be visible at that point? My expectation is they would not be and you would just deal with the short name.

bgbsww avatar Jun 08 '24 10:06 bgbsww

Hi @bgbsww , Thank you for your reply. Here is how it goes: A sheetmetal feature holds the selected faces it was built upon like this

        obj.addProperty(
            "App::PropertyLinkSub", "baseObject", "Parameters", _tip_
        ).baseObject = (selobj.Object, selobj.SubElementNames)

the SubElementNames is a list of faces / edges returned from the getSelectionEx command which return plain names. however when I read back the baseObject property it returns the full TNP name.

For now, as a workaround, I just clean the TNP name. Testing the same on FreeCad Link version, baseObject return the simple name, as expected.

shai

shaise avatar Jun 08 '24 10:06 shaise

@GS90 , Thanks, I'll take a look. Please make a new issue for it. and delete the message from here.

shaise avatar Jun 08 '24 11:06 shaise

Hi @bgbsww ,

Attached is an example I can not make any workarounds since this time the error is in C++. load the file, select Bend feature and change the length to say 15. This is the error:

15:04:41  <Exception> FeatureDressUp.cpp(209): Invalid edge link: ;Edge60;D9d4;:H,E.Edge70
15:04:41  Fillet: Invalid edge link: ;Edge60;D9d4;:H,E.Edge70

Test-SM-TNP.zip

shaise avatar Jun 08 '24 12:06 shaise

Thank you! The reference on PropertyLinkSub was super helpful; there are no differences at the python layer, and no differences in the C++ implementation of PropertyLinkSub.

However, it appears that the ComplexGeoData::findElementName which is now Data::findElementName is missing this suspiciously germane clause at the top:

    // skip leading dots
    while(subname && subname[0] == '.')
        ++subname;

I need to keep running this down, but this smells right.

bgbsww avatar Jun 08 '24 13:06 bgbsww

Thanks for all your hard work on TNP!

shaise avatar Jun 08 '24 13:06 shaise

With that code in place, your example file fails until I replace Edge70 (one of the fillets), because it has a bad reference, but once that's done it appears to work correctly.

I need to run all the test suites and submit a PR, expect it to be in place soon, and hopefully you won't need the workarounds at that point.

bgbsww avatar Jun 08 '24 13:06 bgbsww

Thanks again!

shaise avatar Jun 08 '24 13:06 shaise

Hi @bgbsww, thanks for your heroic work on TNP. Has there been any progress on this specific issue ?

luzpaz avatar Sep 10 '24 09:09 luzpaz

@luzpaz , I think this can be closed as my attached test above now works. perhaps only thing needed is to comment out some warning messages:

14:39:44  <App> PropertyTopoShape.cpp(394): Pending recompute for generating element map: Test_SM_TNP#BaseShape
14:39:44  <App> PropertyTopoShape.cpp(394): Pending recompute for generating element map: Test_SM_TNP#Body
14:39:44  <App> PropertyTopoShape.cpp(394): Pending recompute for generating element map: Test_SM_TNP#Bend
14:39:44  <App> PropertyTopoShape.cpp(394): Pending recompute for generating element map: Test_SM_TNP#Fillet
14:39:44  <App> PropertyTopoShape.cpp(394): Pending recompute for generating element map: Test_SM_TNP#Fillet
14:39:52  <PropertyLinks> PropertyLinks.cpp(429): Test_SM_TNP#Bend.baseObject auto change element reference Test_SM_TNP#BaseShape ;Edge24;D23d2;:H,E.Edge34 -> ;Edge24;D1866;:H,E.Edge34
14:39:52  <PropertyLinks> PropertyLinks.cpp(429): Test_SM_TNP#Fillet.Base auto change element reference Test_SM_TNP#Bend ;Edge60;D9d4;:H,E.Edge70 -> ;Edge60;D913;:H:5,E;:H,E.Edge70

shaise avatar Sep 10 '24 11:09 shaise

maybe you can close this in the PR that comments out the warning messages ?

luzpaz avatar Sep 10 '24 14:09 luzpaz

I think this is for @bgbsww to decide. perhaps its on purpose. @JaapStruyk can you check if your issue is fixed?

shaise avatar Sep 10 '24 15:09 shaise

The Pending Recompute warning should stay until we add a UI that tells people they've opened a file without an element map and it should be recomputed. In places like this one it is unnecessary.

I've been avoiding that PR since we are in feature freeze, but getting increasing pressure to write it and request special dispensation.

The auto change warning should get set up to show only at the very highest debugging levels - ordinary users shouldn't have to see it. That could be a simple PR that can also close this, presuming the issue itself is fixed.

bgbsww avatar Sep 10 '24 15:09 bgbsww

Hi @shaise, thank you for your work.

This problem almost eliminates the possibility of using the workbench after fixing TNP... After changing the base geometry "fillet", "chamfer", external geometry, they all give an error if they are located after sheet metal elements.

Is there hope that someday the problem will be solved?

GS90 avatar Dec 08 '24 18:12 GS90

@GS90, I do not know how much TNP is accessible to external workbenches. I'm now in a process to clean the WB and make it more user friendly. This will take some time. After that I will take a look and see if TNP can be used.

shaise avatar Dec 08 '24 19:12 shaise

Okay, got it. But I'll still leave here a more detailed description of the tragedy to date. This really interferes with work.

22:37:57 <PropertyLinks> PropertyLinks.cpp(447): sm#Chamfer.Base missing element reference sm#BaseShape ;Edge47;D522;:H,E.Edge85 22:37:57 <PropertyLinks> PropertyLinks.cpp(447): sm#Chamfer.Base missing element reference sm#BaseShape ;Edge33;D22db;:H,E.Edge63 22:37:57 <Exception> FeatureDressUp.cpp(209): Invalid edge link: ;Edge47;D522;:H,E.Edge85 22:37:57 Fillet: Invalid edge link: ;Edge52;De5a;:H:5,E;:H,E.Edge62 22:37:57 Chamfer: Invalid edge link: ;Edge47;D522;:H,E.Edge85

Info: OS: Ubuntu 22.04.5 LTS (ubuntu:GNOME/ubuntu/xcb) Architecture: x86_64 Version: 1.0.0.39109 (Git) Conda AppImage Build type: Release Branch: (HEAD detached at 1.0.0) Hash: 2fcc5317fe3aee96ca73475986a577719fc78e20 Python 3.11.9, Qt 5.15.13, Coin 4.0.3, Vtk 9.2.6, OCC 7.7.2 Locale: English/United States (en_US) Stylesheet/Theme/QtStyle: unset/FreeCAD Classic/Qt default Installed mods:

  • fasteners 0.5.33
  • Assembly4 0.50.15
  • Plot 2022.4.17
  • CfdOF 1.29.1
  • sheetmetal 0.6.1
  • addFC 0.7.5

https://github.com/user-attachments/assets/4ee1ea93-0aa8-4754-8a71-ce4a761fee9e

sm.zip

GS90 avatar Dec 08 '24 20:12 GS90

@shaise make sure faces for extrudes are made with makeFace, and etc.

i can make a pr to help fix this if you would like

drwho495 avatar Jul 19 '25 19:07 drwho495

@drwho495 , Thanks! Finally, I can understand why things are not working. I think this should be well noted in the "how to make an external WB" wiki. Can you give a small example, preferably from sheetmetal code, of how to use makeFace instead of Face, I can then take it from there.

Thanks!!!

shaise avatar Jul 20 '25 05:07 shaise

@drwho495 , Thanks! Finally, I can understand why things are not working. I think this should be well noted in the "how to make an external WB" wiki. Can you give a small example, preferably from sheetmetal code, of how to use makeFace instead of Face, I can then take it from there.

Thanks!!!

So I looked at the code for extend and it should generate element maps properly, but it isn't. The code looked really complicated for a simple operation. How is it supposed to work?

drwho495 avatar Jul 20 '25 17:07 drwho495

How is it supposed to work?

Do you mean what is the algorithm that extend a face?

I always suspected the problem is with the "App::PropertyLinkSub". This property holds the base object and the elements (Edge1, Face2, etc) on which the extend function should work on. However "Edge1" and "Face2" are not TNP names. this means that whenever the base object changes, the internal system should forcefully change the element names in order for it to work. I do not know if this is the case. By the way, on realthunder's FreeCad version, everything works fine. no TNP issues with SheetMetal,

shaise avatar Jul 20 '25 19:07 shaise