Bugzilla – Bug 297

ParametrizedLine::intersection doesn't return intersection point

Last modified: 2011-12-10 12:19:54 UTC

Parametrized::intersection returns a scalar rather than a vector representing the point of the intersection. I imagine that this should be changed to return the intersection. However this would be a API breaking change for anyone dependent on the current behavior. We could perhaps add another function until the old behavior can be deprecated. I suggest Parametrized::intersectionPoint

I realized that problem recently, it's been bugging me, and just today I was thinking about exactly the same solution (add intersectionPoint method)! We can't break the API at all, so in this case there's little point in deprecating it. We can however file a tracking bug for all the things we'd like to break if we ever could, just in case (not before year 2042). FYI the return value is the parameter value 't' along the parametrized line.

would something like this be correct?: template <typename _Scalar, int _AmbientDim, int _Options> template <int OtherOptions> inline ParametrizedLine<_Scalar, _AmbientDim,_Options>::VectorType ParametrizedLine<_Scalar, _AmbientDim,_Options>::intersectionPoint(const Hyperplane<_Scalar, _AmbientDim, OtherOptions>& hyperplane) { _Scalar t = -(hyperplane.offset()+hyperplane.normal().dot(origin())) / hyperplane.normal().dot(direction()) ; return origin() + (direction()*t); } If that's ok, I'll attach a patch for it.

Created attachment 183 [details] Implements discussed feature Should do the trick. Let me know if there are any style issues, and I'll correct them.

Created attachment 184 [details] ultra simple test case Probably not enough for a unit test, but at least a basic test.

it would be better if intersectionPoint would use intersection to compute t, instead of duplicating the code.

btw, I agree that it would have been better to have intersection returns the intersection point, and add an intersectionParam rather than the other way round, but as Benoit said, it's too late now. It seems we are also lacking an "Vector evaluate(const Scalar& t) const" method. inetrsectionPoint could then be as simple as return evaluate(intersection(...));

> it would be better if intersectionPoint would use intersection to compute t, > instead of duplicating the code. Good point, though my instinct would be to pull out t into a third fuction called by both to fix the semantic confusion/naming issue at least internally. Not sure what to call it though other than just "t"? > It seems we are also lacking an "Vector evaluate(const Scalar& t) const" > method. > > inetrsectionPoint could then be as simple as > > return evaluate(intersection(...)); I can add evaluate along with this patch if there's agreement that this is desired.

> ...pull out t into a third fuction... > Not sure what to call it though other than just "t"? Sorry, brain still not functioning. Need more sleep. intersectionT perhaps.

Created attachment 187 [details] Candidate to address comments Attempt to address comments. More critiques welcome.

Created attachment 188 [details] Candidate fixed.

Created attachment 193 [details] Adds const specifier Fixes problem related to bug 311 ( http://eigen.tuxfamily.org/bz/show_bug.cgi? id=311 )

Comment on attachment 193 [details] Adds const specifier Review of attachment 193 [details]: ----------------------------------------------------------------- Chiming in very late in this conversation; sorry, I would really prefer different naming: ::: Eigen/src/Geometry/ParametrizedLine.h @@ +106,5 @@ > VectorType projection(const VectorType& p) const > { return origin() + direction().dot(p-origin()) * direction(); } > > + /** \rreturns the point at parameter t along the line */ > + VectorType evaluate( Scalar t ) const; How about point(Scalar t) instead? I know that 'evaluate' was proposed in an earlier comment but I really prefer point(). In Eigen we normally name functions after what they return, not after what they do. @@ +113,2 @@ > template <int OtherOptions> > + Scalar intersectionT(const Hyperplane<_Scalar, _AmbientDim, OtherOptions>& hyperplane) const; How about intersectionParameter instead?

Created attachment 233 [details] Patch with adjusted naming based on comments by Benoit This patch takes the comments into consideration. * I changed intersectionT -> intersectionParameter as requested * I changed evaluate -> intersectionPoint rather than to the suggested "point" as it seemed to follow the pattern better. Let me know if you prefer "point" over "intersectionPoint" and I'll be happy to modify the patch again.

Created attachment 234 [details] Ultra-simple test matched to new patch

Created attachment 235 [details] Patch with adjusted naming based on comments by Benoit Comment typo correction

patch applied and improved. I used pointAt(t). https://bitbucket.org/eigen/eigen/changeset/0e182249fb56/ changeset: 0e182249fb56 summary: feature 297: add ParametrizedLine::intersectionPoint() and intersectionParam() -> intersection() is deprecated https://bitbucket.org/eigen/eigen/changeset/e4a753eb68f7/ changeset: e4a753eb68f7 summary: feature 297: s/intersectionPoint/pointAt, fix documentation, add a unit test