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

Part: Improve support for Links #6478

Merged
merged 1 commit into from Apr 8, 2022
Merged

Conversation

mwganson
Copy link
Contributor

I accidentally closed the other PR for this, so I am creating this one. There haven't been any changes to the code except to resolve some conflicts with a recent commit (removing some includes).

Link to closed PR: #6353

Link to forum discussions:

https://forum.freecadweb.org/viewtopic.php?f=10&t=66159
https://forum.freecadweb.org/viewtopic.php?f=17&t=66449

It's a fairly big PR and the feature freeze is coming, so this probably will be for 0.21. But I think it would be nice to improve Links support in Part for 0.20, if possible.

@github-actions github-actions bot added the WB Part Related to the Part Workbench label Feb 28, 2022
@freecadci
Copy link

pipeline status for feature branch PR_6478. Pipeline 481498940 was triggered at a02ccc0. All CI branches and pipelines.

@luzpaz
Copy link
Contributor

luzpaz commented Mar 1, 2022

I accidentally closed the other PR for this, so I am creating this one.

You can re-open a closed PR, right? 😕

@mwganson
Copy link
Contributor Author

mwganson commented Mar 2, 2022

It kept showing 0 commits even though I added commits to it. Maybe after reopening they would have shown up?

@donovaly
Copy link
Member

@realthunder , can you please review this PR?

@realthunder
Copy link
Collaborator

@mwganson Hi, I've gone through your changes. Here are my thoughts.

First, DocumentObject::getLinkedObject() by default will recursively resolve to the final linked object. If the object is not a link, it will return itself. So it is always safe to just call getLinkedObject() directly. And normally, you shouldn't explicitly check of App::Link::getClassTypeId(), as the core functionality of link is implemented as an extension App::LinkBaseExtension and can be install to any object. It is enough to simply use getLinkedObject() to check for link.

Second, If you have removed Part::Feature::getClassTypeId() checking in the relevant part features, you don't need to check it in the relevant command or dialog either. You can simply check if Part.getShape() returns a valid shape, which makes it possible to support other non Part::Feature, such as App::Part container. My general idea is to put as little type checking as possible. Since all that's needed as input for most part features is the shape, then that's all we need to check, instead of the input object type. I haven't done so already because of one complication. It is currently very difficult to obtain per face color from non part features, especially for containers which may be nested. There will be a general solution to this problem once the new topo naming gets merged.

Third, speaking of topo naming, although it is not relevant now, it would be better to use Part::TopoShape whenever possible, and the companion Part::Feature::getToposhape(), instead of TopoDS_Shape or Part::getShape(), because future topo naming information will be stored inside Part::TopoShape.

So my advice is that, if this PR is aiming for 0.20 release, then please make some changes to the usage of getLinkedObject(), and remove App::Link::getClassTypeId() check. If it is for the next development cycle, then I'd suggest to remove most of the type checking and check for input shape directly.

@mwganson
Copy link
Contributor Author

@realthunder

Thanks for the information.

I will revise based on your feedback.

@mwganson
Copy link
Contributor Author

@realthunder

I'm making some good progress, and I think it will be useful to be able to use App:Part objects similarly to using App::Link objects, but I have discovered something. If the App::Part container contains a sketch I can extrude it by selecting the Part and clicking Extrude toolbar icon. But if the sketch is later made invisible the Extrude object fails to recompute.

Same is true for Check Geometry function. If I have for example a Cube and Cylinder in the App::Part container, select the container, and click the Check Geometry toolbar icon, it checks both the Cube and the Cylinder (and might show some self-intersections if they overlap), but if Cube is made invisible it does not get checked, same for Cylinder.

The behavior seems to be if the Part::Features within the App::Part are made invisible they are not returned as part of the shape with:
//docobj is a pointer to some App::Part document object.
Part::TopoShape topoShape = Part::Feature::getTopoShape(*docobj->getLinkedObject());
topoShape.isNull(); //true if all objects in the App:Part are hidden, false if any are visible

This can be demonstrated in python without need to build the PR.

Create a App::Part container and put a Part::Cylinder into it. Select the Part and press Ctrl+Shift+P. In the python console:

shp = obj.getLinkedObject().Shape

This works if the Cylinder is visible, but fails with App.Part has not attribute 'Shape' if the Cylinder is made invisible.

Is this the expected behavior or a bug? I can see how it could be useful to be able to control which shapes are in the compound that represents the App::Part container's shape by making them visible or invisible, but this will also be a source of confusion for many users. Visibility does not normally matter when it comes to the shape of an object.

@mwganson
Copy link
Contributor Author

Converting to draft because I still have a few more dialogs to revise. Ones I have done so far: ruled surface, check geometry, extrude, thickness, offset2d, offset 3d, cross-sections, and revolution.

@mwganson
Copy link
Contributor Author

I am using Part::TopoShape where possible, but my knowledge of OCC is not great, so where TopoDS_Shape is already being used it is trivial to convert from TopoShape to TopoDS_Shape.

Part::TopoShape topoShape = Part::Feature::getTopoShape(docobj->getLinkedObject());
TopoDS_Shape shape = topoShape.getShape();

This will hopefully make it a bit easier if the TopoShape is needed later for topo naming mitigation since it will be there already.

@freecadci
Copy link

pipeline status for feature branch PR_6478. Pipeline 491259460 was triggered at 86db251. All CI branches and pipelines.

@realthunder
Copy link
Collaborator

Part::Feature::getShape/getTopoShape() will handle link inside. In fact, if the link is at a different placement, calling getTopoShape(obj->getLinkedObject()) will obtain the shape at the wrong location.

