Refactoring Unfolder

[sliptonic]

I've been looking at sheetmetal in general. This addon is quite good. I ran some code analysis tools like Wily and Radon. Overall the code quality is good. The notable exception is the unfolder. It has a couple of problems that make it a good candidate for refactoring.

First, it has a high cyclomatic complexity. More than 60 in some places.

It co-mingles the GUI and unfold logic. That makes it impossible to run the unfolder from a macro or external script https://github.com/shaise/FreeCAD_SheetMetal/issues/227 It also makes it difficult or impossible to write unit tests for the core unfold logic. Those unittests are essential if we want to refactor the unfold logic or improve performance.

It has a deep assumption about materials being defined in spreadsheets. This will make it hard to take advantage of the reworked materials system when it's available. https://forum.freecad.org/viewtopic.php?t=78242&hilit=materials

The GUI is designed in QtCreator but not used in .ui form. Instead it has been exported with pyuic and the resulting python has been modified. This makes it harder for a designer to improve the UI.

The unfolder produces a solid model and, optionally, a sketch of the 2D sheet. These objects aren't parametric so if the user wants to change anything they have to be deleted and the process recreated.

The resulting file, SheetMetalUnfolder.py is thousands of lines long and difficult to improve.

I'm suggesting the following changes. I've started implementing them in this PR.

• Move the GUI entirely out to a separate module. Make the TaskPanel class just handle the setting/getting data and the control interaction. Make the TaskPanel load the .ui file natively.
• Move the GUI commands out to separate modules.
• Create a separate class to handle interpreting a spreadsheet to generate a kfactor table.
• Turn the output geometry into Python Feature objects so they can be parametric.
• Minimally refactor the core unfolder to support the above changes. Not a deep refactor at this time.

[sliptonic]

There's a few things about the UI that I don't understand or that I think can be improved. There's a checkbox for 'use MDS' and a combobox that lists the MDS spreadsheets. There's also a checkbox next to manual kfactor.

This results in a lot of internal logic to check if the right combination of checkboxes and combo selection is made. It also adds logic to enable and disable various controls based on the state. It seems like this can be greatly simplified.

Remove the 'use MDS' checkbox entirely. Rmove the 'manual k-factor' checkbox.
Make the first option in the combobox = 'none'. If 'none' is selected, use the manual k-factor, otherwise use the k-factor derived from the sheet.

It also doesn't appear that the 'apply' button does anything. I'd remove it as well.

[shaise]

Hi @sliptonic ,

Yes, this is the result of quite a hot debate around one of the PRs. While 'Professional" users said that an MDS must be used without any manual K factor option, many hobbyists (including me) disliked the complexity of MDS for most cases. So basically its a patch over patch and I agree with you it should be as you've suggested. I see you made lots of important changes. Thanks! The reason the unfolder is so mixed up is because I tried to use @ulrich1a 's unfolder macro with as few changes as possible, so I can easily update it whenever the macro is updated. However since this macro is long left unchanged its a good time to refactor it. Tomorrow I will take a look at these changes and merge them.

shai

[sliptonic]

I'm no where near merging. I just started the PR in draft to start the conversation. I'll keep updating my branch so you can see what I'm working on.

[sliptonic]

Yes, this is the result of quite a hot debate around one of the PRs. While 'Professional" users said that an MDS must be used without any manual K factor option, many hobbyists (including me) disliked the complexity of MDS for most cases. So basically its a patch over patch and I agree with you it should be as you've suggested. I see you made lots of important changes. Thanks! The reason the unfolder is so mixed up is because I tried to use @ulrich1a 's unfolder macro with as few changes as possible, so I can easily update it whenever the macro is updated. However since this macro is long left unchanged its a good time to refactor it. Tomorrow I will take a look at these changes and merge them.

At least as far as unfolding goes, the engineering mode only controls whether the manual kfactor should be used.

It seems like we could make the The MDS combobox include: "None" Kfactor is not computed and an error is thrown. Default if engineering mode is enabled. "Manual K-factor" Use the manual setting. Default if eng mode is disabled.

Use the selected spreadsheet. This would force the user to make the explicit action of changing the combobox. It would be on them to review the manual kfactor setting at that time.

[shaise]

Yes, I think its a good solution. I was a bit reluctant regarding the "Engineering mode". I think this complicate things. There is no global "Engineering mode" for freecad, and It looks strange that only sheetmetal WB should have such mode. However since this option is off by default I'm ok with it.

[sliptonic]

Are you intentionally keeping backwards compatibility with Py 2.7.x? The rest of FreeCAD has dropped all support.

[shaise]

Not at all. If FreeCad 0.20 (official release) does not use 2.7 it can be safely removed

[sliptonic]

I've got the UI working the way I like now. Much simpler and easier to maintain. I'm about to start the dirty work of refactoring the rest of the unfolder.

Do you have any sample projects that I can base some tests on? I know there aren't proper unittests but known-good samples that I can use as a starting point would help.

[shaise]

Hi @sliptonic ,

I think I will go through the issues with all the fixes for the unfolder and collect the models. If it works on them it should be fine.

shai

[sliptonic]

So far, I've largely kept the unfold dialog close to the current version and tried to address the user intent. But the export dxf/svg thing is bugging me.
Maybe it's just a convenience thing but it feels like the dialog is doing too much. It's handling the unfold operation AND the export.

There are already export tools in the UI that let the user export dxf/svg and also choose a filename as well. The function here assumes a filename.

