Code review of merged Link3 branch
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Be nice to others! Respect the FreeCAD code of conduct!
Re: Code review of merged Link3 branch
git commit 503f987cf fixes the most urgent issue. With the method allowOverrideViewProviderName() all document objects by default don't allow to override its view provider type.
-
- Veteran
- Posts: 2190
- Joined: Tue Jan 03, 2017 10:55 am
Re: Code review of merged Link3 branch
I think this allowOverrideViewProviderName() function should be added to ViewProvider, because whether it is safe for use as an override or not is really determined by the corresponding ViewProvider's implementation. Maybe shorten the name to just allowOverride(). This function can be called right after view provider creation.wmayer wrote: ↑Fri Sep 13, 2019 10:56 pm git commit 503f987cf fixes the most urgent issue. With the method allowOverrideViewProviderName() all document objects by default don't allow to override its view provider type.
Edit: even better, define the function as allowOverride(pObj) and let the view provider decide whether the object is supported or not. Currently, App::ViewProviderLink and PartGui::ViewProviderPartExt can accept any object.
Re: Code review of merged Link3 branch
Yes, it's true that a DocumentObject can't really decide if its ViewProvider has this flexibility or not, only the ViewProvider itself knows it. When I realized the consequences of git commit ff1d1cd341 I was not really sure what the intended behaviour is supposed to be. Is it really to allow to couple an arbitrary ViewProvider with a DocumentObject or was the actual intention to allow that a Python proxy can implement the method getViewProviderName().I think this allowOverrideViewProviderName() function should be added to ViewProvider, because whether it is safe for use as an override or not is really determined by the corresponding ViewProvider's implementation. Maybe shorten the name to just allowOverride(). This function can be called right after view provider creation.
For the former I couldn't find an example in the code where this is currently used (and probably there aren't many examples where this is even useful) and for the latter I only found a single example in the Draft workbench.
So, because I was not quite sure about the intentions and plans I decided to implement a quick fix that doesn't touch too much of the new possibility but at least avoids its downside.
OK, so then it must be checked inside Gui::Document::slotNewObject() whether the created view provider is compatible and if not an instance of the type delivered with getViewProviderName() must be created.Edit: even better, define the function as allowOverride(pObj) and let the view provider decide whether the object is supported or not. Currently, Gui::ViewProviderLink and PartGui::ViewProviderPartExt can accept any object.
-
- Veteran
- Posts: 2190
- Joined: Tue Jan 03, 2017 10:55 am
Re: Code review of merged Link3 branch
I'll submit a PR for that soon.
-
- Posts: 656
- Joined: Tue May 19, 2015 1:11 am
Re: Code review of merged Link3 branch
I have submitted this pull request, PR#2553. Compiles, runs, and passes tests on my local machine. I'll monitor the travis builds and update as necessary.ezzieyguywuf wrote: ↑Wed Sep 04, 2019 8:47 pm well then, I will begin work on a PR that incorporates these concepts.
Re: Code review of merged Link3 branch
Here is another regression of the Link3 branch that affects the selection:
https://forum.freecadweb.org/viewtopic.php?f=3&t=39957
IMO, the basic problem is the new concept that document objects that are children of certain objects with grouping behaviour (e.g. Body or Part but not DocumentObjectGroup) are considered as sub-elements. Now when querying with SelectionSingleton::isSelected() whether an object is selected it automatically checks if claimed objects are selected and if yes returns true.
This behaviour might be OK for certain use cases but it's a regression for other uses cases which leads to undesired side-effects as shown in the referenced topic.
The behaviour results into another strange side-effect: it allows you to first select the sub-feature(s) of a body but not the body afterwards while when you first select the body you can select any sub-feature.
Also, it makes the API inconsistent because getting the selection of a document and querying all objects of a document if selected leads to different results:
Btw, the Python function Gui.Selection.getSelection(docName=None, resolve=True, single=False) doesn't behave as described.
General remark:
When adding new features it's IMO best to not simply change the implementation of the existing API but add new methods instead and adjust those code parts that should benefit from the changes. This way it can be excluded to introduce any regressions like this in other parts of the code.
https://forum.freecadweb.org/viewtopic.php?f=3&t=39957
IMO, the basic problem is the new concept that document objects that are children of certain objects with grouping behaviour (e.g. Body or Part but not DocumentObjectGroup) are considered as sub-elements. Now when querying with SelectionSingleton::isSelected() whether an object is selected it automatically checks if claimed objects are selected and if yes returns true.
This behaviour might be OK for certain use cases but it's a regression for other uses cases which leads to undesired side-effects as shown in the referenced topic.
The behaviour results into another strange side-effect: it allows you to first select the sub-feature(s) of a body but not the body afterwards while when you first select the body you can select any sub-feature.
Also, it makes the API inconsistent because getting the selection of a document and querying all objects of a document if selected leads to different results:
Code: Select all
# Load a project with a body and select a sub-feature
Gui.Selection.getSelection() # has only one element
selection=[]
for i in App.ActiveDocument.Objects:
if Gui.Selection.isSelected(i):
selection.append(i)
selection # has two elements
When using None it raises an error and says it expects a str, not None.docName - document name. None means the active document, and '*' means all document
The default argument is True but here you can define three different modes which somehow makes no sense. The default of resolve should be 1 (or 2) but not True.resolve - whether to resolve the subname references.
0: do not resolve, 1: resolve, 2: resolve with element map
General remark:
When adding new features it's IMO best to not simply change the implementation of the existing API but add new methods instead and adjust those code parts that should benefit from the changes. This way it can be excluded to introduce any regressions like this in other parts of the code.
-
- Veteran
- Posts: 2190
- Joined: Tue Jan 03, 2017 10:55 am
Re: Code review of merged Link3 branch
PR submitted here
BTW, I looked at MeshPart Tessellation code. It seems to use its own tree widget to offer selection if the user does not select anything in the treeview. Do you think it is still needed? I can easily add hierarchical selection (object in group), and non Part::Feature object support using Part::getShape(), but the extra tree widget is going to cause some confusion, because it displays no hierarchy even though it is a tree. Shall I just remove it?
This is in order to disambiguate the same object in different placement because of it appears under different parents, for example, a child in a group, and the same child in a link to the same group.wmayer wrote: ↑Sun Oct 13, 2019 11:00 am IMO, the basic problem is the new concept that document objects that are children of certain objects with grouping behaviour (e.g. Body or Part but not DocumentObjectGroup) are considered as sub-elements. Now when querying with SelectionSingleton::isSelected() whether an object is selected it automatically checks if claimed objects are selected and if yes returns true.
The design goal is to add the new object path information to the selection while making it completely transparent to legacy code, if the implementation is correct, that is. The problem here is a bug in the implementation. There is a way to distinguish sub-object from sub-element (Face, Edge, Vertex etc.) in the path. Each sub object name must ends with a dot, the sub-element must contain no dot. So a selection path (i.e. the SubName) with sub-object but no sub-element will end with a dot. To support legacy code, Gui.Selection will auto resolve to the last sub-object, and store any sub-element inside SubName just like before.This behaviour might be OK for certain use cases but it's a regression for other uses cases which leads to undesired side-effects as shown in the referenced topic.
Yes, it would be better to add new API to obtained the selection path. But in this case, for the existing APIs, while the call signature can stay the same, the implementation still needs to be changed to incorporate the new concept.General remark:
When adding new features it's IMO best to not simply change the implementation of the existing API but add new methods instead and adjust those code parts that should benefit from the changes. This way it can be excluded to introduce any regressions like this in other parts of the code.
BTW, I looked at MeshPart Tessellation code. It seems to use its own tree widget to offer selection if the user does not select anything in the treeview. Do you think it is still needed? I can easily add hierarchical selection (object in group), and non Part::Feature object support using Part::getShape(), but the extra tree widget is going to cause some confusion, because it displays no hierarchy even though it is a tree. Shall I just remove it?
Re: Code review of merged Link3 branch
Thanks!PR submitted here
Does this logic apply to any object that implements the group logic or only those that have their own placement? I ask because for document object group I didn't notice the issue but for Part container and Body.This is in order to disambiguate the same object in different placement because of it appears under different parents, for example, a child in a group, and the same child in a link to the same group.
OK.The problem here is a bug in the implementation.
After a bit of searching I found it in getSubObjects() of DocumentObject and the overridden extensionGetSubObjects() methods.There is a way to distinguish sub-object from sub-element (Face, Edge, Vertex etc.) in the path. Each sub object name must ends with a dot, the sub-element must contain no dot.
I see.To support legacy code, Gui.Selection will auto resolve to the last sub-object, and store any sub-element inside SubName just like before.
Let me check after integrating your fix.BTW, I looked at MeshPart Tessellation code. It seems to use its own tree widget to offer selection if the user does not select anything in the treeview. Do you think it is still needed? I can easily add hierarchical selection (object in group), and non Part::Feature object support using Part::getShape(), but the extra tree widget is going to cause some confusion, because it displays no hierarchy even though it is a tree. Shall I just remove it?
Re: Code review of merged Link3 branch
It's gone now: git commit 1f5f00e655Let me check after integrating your fix.
It can't say what the original idea was to have the tree widget there because the git history doesn't go far enough. But I think the dialog once was modal and to avoid to disturb the workflow too much the tree was there for convenience. But because the dialog is non-modal (now) the tree can be considered obsolete -- especially if it's problematic with the Link philosophy.
-
- Veteran
- Posts: 2190
- Joined: Tue Jan 03, 2017 10:55 am
Re: Code review of merged Link3 branch
There are actually two types of object path in play here. One is entirely in the App namespace, that relies on DocumentObject::getSubObject() API to resolve the path. The default implementation is to find the first dot in the given subname path, resolve the first level sub-object, and then calls its getSubObject() to resolve the rest of the path. BTW, getSubObjects() has a related but different purpose. getSubObjects() is used to enumerate sub-objects, similar to claimChildren() but in App namespace. It is mostly used for exporting object hierarchy, such as STEP export.
The other type of object path is the Coin3D SoPath. When the user clicks the 3D view, SoFCUnifiedSelection will obtain a set of picked points that contains the path of the selected Coin3D tree node. Before the merge, we identifier the selected object by searching view provider root node from the tail of the path. Now, it searches from the head. The picked SoPath is then passed to the found ViewProvider's getElementPicked() function, which translates the SoPath into App subname path. For object's like GeoFeatureGroup and Link, their getElementPicked() (actually through extension method) is implemented by searching further down the SoPath, find the next level view provider, and calls its getElementPicked(). There is a reverse of this process with ViewProvider::getDetailPath(), which translate subname path into SoPath.
Now, the reason why plain group does not appear to support subname path is because it's view provider does not stack the children under its root node. If you construct a subname path programmatically, you can resolve it using its getSubObject() API, it's just that its view provider node tree does not contain this information. In fact, when a child is picked in 3D view, the group's node won't even appear in the picked SoPath. For backward compatibility reason, I decided to leave it this way. When linked, though, App::Link will internally build a node tree to reflect the hierarchy, and you will get similar selection behavior as App::Part.