@realthunder
Copy link
Collaborator

The behavior seems to be if the Part::Features within the App::Part are made invisible they are not returned as part of the shape with:

I have an very old pending PR that address this problem, by having the ExportMode attribute in group feature to control how to export shape of a group. The default the export by visibility. There is also option to control export by injecting property into each child for individual control. I'll see if I can refresh this PR.

@mwganson mwganson force-pushed the part_links branch 3 times, most recently from 7b286df to 204dbed Compare March 14, 2022 16:10
@mwganson
Copy link
Contributor Author

@realthunder
Okay. I will use Part::Feature::getTopoShape(*docobj) instead of Part::Feature::getTopoShape(*docobj -> getLinkedObject())

I have discovered another anomaly when using the App::Part containers.

Create a new App::Part. Add a sketch to it with a rectangle inside the sketch. Make the sketch visible. Select the App::Part, press Ctrl+Shift+P. In the python console:

obj.Shape
(as expected)

Extrude the sketch. Add the extrude to the App::Part. So now you have:

Part
    > Origin
    >  Extrude
            Sketch

Now make Extrude invisible and Sketch visible. Select the App::Part, Ctrl+Shift+P. In the console:

obj.Shape
Attribute error: 'App.Part' object has no attribute 'Shape'.

When selecting the App::Part, the sketch appears green in the 3d view, indicating it has been selected. It looks like what is happening is only the top level shapes are exported, and only then if they are visible.

@mwganson mwganson marked this pull request as ready for review March 14, 2022 20:31
@mwganson
Copy link
Contributor Author

Some things I did not touch because we are in the feature freeze and already this is a relatively large PR. Among the untouched: Fillets, Chamfers, Project to face, Shape builder, Defeaturing.

@realthunder
Copy link
Collaborator

When selecting the App::Part, the sketch appears green in the 3d view, indicating it has been selected. It looks like what is happening is only the top level shapes are exported, and only then if they are visible.

This is by design. The part container (actually App::GeoFeatureGroupExtension) will auto add all dependency of its children. But those dependency are not claimed in the tree view. The logic is that since those dependency are used to create the shape of some child object in the container, it shouldn't be part of the compound shape of the container. Think of a fusion in a Part container. It does not make sense to include the tool shapes of the fusion as they'll occupy the same 3D space. It makes even less sense for a feature like cut.

@freecadci
Copy link

pipeline status for feature branch PR_6478. Pipeline 492097089 was triggered at 204dbed. All CI branches and pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_6478. Pipeline 493289350 was triggered at 62f885a. All CI branches and pipelines.

@donovaly
Copy link
Member

This will be the last week I which feature PRs can be merged for FC 0.20.

What is the status of this one?

@mwganson
Copy link
Contributor Author

I think it's ready to go unless changes are required upon review. I have rebased locally. After I finish compiling and running the tests I will force push.

@donovaly
Copy link
Member

@realthunder , is this PR ready to be merged in your option?

Copy link
Collaborator

@realthunder realthunder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is mostly okay. Just a few minor changes to make it more flexible.

src/Mod/Part/Gui/Command.cpp Outdated Show resolved Hide resolved
src/Mod/Part/Gui/Command.cpp Outdated Show resolved Hide resolved
src/Mod/Part/Gui/Command.cpp Outdated Show resolved Hide resolved
src/Mod/Part/Gui/TaskSweep.cpp Outdated Show resolved Hide resolved
src/Mod/Part/Gui/TaskSweep.cpp Outdated Show resolved Hide resolved
@freecadci
Copy link

pipeline status for feature branch PR_6478. Pipeline 497808538 was triggered at a07b7cb. All CI branches and pipelines.

@mwganson
Copy link
Contributor Author

mwganson commented Apr 3, 2022

Sorry, I did not get a notification of the new reviews. This is now tagged for 0.21 anyway. But I suppose it could be argued it is a bug fix and not a new feature.

@donovaly donovaly removed the For 0.21 label Apr 4, 2022
@donovaly donovaly added this to the 0.20 milestone Apr 4, 2022
@donovaly
Copy link
Member

donovaly commented Apr 4, 2022

But I suppose it could be argued it is a bug fix and not a new feature.

OK, putting it then on the 0.20 milestone list.

@freecadci
Copy link

pipeline status for feature branch PR_6478. Pipeline 508810306 was triggered at 7853243. All CI branches and pipelines.

@donovaly
Copy link
Member

donovaly commented Apr 6, 2022

@mwganson what is your time frame for this PR? Since it is in the 0.20 milestone, it should be merged soon, let's say it should be ready within a week? Would that be possible?

@mwganson
Copy link
Contributor Author

mwganson commented Apr 6, 2022

I think it's ready now. Sooner the better so more people can build and test and report any issues.

@donovaly
Copy link
Member

donovaly commented Apr 7, 2022

@realthunder , is the PR OK to be merged in your opinion?

@realthunder
Copy link
Collaborator

Looks fine to me.

@donovaly
Copy link
Member

donovaly commented Apr 8, 2022

Great, so one PR less from the 0.20 milestone.
Merging now.

@donovaly
Copy link
Member

donovaly commented Apr 8, 2022

@mwganson , don't forget to add documentation to the Wiki and to add it to our release notes: https://wiki.freecadweb.org/Release_notes_0.20

@donovaly donovaly merged commit 731ed82 into FreeCAD:master Apr 8, 2022
@mwganson mwganson deleted the part_links branch April 20, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB Part Related to the Part Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants