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

PartDesign: OnDelete basefeature fixing and visualization #1224

Closed
wants to merge 2 commits into from

Conversation

abdullahtahiriyo
Copy link
Contributor

=========================================================

fixes #3084
fixes #3061 (at least what it is understood in the analysis as being a bug, the visualization left).

Basically when deleting a feature, if it is the base feature (feature with which it will merge or cutout), the dependent objects'
base feature is made to be the base feature of the object being deleted. This code pre-existed at body level, but was not being called (see below).

Additionally, if the visible object is not the one being deleted, we leave that one visible. If the visible object is the one
being deleted, we make the previous object visible.

Deletion from the tree of a feature is handled by Document.removeObject, which has no clue about what a body is. Therefore, Bodies, although an
"activable" container, know nothing about what happens at Document level with the features they contain.

The Deletion command StdCmdDelete::activated, however does notify the viewprovider corresponding to the feature (not body) of the imminent deletion
(before actually doing it). Consequently, the only way of notifying a body of the imminent deletion of one of its features so as to do the clean up
required (moving basefeature references, tip management) is from the viewprovider of the feature being deleted.

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:

  • Branch rebased on latest master git pull --rebase upstream master
  • Unit tests confirmed to pass by running ./bin/FreeCAD --run-test 0
  • Commit message is well-written
  • Commit message includes issue #<id> or fixes #<id> where <id> is the associated MantisBT issue id if one exists

And please remember to update the Wiki with the features added or changed once this PR is merged.


=========================================================

fixes FreeCAD#3084
fixes FreeCAD#3061 (at least what it is understood in the analysis as being a bug, the visualization left).

Basically when deleting a feature, if it is the base feature (feature with which it will merge or cutout), the dependent objects'
base feature is made to be the base feature of the object being deleted. This code pre-existed at body level, but was not being called (see below).

Additionally, if the visible object is not the one being deleted, we leave that one visible. If the visible object is the one
being deleted, we make the previous object visible.

Deletion from the tree of a feature is handled by Document.removeObject, which has no clue about what a body is. Therefore, Bodies, although an
"activable" container, know nothing about what happens at Document level with the features they contain.

The Deletion command StdCmdDelete::activated, however does notify the viewprovider corresponding to the feature (not body) of the imminent deletion
(before actually doing it). Consequently, the only way of notifying a body of the imminent deletion of one of its features so as to do the clean up
required (moving basefeature references, tip management) is from the viewprovider of the feature being deleted.
@wwmayer
Copy link
Contributor

wwmayer commented Jan 14, 2018

As far as I can see this PR makes bug 3084 even worse. As a test case I used the following:
Sketch on xy plane and Pad
Map Sketch001 to a face of Pad and Pocket
Map Sketch002 to a face of Pocket and Pad001
Map sketch003 to a face of Pad001 and Pad002

Now delete Pad002 and confirm.

The issues:
With this PR nothing is actually deleted but Pad001 is moved outside the body.
When touching Pad001 or Sketch002 then after a recompute it shows the error "Links go out of the allowed scope"
Inside the body there is still a reference to Sketch002 that comes before Pad002

@wwmayer
Copy link
Contributor

wwmayer commented Jan 14, 2018

I wonder how this PR can be related to issue #3061. The only code that has been changed is the method ViewProvider::onDelete but that won't be executed when re-ordering the operations.

@abdullahtahiriyo
Copy link
Contributor Author

You are right. I did most of the testing before adding the "return false". Then changed added it and when testing I did not realise the feature was going out of the body (I had more features and it was scroll down of the tree). I have just pushed a fix to this.

After the fix, I have repeated your test. I have always added on top of the previous object. If I delete Pad0002 it seems to work just fine. If I delete "Pad001". After the operation only "Pad" is shown. The reason is that Pad002 starts at z=20 mm and Pad ends at z=10 mm (Pad001 was between 10 and 20 mm). After that deletion it is not a continuous solid anymore. The user must decide how he wants to make the solid continuous afterwards.

This fix does not remap supports of sketches. Doing that would be guessing what the user wants. If the sketch is at the right position after the deletion, then visualization will be ok. The user will still need to decide if he wants to change the support (map the sketch of the following feature on a face of the previous one), or otherwise attach it.

It is also ok to leave the sketch of the pad on the spot, as we do not know what the user intends to do with it.

What I intended with this fix is to do the clean up of the references (inlist), which is not done otherwise, so that the feature n+1 is in the inlist of feature n-1 if feature n is deleted.

Regarding #3061

  • I may have misunderstood the ticket, but most of this ticket is regarded as not a bug (see comments). What it seems to be remaining is the visualization after deletion (there is a deletion in the steps to reproduce). That should be fixed here.

If you have time, give it a try with the new commit, and sorry for the time lost.

@abdullahtahiriyo
Copy link
Contributor Author

Apparently it also fixes this new ticket:
https://freecadweb.org/tracker/view.php?id=3312

@wwmayer
Copy link
Contributor

wwmayer commented Jan 22, 2018

Merged.

@wwmayer wwmayer closed this Jan 22, 2018
@abdullahtahiriyo abdullahtahiriyo deleted the issue_3084 branch November 3, 2020 07:20
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.

None yet

2 participants