search

shaise / FreeCAD_SheetMetal

A simple sheet metal workbench for FreeCAD
http://theseger.com/projects/2015/06/sheet-metal-addon-for-freecad/
GNU Lesser General Public License v2.1
204 stars 58 forks source link

Bring in Ondsel changes #335

Closed adrianinsaval closed 1 month ago

adrianinsaval commented 6 months ago

Hi @shaise, this is entirely on me but unfortunately I didn't get to make the PRs I wanted in time and in smalls steps for all the changes I've been working on, sorry about that. This PR brings in all I did until now:

  1. Add task panel to AddWallCmd
  2. refactor App/Gui separation
  3. refactor unfold command and make it parametric

For now the unfold command is not doing export anymore as I considered that for a more consistent UX this should be left to the regular export command, I will start a discussion with the design working group about this and the projected sketches. Let me know what you think of this.

The PR is pretty big but a lot of it is just moving things around into separate files. Are you ok with the PR like this or do you want me to work on breaking it down in smaller chunks?

I will be more disciplined in the next development steps and make smaller easier to review PRs

shaise commented 6 months ago

Hello @adrianinsaval Thank you for this hard work. Indeed it is best to do things in small chunks, but for this commit we will do our best to test it as best as we can. The main issue in this commit, is that it is incompatible with old files. For me this is the most important thing. I do not want people to not be able to open old files. This is probably because some classes were renamed. When you save a file freecad store the features class name. If you change it, old files will not be able to load. Same for properties of a feature. This is why I always make everything backward compatible even if it means adding many if-then-s. for example this simple file does not load: test-makebend.zip

adrianinsaval commented 6 months ago

Hmmm I'll have to check which is the culript here. The SMSolidBend class still exists. But you're totally right

GS90 commented 6 months ago

Hello, Compatibility with previously created files is very important...

In the new version of Ondsel, I cannot correctly open files containing sheet metal parts. The main problem is recognizing the object created by the AddWall function. The object is not editable, and everything above in the construction tree is hidden.

It looks like this: WallError WallError.zip

shaise commented 6 months ago

@GS90, Indeed you are right. See the comments above - Ondsel are already notified with this issue, and they will probably fix it. Do you mean that in the latest Ondsel version this PR is already merged? If so, the fix is more urgent.

GS90 commented 6 months ago

@shaise, Yes, the latest version of Ondsel has integrated SM workbench with changes from this PR.

shaise commented 6 months ago

@GS90, Thanks for this info, I'll try to contact them.

adrianinsaval commented 6 months ago

I had misunderstood how restoring objects worked, it should be working now in this PR. I will see about making a bugfix release for ondsel within the next day

shaise commented 6 months ago

@adrianinsaval, Thank you for the fixes but I don't think proxies are a good Idea. I really think the old cmd names and file names should be restored. I can not even select the WB now, When selected it shoes errors and not load: image

adrianinsaval commented 6 months ago

I can not even select the WB now, When selected it shoes errors and not load:

fixed

I really think the old cmd names and file names should be restored.

I can do this but I would like to have clear separation between app and gui sections of the code if possible, let me know

shaise commented 6 months ago

First of all - Thanks for your hard work. Having a nice GUI for all features makes the WB much better!

I can do this but I would like to have clear separation between app and gui sections of the code if possible, let me know

The GUI files can be new but the command ones should stay the same. IMHO its better then using proxies. Proxies are kind of dead software that stays forever. Further more if I save something with the new version it will not load in the old one. Indeed there are times when this is a must, but for commands that basically did not change there is no reason.

some first tests observations:

  1. in AddWall command, some issues in the UI - label and field not in the same line. image

  2. When editing a feature and I want to select different base features, I can de-select by clicking on a selected face/edge but I can not de-select all. (IE by clicking on an empty space)

  3. When adding a feature to a non selected body, instead of the usual error that you are modifying a non selected object, the operation succeeds but the feature created is outside the body. This is what what happens when you do it in the original version: (the correct way) image This is what happens on ondsel version: image

These are only preliminary tests. I need to do more thorough ones. This is why indeed it is best to do it in small steps.

shai

shaise commented 6 months ago

@adrianinsaval , any updates?

adrianinsaval commented 5 months ago

sorry for the delay, I've been dealing with some personal issues. I restored the previous file structure. I don't think this is an ideal structure (the inconsistent naming is confusing) but I can understand wanting to maintain full compatibility. We will ship something similar in a bugfix release of ondsel es but that has some proxies to migrate back files created with our previous release to this format.

adrianinsaval commented 5 months ago
  1. in AddWall command, some issues in the UI - label and field not in the same line.

I did this on purpose to avoid unnecessarily wide dialogs and shortened words as these are often problematic for translations

  1. When editing a feature and I want to select different base features, I can de-select by clicking on a selected face/edge but I can not de-select all. (IE by clicking on an empty space)

I see this as desired behavior, accidentally deselecting everything due to a missclick would be a PITA. The idea is to emulate the behavior of PartDesign (see fillet/chamfer dialog). I do think that a quick way of deselecting everything is missing, I can work on that on the next iteration.

When adding a feature to a non selected body, instead of the usual error that you are modifying a non selected object, the operation succeeds but the feature created is outside the body.

my interpretation was that these where warnings not errors and therefore shouldn't block the interface. My intention was to later migrate these to use the notification system as sketcher does nowadays. Let me check these more closely

shaise commented 5 months ago

Great then. So actually only the last issue is important since it brakes part-design structure.

pierreporte commented 3 months ago

Any news on this?

obelisk79 commented 2 months ago

I hate to see this 'die on the vine'... any chance of forward movement?

shaise commented 2 months ago

If this is not resolved, I plan to cherry pick the changes and merge the important staff. But it will take time as I need to resolve important issues in CAM simulator

adrianinsaval commented 2 months ago

I will update this PR this week

shaise commented 1 month ago

@adrianinsaval , I have manually merged your changes carefully cleaning all conflicts. Hope it worked. If you plan to continue your work, please start clean from the latest master branch.