Bug 297 - ParametrizedLine::intersection doesn't return intersection point
ParametrizedLine::intersection doesn't return intersection point
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Geometry
3.0
All All
: --- Unknown
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-15 05:08 UTC by Andy Somerville
Modified: 2011-12-10 12:19 UTC (History)
3 users (show)



Attachments
Implements discussed feature (1.23 KB, patch)
2011-06-15 06:32 UTC, Andy Somerville
no flags Details | Diff
ultra simple test case (551 bytes, text/x-c++src)
2011-06-15 06:57 UTC, Andy Somerville
no flags Details
Candidate to address comments (2.97 KB, patch)
2011-06-15 19:17 UTC, Andy Somerville
no flags Details | Diff
Candidate fixed. (2.97 KB, patch)
2011-06-15 19:30 UTC, Andy Somerville
no flags Details | Diff
Adds const specifier (3.10 KB, patch)
2011-07-01 03:55 UTC, Andy Somerville
jacob.benoit.1: review-
Details | Diff
Patch with adjusted naming based on comments by Benoit (4.04 KB, patch)
2011-12-10 08:12 UTC, Andy Somerville
no flags Details | Diff
Ultra-simple test matched to new patch (567 bytes, text/x-c++src)
2011-12-10 08:14 UTC, Andy Somerville
no flags Details
Patch with adjusted naming based on comments by Benoit (4.04 KB, patch)
2011-12-10 08:23 UTC, Andy Somerville
no flags Details | Diff

Description Andy Somerville 2011-06-15 05:08:44 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
Comment 1 Benoit Jacob 2011-06-15 05:45:24 UTC
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.
Comment 2 Andy Somerville 2011-06-15 06:13:40 UTC
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.
Comment 3 Andy Somerville 2011-06-15 06:32:56 UTC
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.
Comment 4 Andy Somerville 2011-06-15 06:57:30 UTC
Created attachment 184 [details]
ultra simple test case

Probably not enough for a unit test, but at least a basic test.
Comment 5 Gael Guennebaud 2011-06-15 09:46:11 UTC
it would be better if intersectionPoint would use intersection to compute t, instead of duplicating the code.
Comment 6 Gael Guennebaud 2011-06-15 09:50:17 UTC
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(...));
Comment 7 Andy Somerville 2011-06-15 18:51:43 UTC
> 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.
Comment 8 Andy Somerville 2011-06-15 19:00:48 UTC
> ...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.
Comment 9 Andy Somerville 2011-06-15 19:17:56 UTC
Created attachment 187 [details]
Candidate to address comments

Attempt to address comments. More critiques welcome.
Comment 10 Andy Somerville 2011-06-15 19:30:31 UTC
Created attachment 188 [details]
Candidate fixed.
Comment 11 Andy Somerville 2011-07-01 03:55:09 UTC
Created attachment 193 [details]
Adds const specifier

Fixes problem related to bug 311 ( http://eigen.tuxfamily.org/bz/show_bug.cgi? id=311 )
Comment 12 Benoit Jacob 2011-10-31 03:38:28 UTC
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?
Comment 13 Andy Somerville 2011-12-10 08:12:54 UTC
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.
Comment 14 Andy Somerville 2011-12-10 08:14:23 UTC
Created attachment 234 [details]
Ultra-simple test matched to new patch
Comment 15 Andy Somerville 2011-12-10 08:23:02 UTC
Created attachment 235 [details]
Patch with adjusted naming based on comments by Benoit

Comment typo correction
Comment 16 Gael Guennebaud 2011-12-10 12:19:54 UTC
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

Note You need to log in before you can comment on or make changes to this bug.