Code review of merged Link3 branch

[wmayer]

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.

[realthunder]

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.

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.

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.

[wmayer]

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.

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().

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.

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.

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.

[realthunder]

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.

I'll submit a PR for that soon.

[ezzieyguywuf]

well then, I will begin work on a PR that incorporates these concepts.

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.

[wmayer]

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:

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

Btw, the Python function Gui.Selection.getSelection(docName=None, resolve=True, single=False) doesn't behave as described.

docName - document name. None means the active document, and '*' means all document

When using None it raises an error and says it expects a str, not None.

resolve - whether to resolve the subname references.
0: do not resolve, 1: resolve, 2: resolve with element map

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.

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.

[realthunder]

PR submitted here

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 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.

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 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.

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.

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.


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?

[wmayer]

PR submitted here

Thanks!

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.

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.

The problem here is a bug in the implementation.

OK.

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.

After a bit of searching I found it in getSubObjects() of DocumentObject and the overridden extensionGetSubObjects() methods.

To support legacy code, Gui.Selection will auto resolve to the last sub-object, and store any sub-element inside SubName just like before.

I see.

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?

Let me check after integrating your fix.

[wmayer]

Let me check after integrating your fix.

It's gone now: git commit 1f5f00e655

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.

[realthunder]

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.

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.