getPickedPoint() and getPickedPointList() should not change picking condition

[VolkerEnderlein]

Original report by Wei Hong (Bitbucket: weihong_SCR, ).

---

In getPickedPoint() method, PickAll condition will be set to FALSE and redo the picking if the current PickAll condition is TRUE. This behavior is different from SGI Open Inventor. I assume this method should just return the first picked point if the picking is still valid. This method should not modify the PickAll condition, which should only be set by application.

Best, Wei

const SoPickedPoint SoHandleEventAction::getPickedPoint(void) { SoRayPickAction ra = PRIVATE(this)->getPickAction(); if (!PRIVATE(this)->pickvalid || PRIVATE(this)->didpickall) { ra->setPickAll(FALSE); PRIVATE(this)->doPick(ra); } return ra->getPickedPoint(); }

/! Returns a list of all intersection points below the mouse cursor. / const SoPickedPointList & SoHandleEventAction::getPickedPointList(void) { SoRayPickAction * ra = PRIVATE(this)->getPickAction(); if (!PRIVATE(this)->pickvalid || !PRIVATE(this)->didpickall) { ra->setPickAll(TRUE); PRIVATE(this)->doPick(ra); } return ra->getPickedPointList(); }

[VolkerEnderlein]

Original comment by Roy Walmsley (Bitbucket: walroy, GitHub: walroy).

---

Hi Wei,

Thanks for your comment.

I don't know about SGI behaviour on this. However, within SoRayPickAction the PICK_ALL flag is being dynamically used to control the picking process. The initial flag setting will be made by the application. It just get's changed dynamically depending on the circumstances.

Have you tried modifying your local copy of the code as you suggest and noting and behaviour changes? Or do you think that you are seeing incorrect behaviour in your application?

Wishing you a happy new year.

Roy

[VolkerEnderlein]

Original comment by Wei Hong (Bitbucket: weihong_SCR, ).

---

Hi Roy, I think it will be better if the PICK_ALL flag is not modified within these methods. This flag should only be set by application. getPickedPoint() just returns the first intersection point and getPickedPointList() just returns the whole list if the picking is valid. In the current way, if one node uses getPickedPoint() method and another node uses getPickedPointList() method, there will be a performance impact if you have a large scene graph.

Happy new year. Wei

[VolkerEnderlein]

Original comment by Roy Walmsley (Bitbucket: walroy, GitHub: walroy).

---

Hi Wei,

Thanks for your further comment. Please forgive me, but I am having some trouble understanding the significance of your observation. As I see it, these are methods on an action. And the action will be used by code external to the scene graph. But perhaps you could illustrate the problem with a simple example.

Also, I repeat my earlier questions. Have you tried modifying your local copy of the code as you suggest and noted any behaviour changes? Or do you think that you are seeing incorrect behaviour in your application?

Thanks again,

Roy

[VolkerEnderlein]

Original comment by Wei Hong (Bitbucket: weihong_SCR, ).

---

Hi Roy, I don't see any incorrect behavior. But I do see a performance issue in my application. The ray pick action has been triggered so many times for one handle event action. For example, let's say I have two nodes in my scene graph. Node A is calling getPickedPoint() to use the first intersection point, while Node B is calling getPickedPointList() to scan the whole list of the intersection points. In the application, the PICK_ALL flag to be true at beginning. When Node A is traversed, getPickedPoint() method will set PICK_ALL flag to false and redo the picking. While when Node B is traversed, getPickedPointList() method will set PICK_ALL flag to true and redo the picking again. When you have more nodes in the scene graph like these, the ray pick action will be triggered many times. You will see a performance issue. I have an application started from VSG inventor. After porting to Coin3D, I noticed this performance issue as described. This issue can be fixed if getPickedPoint() and getPickedPointList() don't check the PICK_ALL flag.

Thanks, Wei

[VolkerEnderlein]

Original comment by Roy Walmsley (Bitbucket: walroy, GitHub: walroy).

---

Thanks for your patience, Wei. I can now see the point you are making. Have you tried modifying your own local copy of the code? If so, I presume that solved the performance issue. Did the change have any other noticeable effect, in particular, any detrimental effects?

[VolkerEnderlein]

Original comment by Mario Koerner (Bitbucket: Renreok, GitHub: Renreok).

---

Hi Wei,

I also noticed the same problem when I implemented my own selection node. Alternating calls to SoHandleEventAction::getPickedPoint() and SoHandleEventAction::getPickedPointList() generate a SoRayPickAction each time, leading to bad performace for large scene graphs or nodes with complex geometry (e.g. large triangle meshes).

In my case I solved that by exclusively using getPickedPointList(), but that's of course not a good option for larger projects, where different nodes are developed independently (and shall smoothly run with the standard Inventor nodes offered by Coin).

So it would be great if the problem could be solved more generally. It also seems that VSG Inventor solves this in a smarter way. I think it is worth a try to always pick with PICK_ALL set to TRUE and to only return the first point from the list in getPickedPoint(). The SoRayPickAction traverses the whole scene graph in either way (because the foremost node could be the last in the graph). Only the sorting of the picked point list could take some time, but it should be short compared to the time for scene graph traversal.

Can you give it a try and report your findings?

Best regards, Mario

[VolkerEnderlein]

Original comment by Roy Walmsley (Bitbucket: walroy, GitHub: walroy).

---

Hi Wei and Mario,

Mario, thanks for your contribution.

I have been reviewing the code, and have the following proposal I would appreciate your comment on.

I note that, in the getPickedPointList() function, the test as to whether to redo the pick scene traversal is a) checking whether the pick is valid and b) checking that it hasn't already picked all. Both seem necessary. Bearing in mind that the initial call in the function is to PRIVATE(this)->getPickAction(), which may have instantiated a new action, setting the pick all flag to TRUE is a necessary step.

Similarly, in the getPickedPoint() function, the test as to whether to redo the pick scene traversal is a) checking whether the pick is valid and b) checking whether it has already picked all. I don't see the point of b). The actual call to the SoRayPickAction->getPickedPoint() simply returns the first point in the sorted list anyway (as in, point with index 0). So my first proposal is to remove test b).

Also, similar to the first case, if the call has to be made then the pick all flag is currently set to FALSE. But, perhaps it should be set to TRUE, as Mario suggests. There might be a performance impact if you're picking all when you don't need to. But, perhaps the saving is worth the cost. So my second proposal is to set the pick all flag to TRUE.

Regards,

Roy

[VolkerEnderlein]

Original comment by Wei Hong (Bitbucket: weihong_SCR, ).

---

Hi Roy, I am fine with your first proposal to remove the test b). For your second proposal, where do you want to set the pick all flag to TRUE?

Best, Wei

[VolkerEnderlein]

Original comment by Roy Walmsley (Bitbucket: walroy, GitHub: walroy).

---

Hi Wei, Thanks for the feedback. I was thinking that, in the getPickedPoint() function, I would change the setPickAll(FALSE) to setPickAll(TRUE). However, since that posting I am wondering whether to hold back on that, and just do the first proposal and then see how that changes the performance.

Regards,

Roy