Attempt to fix part of #2504

About the development of the Part Design module/workbench. PLEASE DO NOT POST HELP REQUESTS HERE!
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
poserge
Posts: 58
Joined: Mon Apr 06, 2015 4:06 am

Attempt to fix part of #2504

Post by poserge »

Hi,

I'm trying to fix #2504 but I'm not sure if it is correct or not.
Bug report consist of two parts:
1. undo after migrate is not working properly
2. Revolve and Groove give errors when applied to a face.
This PR fixes second part and it is done by not allowing Revolve and Groove to be applied to faces.

I've read in old thread that they are supposed to work on faces but I'm not sure if it still should be possible.
This is the link to PR

Thanks for review.

Update: if I should not post each and every small change I'm making please let me know or just move this post somewhere else. I just feel bad when something doesn't work and I know I can fix it.
C++ beginner, willing to help FC with coding. Using FC for converting/decomposing STEP models into iges.
ickby
Veteran
Posts: 3116
Joined: Wed Oct 05, 2011 7:36 am

Re: Attempt to fix part of #2504

Post by ickby »

Havin a seperate discussion if you have question is good, that makes it seperated clearly. If you are sure how to do things you don't need to ask, but if unsure the current workflow works well.

To the problem:
Revolve/Groove should work with faces too. They are also ProfileBased, and that class should theoretically abstract all differences between sketch and face away. We should investigate why Revolve/Groove are failing. Simply avoiding faces is not a good fix.
ickby
Veteran
Posts: 3116
Joined: Wed Oct 05, 2011 7:36 am

Re: Attempt to fix part of #2504

Post by ickby »

Note that the linked bug also shows some bad behavior for drag/drop. I fixed this already in a local branch, so no need to put work into this topic.
poserge
Posts: 58
Joined: Mon Apr 06, 2015 4:06 am

Re: Attempt to fix part of #2504

Post by poserge »

Thanks, I'll investigate.

About drag-n-drop -i remember I've fixed something similar viewtopic.php?f=27&t=18975. Is the bug still there?
C++ beginner, willing to help FC with coding. Using FC for converting/decomposing STEP models into iges.
ickby
Veteran
Posts: 3116
Joined: Wed Oct 05, 2011 7:36 am

Re: Attempt to fix part of #2504

Post by ickby »

poserge wrote:About drag-n-drop -i remember I've fixed something similar viewtopic.php?f=27&t=18975. Is the bug still there?
Ah no, your fix from back then works well. I fixed a few other drag n drop bugs in my branch. I'm actually not sure if the one mentioned in the bug report is still there, can't test right now. I just wanted to ensure that you do not spent too much time to fix drag n drop bus while I have done alot on this front already.
poserge
Posts: 58
Joined: Mon Apr 06, 2015 4:06 am

Re: Attempt to fix part of #2504

Post by poserge »

Hi,

I think I've fixed Revolve and Groove to accept faces. Here's the link to PR.

P.S. I've opened new PR on this which is probably not the best way of doing PRs.. Should have probably re-opened the initial one and augmented with new commits..
C++ beginner, willing to help FC with coding. Using FC for converting/decomposing STEP models into iges.
ickby
Veteran
Posts: 3116
Joined: Wed Oct 05, 2011 7:36 am

Re: Attempt to fix part of #2504

Post by ickby »

this looks good, thanks for digging into it. One question: could you add a unit test for this behavior to part design? Lately we have a lot of failing functionality that worked once, as PartDesign is complex with all its selections and rules etc. this can happen easily, and I'm a bit afraid that we will not manage to keep such things running if there are no unit tests.
poserge
Posts: 58
Joined: Mon Apr 06, 2015 4:06 am

Re: Attempt to fix part of #2504

Post by poserge »

I agree that tests are required, I'll add them. Thanks for reviewing.
C++ beginner, willing to help FC with coding. Using FC for converting/decomposing STEP models into iges.
Post Reply