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

Bring in Ondsel changes #335

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

adrianinsaval
Copy link

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

sliptonic and others added 13 commits April 21, 2024 21:43
move .ui files to a resources directory
GetViewConfig and SetViewConfig where copying over more stuff than they should
in some situations bendwall's task panel could not unset greedy selection mode
the behavior of baseshapecmd regarding Body creation was awkward and inconsistent with PartDesign commands, the command now knows when it needs to create a body or just use the an active one if it exists
App/Gui separation
Parametric unfold object
Added button for face selection in dialog
remove sketch creation and export from unfold command, to be added back as a new separate command
@shaise
Copy link
Owner

shaise commented May 10, 2024

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
Copy link
Author

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

@GS90
Copy link

GS90 commented May 12, 2024

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
Copy link
Owner

shaise commented May 13, 2024

@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
Copy link

GS90 commented May 13, 2024

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

@shaise
Copy link
Owner

shaise commented May 13, 2024

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

@adrianinsaval
Copy link
Author

adrianinsaval commented May 14, 2024

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

@adrianinsaval adrianinsaval force-pushed the refactorAppGui branch 2 times, most recently from ae9773f to 2a9d281 Compare May 14, 2024 05:56
@shaise
Copy link
Owner

shaise commented May 14, 2024

@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
Copy link
Author

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
Copy link
Owner

shaise commented May 14, 2024

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
Copy link
Owner

shaise commented May 29, 2024

@adrianinsaval , any updates?

@adrianinsaval
Copy link
Author

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
Copy link
Author

  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
Copy link
Owner

shaise commented May 31, 2024

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

@pierreporte
Copy link

Any news on this?

@obelisk79
Copy link

obelisk79 commented Sep 22, 2024

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

@shaise
Copy link
Owner

shaise commented Sep 22, 2024

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

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.

6 participants