Hi Robert, Thanks for taking time to review this submission.
The pruning of the last element is indeed unsafe. I didn't know whether I should protect against this, as the function doesn't really make sense if you give it an empty node path. Given that the function implementation was already unsafe (see line 1932: nodePath.back()->accept(iv);), I chose to do nothing about it. A simple if (nodePath.empty()) return false; at the start of the function might do it? I'll leave that decision up to you. About the change at line 99. I didn't do that one, it is preMult() in OSG 2.6.1 (the version I'm submitting against). From what I can see from SVN, preMultScale() got introduced into the trunk after 2.6.1. Regards, Tanguy -----Original Message----- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Robert Osfield Sent: Wednesday 10 December 2008 10:26 To: OpenSceneGraph Submissions Subject: Spam: Re: [osg-submissions] FIX: osg::View::computeIntersection with nodepath Hi Tanguy, I'm just reviewing your changes. The const changes is good and I'll go ahead an merge this. The change from to use computeLocalWorldToLocal() also looks sounds as the code later does an inverse of the associated matrix to get from window coords into local coords. The pruning of the last element in the nodepath also looks the sensible thing to do, but the way you've coded it doesn't look safe to me, as it won't handle the case of nodepath being empty. I'll refactor this code to automatically detect such cases. The change at line 99 looks redundant - changing from using matrix.preMultScale to preMult(Matrix::scale()), if it it's it suggests a bug in preMultScale that needs to be addressed. Is this change just an experiment to see if made a difference or did you really see a bug with the original code at line 99? Robert. On Fri, Dec 5, 2008 at 2:33 PM, Tanguy Fautre <[EMAIL PROTECTED]> wrote: > Hi, > > This is based on OSG 2.6.1. > > bool View::computeIntersections( > float x,float y, > osg::NodePath& nodePath, > osgUtil::LineSegmentIntersector::Intersections& intersections, > osg::Node::NodeMask traversalMask) > > is currently, as far as I can tell, broken in several ways: > > - Matrix computation is incorrect, it should be computeLocalToWorld() > and not computeWorldToLocal(). > - Last node of the node path should be ignored; otherwise it can lead to > applying a transformation node twice. > - nodePath argument should be const > > > I'm attaching an updated version that fixes these 3 issues. I've tested > it on a complex scenegraph featuring several level of transformations, > and it is now working fine. > > > Regards, > > Tanguy > > > _______________________________________________ > osg-submissions mailing list > [email protected] > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org > > _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
