PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by realthunder »

PR link: https://github.com/FreeCAD/FreeCAD/pull/2723

The motivation of this PR is to enhance App::Part (GeoFeatureGroupExtension) for better Link support, and to fix some backward compatibility issue of plain group (GroupExtension). In the development process, however, more and more effort was spent on enhancement of 3D visualization and selection, which are related to the changes in group behavior one way or another.

First, STEP import now uses App::Part by default. A new option 'Use legacy importer' allows to switch back to old STEP importer before the merge. Another option 'Use App::Part' (when disabled) allows you use back LinkGroup. But there is hardly any reason to use LinkGroup anymore for an average user. Because App::Part can do most things LinkGroup can, such as override children visibility and color. You can try these with the 'Set colors' context menu action of an App::Part. Another change is that App::Part (OriginGroupExtension) no longer creates origin features by default. This saves six objects per container, which is rather significant for STEP import with large amount of small assemblies, where the majority of the imported container will never need to use the origin feature. To initialize the origin features, simply toggle the visibility of the Origin group.
step-import.png
step-import.png (102.18 KiB) Viewed 7716 times

GroupExtension (and thus GeoFeatureGroupExtension) has a new property 'ExportMode' to determine how to export its children. 'Export' here means to generate a shape using Part.getShape() so that the group can be used for modeling directly, as well as STEP export. By default both DocumentObjectGroup (i.e. plain group) and App::Part group set this mode to 'By Visibility', meaning that only the visible children are include for shape generation. All other objects having these two extensions will set the property to 'Disabled', meaning no export. When export 'by visibility', changes to any child object will touch the group to force a recompute in order to sync the shape. Another option for 'ExportMode' is 'Child Query', which will inject a new boolean property 'Group_EnableExport' (shown in property eidtor simply as 'Enable Export') into each child object, and use that property to decide whether to include the children for export or not. And finally 'Both', meaning export children by both 'Child Query' and visibility.
group-export-mode.png
group-export-mode.png (49.45 KiB) Viewed 7716 times

There is a change in plain group visibility toggling behavior. Previously, when a group is hidden, all its (nested) children are hidden by changing the 'Visibility' property of all of them. This is still the same, but the plain group will remember its children visibility before toggling, and will restore the children visibility when the group is shown again. The tree view has special logic to show user the remembered the visibilities of all children instead of the actual visibility. So you won't see all of them being grayed out. If all immediate children of some group is made invisible, the next time the group is shown it will make all children visible again. If you want the old behavior that when a group is made visible, all its (nested) children become visible, you can use the tree view context menu action 'Toggle group visibility'.

Image


(@wmayer, I can split the PR here, as the above is less graphics related)


The primary reason of introducing 'ExportMode' is that it seems export by child visibility (which is the default behavior since the big merge) disturbs many existing objects, because toggling children visibility may touch group object. Motivated by this, I have developed a new view option 'SelectionOnTop'. When enabled, any selection will make the selected object to be rendered on top with transparency, even if the object is hidden or burried in multiple level of hierarchies with multiple hidden parents in between. The tree view option 'SyncView' is modified such that if an object outside of the current view is selected in the tree, the view will auto zoom/pan to show the object. In other words, these two options, when working together, allows you to show any object, no matter where it is, by simple selecting it in the tree view. This is achieved by a new Coin3D node SoFCSwitch, replacing the mode switch node in every view provider. This node allows one to dynamically override the switch in various ways. In addition, the object is not only rendered on top, but picked on top first, meaning that those object will get picked in 3D view over other objects. The mouse over highlight of tree view will show hidden objects as well.

Image


You may be questioning about the performance with all the transparency and on top rendering. And this PR addressed that as well. Transparency and selection rendering now has VBO accerlation, too. It also fixed the long existing transparency rendering artifacts. Current FC handles transprency by first sorting all the transparent trianglation primitives and then rendering them from back to forth, for each and every frame. The problem is that the sorting is based on screen projection distance, and is susceptible to rounding erros. The end result is that at some viewing angle, the incorrect sorting may lead to 'holes' in the rendered object. The solution is to sort the face as a whole without triangluation. The sorting may still be wrong in some viewing angle, but at least the visual appearnce is consistent for the whole face, instead of holes caused by rendering triangles from other faces. This improves rendering performance as well, since we have less primitives to sort (10~100+ times less), and we can reuse the VBO buffered indices for each face. In addition, opaque and transparent faces within the same object (more precisely, the same SoBrepFaceSet node) will be rendered in separate passes.

In short, the transparency rendering not only looks better, but runs faster.

phpBB [video]



The 3D ray picking performance has been improved. The per face sorting for transparency rendering caches the bounding box per face. This is used to improved the picking speed as well. (@wmayer, I have tried octree acceleration like you mentioned in the other post, but did not include it in this PR, because my test shows that the bottleneck lies somewhere else. generatePrimitives() does not dynamically create anything. Just repeatedly fills in some local variables and invokes callback, which shouldn't be slower than unaccelerated rendering). The performce suffers the most when the picking radius is too big, or the camera is zoomed too far. FC has a user configurable ray pick radius setting. Just imagine a circle instead of mouse pointer, any 3D primitive touches or is inside the circle will be included in the returned picking list. The default picking radius is 5 pixels, which is fairly reasonable for normal viewing. But if the user sets it too big, or when the camera is zoomed out, that circle may intersects tens of thousands of primitives, depending on the scene complexity, most of which are useless lines and points. The fact that each returned picked point includes a full Coin3D path leading to the picked geometry only makes it worse, especially so for deep hierarchies. This problem has been fixed (using SoFCRayPickAction) such that only the nearest picked point will be kept during traversal. Another optimization is to limit the ray picking frequency using a timer.

A new 3D view context menu action 'Pick geometries' is added for easy picking of hidden geometries. Similar function has been available with SelectionView's 'Pick list' since the big merge. But a menu action is more convenient in many use case. The picking is also enhanced with the 'SelectionOnTop' feature.

Image


Another visual related improvement is the bounding box calculation. There is a new ViewProvider API, getBoundingBox(), first introduced in the big merge. This PR refactored this API to calculate a more accurate bounding box for containers. Coin3D's SoGetBoundingBoxAction and SoBoundingBoxCache have a design flaw resulting sub-optimal bounding box when there are intermediate transformation nodes inbetween the hierarchy. The reason is that Coin3D caches bounding box at each hierarchy (SoSeparator), with the box aligned to the axes of the local coordinates, this means, the lower hierarhcy bounding box will have to go through multiple projections when accumulating to calculate the bounding box of the upper hierarchy. The projection transforms the bounding box to align to the corresponding coordinate system, which inevitably makes it bigger. ViewProvider::getBoundingBox() accepts an initial transformation matrix, and will traverse down the view provider hierarchy by itself instead of relying on Coin3D, until it reaches a bottom shape view object, which will obtain the bounding box through SoGetBoundingBoxAction. The box will then be projected once and only once using the accumulated transformation matrix during traversal. The result will be a correctly aligned and optimally bounded box in the requested coordinate system (defined by the initial transformation matrix). See the different result in picture below, the bigger box also results in a smaller rendered object when doing view fit as shown here.
bound.png
bound.png (50.29 KiB) Viewed 7716 times

(@wmayer, the reason to work on bounding box calculation is because of the visibility override affects Coin3D cached box, which leads me to discover its design flaw.)

This improved getBoundingBox() API is used in many places,

* ViewAll and ViewSelection command uses this to get a better view fit. Note that ViewSelection has changed behavior to expand the view to include the selection, instead of fitting the view for selection only. You can use a false boolean parameter Preferences/View/ViewSelectionExtended to get back the old behavior.

* The 'BoundingBox' property is now moved from ViewProviderGeometry to ViewProviderDocumentObject to show a bounding box obtained with this method. In other word, every object can show a bounding box with size now.

* The same bounding box will be shown if the view object has its 'SelectionStyle' set to 'BoundBox'.

* DocumentObjectGroup now defaults to 'BoundBox' selection style. Previously, there is no visual feedback when a group object is selected.

* App::Part and PartDesign::Body now uses this API to calculate origine and datum object size.

* The 'Box element selection' command also benifites from the more accurate bounding box. BTW, when I was browsing Coin3D and some existing FC code, I found out the easy way of distinguish back face from front face. This command now only selects the front face by default. To select back face as well, hold 'Alt' key before releasing the mouse button. The 'element selection' also respects selection on top the same way as ray pick, meaning that when 'SelectionOnTop' is enabled and there are selections, the box element selection will only pick faces of the top objects. As shown in the following screencast, the first pick select the cube and makes it on top. The second pick only works on the cube, even though it touches the cyclinder. None of the two picks selects any back face.

Image


Face (pre)selection rendering is improved to auto change its highlight color if it is similar to object's original face color. But there are cases where even this does not provide enough distinction. Here is an example, the top picture is the original shape with a face colored red. The second one is the old FC transparency rendering with the inner face selected. You can clearly see the transparency artifacts here. The third picture is the improved rending with the inner face pre-selected. The fouth picture shows the inner face selected. Here is the problem. It is supposed to be green, but the sorting order is wrong. The outer face is considered nearer, so it is drawn later overriding the inner face color. The last picture shows the selection with triangulation wire. In case you are wondering why the pre-selection rendering works, it is because I have 'SelectionOnTop' enabled, and pre-selected faces are rendered in a separate group node after the selected objects.
menu-face-wire.png
menu-face-wire.png (29.76 KiB) Viewed 7716 times
selection-face-wire.png
selection-face-wire.png (305.45 KiB) Viewed 7716 times

Last but not the least, there is this intreresting feature that I always wanted ever since I started Link development. It is now made possible thanks to the above visual and selection advancement. Think about how one builds a model with Part features. Unlike PartDesign, we often move the features around before doing boolean operation on them to produce new shape. Because of all these movements, it is often difficult to see the relationship between the parent and child shape, especially so when there are multiple hierarchies inbetween. Now, when you create a Link to a Part feature, it will show (with mouse over tree view highlight, or 'SelectionOnTop' enabled) all its children under the same coordinate system defined by the Link's Placement. It makes a linked feature behave similar to an App::Part. No matter how deep a child resides, you can easily find out how it contributed to the parent shape. As shown in the screencast below, you can't really see the parent child relationship in a normal Part feature. But with a Link, the relationship is obvious and always up-to-date.

Image


(@wmayer, this is not really a feature per se. It is a work around for Linked children selection problem inside an App::Part. For a Link inside App::Part, the Link's child may exist in the (grand)parent App::Part, or not, regardless of which, it is hard to determine where the child should be displayed. I know this problem exists in the big merge, but I haven't decided what to do with App::Part at that time, and also it is very hard to make it right. I have since made the decision to use App::Part as the main container, so I have to face all the problems it has. With the visibility overriding stuff in place, it is possible now. In fact, that is actually one of the reason why I worked on visibility override in the first place, which leads to a big PR as it is now).

This feature still needs some improvement, and I hope it does not create too much confusion. You can consider what's shown by the Link a 'virtual image' of the child that is mapped into the coordinate space defined by the Link's placement. Because the actual child may live in a very different coordinate space. So when you actually toggle the child visibility, you may find it pops up in some other place. Ideally it would be better for the Link to have separate (nested) children visibility control as well. In fact, IMO, every object in FC can/should behavior like a container, and App::Part will no longer need to force claim all dependency of its member. Heck, you can even use a Part compound to do assemble, or any other Part features. The tree view hierarchy and 3D scene hierarhcy will be exactly the same. And the world will be a much better place... But again, that'll force the user to break some old habit, and may be too much for some... Well, maybe we'll see about that in the future.
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by DeepSOIC »

This is still the same, but the plain group will remember its children visibility before toggling, and will restore the children visibility when the group is shown again.
Why? Why not just make it consume the viewprovider like App::Part does?

Anyway. on to reading...
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by DeepSOIC »

realthunder wrote: Fri Nov 22, 2019 12:14 pm Now, when you create a Link to a Part feature, it will show (with mouse over tree view highlight, or 'SelectionOnTop' enabled) all its children under the same coordinate system defined by the Link's Placement.
Well, this is cute, but IMO it's indeed creating more confusion than it solves. The proper ways to address this are imo:
* do not show the "children" of Cut in the link (and some (NormandC?) even blame that this deep nesting for driving them away from Part, and I sometimes agree, i.e. it may be a good idea to not have Cut claim any tree children at all).
* put them all in Part and make Link of part
* put them into a specialized container like PoM's Module, and make link of container
* clone before linking

The biggest problem there, imo, is that it displays something in a place where it isn't, and it's important that the user understands that there isn't anything there. Otherwise we naturally have a feel we can use that "placed thing" for something, and end up in a gotcha that we can't.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by DeepSOIC »

realthunder wrote: Fri Nov 22, 2019 12:14 pm To initialize the origin features, simply toggle the visibility of the Origin group.
Simply?! Isn't it too much apparent simplicity at a cost of hidden code complexity? I even don't like that just toggling stuff around to explore a project makes my file marked as changed, now this?

How about a context menu entry instead? Or even a specialized tool, delete origin features / create?
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by DeepSOIC »

realthunder wrote: Fri Nov 22, 2019 12:14 pm Because App::Part can do most things LinkGroup can, such as override children visibility and color.
What's the point of that? A thing can only be in one group/part, so isn't it 100% redundant? What does the visibility of the actual contained object do then? or do you aim to eliminate that "object can only be in one group" rule?
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by DeepSOIC »

realthunder wrote: Fri Nov 22, 2019 12:14 pm A new 3D view context menu action 'Pick geometries'
Now that's a good one 😍
Last edited by DeepSOIC on Fri Nov 22, 2019 4:37 pm, edited 1 time in total.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by DeepSOIC »

realthunder wrote: Fri Nov 22, 2019 12:14 pm Another optimization is to limit the ray picking frequency using a timer.
Can you elaborate on what exactly is optimized there? It can only slow down the response to moving the cursor, and create a potential for a wrong thing being picked because picking it just took so long that a user moved the pointer elsewhere and clicked...
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by realthunder »

DeepSOIC wrote: Fri Nov 22, 2019 1:54 pm
This is still the same, but the plain group will remember its children visibility before toggling, and will restore the children visibility when the group is shown again.
Why? Why not just make it consume the viewprovider like App::Part does?
The child of a plain group can be freely claimed by other objects, like some Part feature. Consuming viewprovider will make it hard to toggle visibility of those child. A better way is to create a child group node, and mirror the view provider nodes, instead of consuming it. That's actually how I did it with the Link's children. And I have tried it on plain group too, which makes it behave like a Link. And as you mentioned later, it can cause confusion, so I reverted it back.

DeepSOIC wrote: Fri Nov 22, 2019 2:27 pm
realthunder wrote: Fri Nov 22, 2019 12:14 pm Now, when you create a Link to a Part feature, it will show (with mouse over tree view highlight, or 'SelectionOnTop' enabled) all its children under the same coordinate system defined by the Link's Placement.
The biggest problem there, imo, is that it displays something in a place where it isn't, and it's important that the user understands that there isn't anything there. Otherwise we naturally have a feel we can use that "placed thing" for something, and end up in a gotcha that we can't.
I agree here. That's why I said every object should behave like Link here. My idea is that Part (or maybe any workbench) should not allow to reuse any object that's not in the global coordinate space, or equivalently, in the root of the tree view (since they will have exactly the same hierarchy by then). To reuse any claimed object, just create a Link to it. The added benefits is that now you can freely change the placement of any child deep into the history without worrying it being used somewhere else. That's what I meant by 'break some old habit'. This idea maybe similar to your suggestion 'clone before linking', if by linking you mean reuse the object as tool feature.

DeepSOIC wrote: Fri Nov 22, 2019 4:14 pm Simply?! Isn't it too much apparent simplicity at a cost of hidden code complexity? I even don't like that just toggling stuff around to explore a project makes my file marked as changed, now this?

How about a context menu entry instead? Or even a specialized tool, delete origin features / create?
There is a context menu entry for that actually. And the moment any code calls any of the getX/Y/Z() etc., it will make sure the origin features are ready. And no, the container will not be touched or recomputed. It's just a lazy initialization trick here. The visibility toggling I mentioned is the easiest way for an end user. If you still don't like it, you can use parameter Preference/Group/CreateOrigin to get back the old behavior. To be honest, seven objects per origin is really an overkill for such a simple task.
origin.gif
origin.gif (178 KiB) Viewed 7571 times

DeepSOIC wrote: Fri Nov 22, 2019 4:23 pm
realthunder wrote: Fri Nov 22, 2019 12:14 pm Because App::Part can do most things LinkGroup can, such as override children visibility and color.
What's the point of that? A thing can only be in one group/part, so isn't it 100% redundant? What does the visibility of the actual contained object do then? or do you aim to eliminate that "object can only be in one group" rule?
With Link inside an App::Part, this rule has already been broken. I think you don't understand here. This thing is mostly used to override nested child color or visibility, not the immediate child, although it can do, too. This is an important feature for assembly that the STEP standard specifically mentioned.

DeepSOIC wrote: Fri Nov 22, 2019 4:35 pm
realthunder wrote: Fri Nov 22, 2019 12:14 pm Another optimization is to limit the ray picking frequency using a timer.
Can you elaborate on what exactly is optimized there? It can only slow down the response to moving the cursor, and create a potential for a wrong thing being picked because picking it just took so long that a user moved the pointer elsewhere and clicked...
If the picking calculation cannot keep up with the mouse movement, the lagging is bound to happen. The timer is to ensure that there is at least 0.1 second gap between the end of previous picking and the start of the next (configurable with Float parameter Preferences/View/PreSelectionDelay, 0 to disable).
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by DeepSOIC »

realthunder wrote: Sat Nov 23, 2019 12:32 am Consuming viewprovider will make it hard to toggle visibility of those child.
Understood, makes sense.

realthunder wrote: Sat Nov 23, 2019 12:32 am The added benefits is that now you can freely change the placement of any child deep into the history without worrying it being used somewhere else.
except oftentimes that is exactly what I want to do. And I don't quite understand that "deeply into the history" part. Say, box, cylinder, fusion, sphere, cut. Assume, all primitives are claimed (say, they are in a Part). So, I expand cut, it shows fusion(link) and sphere(link). I expand fusion(link), and move box(link). Then, fusion should get updated as well. As fusion may be again re-used, this means I may have inadvertently screwed something up. So I don't quite see the benefit of automatic screening away the motion of operands, it gets too unintuitive too quickly, doesn't it? I'd rather create the links manually if I deem they are needed.
Last edited by DeepSOIC on Sat Nov 23, 2019 1:28 am, edited 1 time in total.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PR #2723: Plain/Geo group behavior change and related 3D visualization/selection enhancement

Post by DeepSOIC »

realthunder wrote: Sat Nov 23, 2019 12:32 am And no, the container will not be touched or recomputed.
okay, but the project still does change (new objects are created). Well, I can for sure get over it.

realthunder wrote: Sat Nov 23, 2019 12:32 am To be honest, seven objects per origin is really an overkill for such a simple task.
True, could have been just one with subelements.



Thanks for answers!
Post Reply