Click confusion - setting to set pick radius, to make selecting stuff easier

[DeepSOIC]

Pull request on GitHub:
https://github.com/FreeCAD/FreeCAD/pull/193

[wmayer]

Have to test it first...

[wmayer]

There are a few things that should be fixed:

• inconsistent default values for pick radius. In SoFCSelection and SoFCUnifiedSelection the default is 2 while in the preferences and View3DInventorViewer the default is 5. This leads to inconsistent behaviour. The default must be the same everywhere and IMO should be 2 to have the same standard behaviour as now
• weird design: View3DInventorViewer got the new member "pickRadius" but the argument list of "seekToPoint" has been extended to get the pick radius.
  Nicer design is to add the member to SoQTQuarterAdaptor, restore the interface of seekToPoint and add a getter and a virtual setter method for the pick radius. View3DInventorViewer then can override the virtual setter method to pass the radius to the selection node
• SoFCSelection:

  FIXME: use viewer's pick radius instead, somehow

  Remove this block and write a SoAction subclass which is applied to the scene graph in View3DInventorViewer::setPickRadius. This custom action class then passes the new pick radius to SoFCSelection. As an example for such a class look e.g. at SoHighlightElementAction
• in the preferences there should be set a sensible upper and lower limit. Allowing a minimum of 0 and a maximum of 10000 definitely doesn't make any sense. Since the pick radius is in pixels a minimum value should be 1. A maximum could be e.g. 10 or maybe 20.
• in several places it's checked whether the value is less than 0.001. As noted above it should be checked whether it's less than 1.0

[DeepSOIC]

Thanks for the review!
I'm not currently ready to work on this now, updating will take quite some time.

inconsistent default values for pick radius. In SoFCSelection and SoFCUnifiedSelection the default is 2 while in the preferences and View3DInventorViewer the default is 5.

OK, that's easy.

The default must be the same everywhere and IMO should be 2 to have the same standard behaviour as now

I don't know what is now. I read in OpenInventor docs that if pick radius is not set, it defaults to 5. I only saw value of 2 in navigation styles.

Remove this block and write a SoAction subclass which is applied to the scene graph in View3DInventorViewer::setPickRadius. This custom action class then passes the new pick radius to SoFCSelection. As an example for such a class look e.g. at SoHighlightElementAction

That is completely out of my scope of competence. I'll have to learn all this "action" unintuitiveness.

in the preferences there should be set a sensible upper and lower limit. Allowing a minimum of 0 and a maximum of 10000 definitely doesn't make any sense. Since the pick radius is in pixels a minimum value should be 1. A maximum could be e.g. 10 or maybe 20.

My preferred value for pick radius seems to be 20. As for limits, I always prefer no hard limiting, so I would set the limit to at least an order of magnitude higher than my preferred one, that is, at least 200 (200 might make sense for a very insane remote possibility of running FreeCAD on smartphone). Lower limit: radius is not diameter, so 1 pixel has "radius" of 0.5 pixels. And again, I essentially wanted just limit "above zero", that's it.

[DeepSOIC]

There are a few things that should be fixed:

• SoFCSelection:

  FIXME: use viewer's pick radius instead, somehow

  Remove this block and write a SoAction subclass which is applied to the scene graph in View3DInventorViewer::setPickRadius. This custom action class then passes the new pick radius to SoFCSelection. As an example for such a class look e.g. at SoHighlightElementAction

I don't understand. Wht if SoFCSelection node is added to viewer scene graph as a part of viewprovider of some new object? how will it know the radius? Do I have to fire the setPickRadius action to act on all viewproviders being added?

Next. What if I have two viewers. Now, if I setPickRadius for one of viewers, the SoFCSelection nodes (which are shared between the viewers, right?) get the new pick radius, so in the other viewprovider, pick radii become inconsistent. In either solution.

Sorry, but I fail to understand, why the solution you are suggesting is better. It creates far more mess for achieving basically the same overall partially-broken result. Or I am missing something...