Is there a reason why this was done this way?

[sliptonic]

https://github.com/shaise/FreeCAD_SheetMetal/blob/master/SheetMetalUnfolder.py#L3373C1-L3388C88 This looks like unreachable code. We're in the Except block so nothing has been returned. error_cd == 1 should always return False.

Am I missing something?

[shaise]

So far, I've largely kept the unfold dialog close to the current version and tried to address the user intent. But the export dxf/svg thing is bugging me. Maybe it's just a convenience thing but it feels like the dialog is doing too much. It's handling the unfold operation AND the export.

There are already export tools in the UI that let the user export dxf/svg and also choose a filename as well. The function here assumes a filename.

Is there a reason why this was done this way?

As you can see I'm not a great maintainer... I accepted the unfold gui PR with all its changes. We need to see if the general export produce the exact same result as the export inside, and if so, this option might be redundant. Mind you, the TechDraw have also built in export for SVG and DXF so perhaps this was the reasoning behind it.

shai

[shaise]

https://github.com/shaise/FreeCAD_SheetMetal/blob/master/SheetMetalUnfolder.py#L3373C1-L3388C88 This looks like unreachable code. We're in the Except block so nothing has been returned. error_cd == 1 should always return False.

Am I missing something? Indeed it looks like an unreachable code. Perhaps the coder had some logic error, but I can't understand what was the original intention.

shai

[sliptonic]

The main functionality seems to work again. This is largely untested except for a couple files I have. It could certainly use some eyeball before merging

[ceremcem]

Hi @sliptonic . I'm the author of MDS, please do not touch it. We are using this addon in production and any tiny issue costs us lots of money, time and credibility. As you see, currently there is no solid way to perform unit tests, so I personally am afraid of touching the legacy code. Please see this: https://github.com/shaise/FreeCAD_SheetMetal/pull/70

I appreciate your set of goals but please do not break our products :)

[sliptonic]

@ceremcem I don't think that's exactly a reasonable request. How can things ever improve if we refuse to iterate and change things? There has to be a path to improvement. The current version can be tagged and you can continue to run the existing version until the next version is stable.

Part of the reason that I'm doing this refactoring is exactly to allow for unit tests to be written. The current code is not only untested, it's untestable. There's far too much mixing of concerns poor abstraction.

Can you tell me more about how your production process depends on MDS? Do you mean you're dependent on the resulting data or that you depend on the UI working a certain way?

[ceremcem]

MDS defines real materials' K-factor data that are used in our products. I'm using Assembly3 (Linkstage3) branch, so I'm able to unfold on demand, whenever my underlying model changes. So, if MDS is removed, then my model can't update the unfold outputs.

I don't think that's exactly a reasonable request.

I'm only telling that the features exist for a reason.

How can things ever improve if we refuse to iterate and change things?

Correct, I don't refuse the change. In fact, it's a must. Thank you for sparing your time.

The current version can be tagged and you can continue to run the existing version until the next version is stable.

Yes, but this means I'll be unable to receive any other possible updates. Anyway, it's up to you now.

Thanks.

[sliptonic]

I tried to avoid touching the MDS feature as it exists now. I abstracted the UI code out so the UI and the underlying logic can be maintained separately. I also made some minor changes to the UI to make it more user-friendly. The logic of the underlying algorithm is untouched.

As I understood it, the engineering mode was intended to force the user to make a choice for k-factor rather than accepting default values. This is kind of weird and there's nothing like it anywhere else in FreeCAD. That said, I understand the use-case. In the proposed implementation, it achieves the same goal in a slightly different way.

If engineering mode is on, the first value in the MDS list is 'None' and it is selected by default in the panel. The unfolding won't continue if the value is 'None' so the user has to choose something. The second value in the list is 'manual'. If selected, the manual kfactor is used. After that, the list is populated with any valid spreadsheets in the document.

If Engineering mode is off, the MDS list is populated the same way except the 'None' value is omitted and the default value is set to 'manual'. The user can still make a change but for casual users, the dialog produces valid output with fewer mouse clicks.

The MDS lookup has been abstracted out to a separate module. This is anticipating the work Dave Carter is doing for the new materials subsystem. When it's ready, the MDS lookup module can be extended to use either the legacy spreadsheet or the new material cards. The UI and the algorithm shouldn't need to be touched.

[sliptonic]

Any one have a chance to test this?

[ceremcem]

I'm currently not available, sorry. I'm willing to test whenever possible.

On Fri, Jul 21, 2023, 19:37 sliptonic @.***> wrote:

Any one have a chance to test this?

— Reply to this email directly, view it on GitHub https://github.com/shaise/FreeCAD_SheetMetal/pull/280#issuecomment-1645965064, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSVCAUMZQV3G6FIZLEWFR3XRKV37ANCNFSM6AAAAAAZ56CAKI . You are receiving this because you were mentioned.Message ID: @.***>

[sliptonic]

Any chance this can get merged or reviewed?

[shaise]

Hi @sliptonic , I think @ceremcem is not available to test it now. If its OK I can do my own review and merge it this weekend.

[sliptonic]

No huge hurry. Just didn't want it to fall into the void

[shaise]

I was about to do it long ago, I just thought you were waiting for @ceremcem .

[shaise]

Hi @sliptonic Thanks a lot! looks like a huge amount of work. I have tested the unfolder on several test models and it seems to work fine.

[ceremcem]

Wow! I've missed a lot :( Sorry.