I'm thinking of setting pick radius for event action at the moment it is created... That will remove the need of FCSelection to know pickRadius altogether...

[DeepSOIC]

I'm thinking of setting pick radius for event action at the moment it is created...

I have first success regarding this!
in QuarterWidget, I added:

Code: Select all

  SoHandleEventAction* ea = this->pimpl->soeventmanager->getHandleEventAction();
  ea->setPickRadius(20.0);

And it works

so... rework in progress!

[DeepSOIC]

Reworked.

inconsistent default values for pick radius

Addressed. Set to 5 everywhere (I actually measured that currently it is r=5px).

weird design: View3DInventorViewer got the new member "pickRadius" but the argument list of "seekToPoint" has been extended to get the pick radius.Nicer design is to add the member to SoQTQuarterAdaptor, restore the interface of seekToPoint and add a getter and a virtual setter method for the pick radius. View3DInventorViewer then can override the virtual setter method to pass the radius to the selection node

Addressed. Whole implementation is now in SoQtQuarterAdaptor; View3DInventrViewer not changed at all.

[*]SoFCSelection:

FIXME: use viewer's pick radius instead, somehow

Addressed. Neither SoFCSelection nor SoFCUnifiedSelection mess with pick radius anymore, at all.

in the preferences there should be set a sensible upper and lower limit. Allowing a minimum of 0 and a maximum of 10000 definitely doesn't make any sense. Since the pick radius is in pixels a minimum value should be 1. A maximum could be e.g. 10 or maybe 20.

New limits: 0.5 to 200

[*]in several places it's checked whether the value is less than 0.001. As noted above it should be checked whether it's less than 1.0

Only one place remains: in Py implementation of setPickRadius. I want to leave the ability to set value to subpixel, for the sake of experimentation at least. Maybe it's even worth removing this artificial constraint altogether - it is py interface, not UI.

EDIT: known problems
1. In sketcher, constraints do not respect pick radius.
2. In sketcher, when pick radius is high, it is hard to select two points one after another, when they are close to each other. I tried to fix that by disabling Z elevation of selection/highlighting, it helps, but not much. Further changes are required...
3. It is still hard to select vertices on concave corners. I'm thinking on how to fix it.

[wmayer]

Merged.

Addressed. Set to 5 everywhere (I actually measured that currently it is r=5px).

Since that's the default of OpenInventor it's fine, too.

Addressed. Whole implementation is now in SoQtQuarterAdaptor; View3DInventrViewer not changed at all.

Great.

1. In sketcher, constraints do not respect pick radius.

The responsible class is SoDatumLabel.

2. In sketcher, when pick radius is high, it is hard to select two points one after another, when they are close to each other. I tried to fix that by disabling Z elevation of selection/highlighting, it helps, but not much. Further changes are required...

That's exactly the problem you get with a too high pick radius. IMO the only good option is to choose a sensible value for the pick radius. Setting a ridiculously high value and then trying to fix its side-effects is insane.

3. It is still hard to select vertices on concave corners. I'm thinking on how to fix it.

Have a look at SoFCUnifiedSelection::getPickedPoint. It internally checks a list of all elements and if a point is picked it wins.

[DeepSOIC]

Merged.

thanks!!

The responsible class is SoDatumLabel.

will check, but I don't feel lucky...

Setting a ridiculously high value and then trying to fix its side-effects is insane.

That's part of the goal to make FreeCAD touchscreen-friendly. At least I'm trying

Have a look at SoFCUnifiedSelection::getPickedPoint. It internally checks a list of all elements and if a point is picked it wins.

Been there. I have some idea how to improve it, but haven't tried yet. The idea is to convert pick radius into depth region (kinda create confusion sphere), an apply priority selection rules there...

[yorik]

That's part of the goal to make FreeCAD touchscreen-friendly. At least I'm trying

Now finally i see the point behind all this