Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intersection method of a Line. #2930

Closed
wants to merge 6 commits into from
Closed

Intersection method of a Line. #2930

wants to merge 6 commits into from

Conversation

akshayah3
Copy link
Contributor

Gives wrong result in the following case.
a = Ray(Point(0,0),Point(0,4))
b = Ray(Point(0,1),Point(0,-1))
a.intersection(b)
Ray(Point(0,0),Point(0,4))
But the result should be
Segment(Point(0,0),Point(0,1))

@smichr
Copy link
Member

smichr commented Feb 18, 2014

The test that identified this should be added to the test suite.

@akshayah3
Copy link
Contributor Author

@smichr There is no test for this currently, I found the drawback while looking at the code(For rays it checked the xdirection whereas we have to check both x and y directions respectively).I will add some tests in the test suite.

@smichr
Copy link
Member

smichr commented Feb 19, 2014

Does the following work? (I suspect that the 2nd test regarding source is not sufficiently general.)

z = Point(0, 0)
for o in (1,4),(-1,4),(-1,-4),(1,-4):
  o=Point(*o)
  p = o/2
  assert Ray(z,o).intersect(Ray(z, p)) == Ray(p,o)
  assert Ray(z,o),intersect(Ray(p,z)) == Segment(z, p)

@akshayah3
Copy link
Contributor Author

@smichr It is enough because it is not the full code because it just checks for the cases when the rays are similar or when they are parallel.

a1, b1, c1 = self.coefficients
a2, b2, c2 = o.coefficients
t = simplify(a1_b2 - a2_b1)
if t.equals(0) is not False: # assume they are parallel

@smichr
Copy link
Member

smichr commented Feb 19, 2014

I think the lines are parallel in every case; you can assert that with Ray(z,o).is_parallel(Ray(z, p) (or something to that effect). So the code should work. Can you modify it to do so?

@akshayah3
Copy link
Contributor Author

@smichr I think there is a bit of misunderstanding, I already modified the code and it works for the above cases you mentioned.

@smichr
Copy link
Member

smichr commented Feb 19, 2014

it works

It seems like you are assuming it work. Please post your results. I get

>>> z = Point(0,0)
>>> for o in (1,4),(-1,4),(-1,-4),(1,-4):
...   o=Point(*o)
...   p = o/2
...   assert Ray(z,o).intersection(Ray(z, p)) == Ray(p,o)
...   assert Ray(z,o),intersection(Ray(p,z)) == Segment(z, p)
...
Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
AssertionError

@akshayah3
Copy link
Contributor Author

@smichr Use [Segment(z, p)] because in the code it returns like that.Same for the Ray too.

@smichr
Copy link
Member

smichr commented Feb 19, 2014

Note: the code I gave had some errors itself. The following highlights the problem:

>>> for o in (1,4),(-1,4),(-1,-4),(1,-4):
...   o=Point(*o)
...   p = o/2
...   assert Ray(z,o).intersection(Ray(p,o)) == [Ray(p,o)]
...   assert Ray(z,o),intersection(Ray(p,z)) == [Segment(z, p)]
...
Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
AssertionError
>>> o
Point(-1, 4)
>>> Ray(z,o).intersection(Ray(p,o)),[Ray(p,o)]
([Ray(Point(0, 0), Point(-1, 4))], [Ray(Point(-1/2, 2), Point(-1, 4))])

The raw code is

z=Point(0,0)
for o in (1,4),(-1,4),(-1,-4),(1,-4):
  o=Point(*o)
  p = o/2
  assert Ray(z,o).intersection(Ray(p,o)) == [Ray(p,o)]
  assert Ray(z,o),intersection(Ray(p,z)) == [Segment(z, p)]

print o

@akshayah3
Copy link
Contributor Author

@smichr Have a look now, It's working.

@smichr
Copy link
Member

smichr commented Feb 19, 2014

It might be helpful to draw a picture

>>> Ray((0,0),(1,1)).intersection(Ray((-2,-2),(-1.8,-1.8)))
[Ray(Point(-2, -2), Point(-9/5, -9/5))]

That should be [Ray((0,0),(1,1))]. Perhaps you want to see if o.contains(self.source) then return self else return o?

@smichr
Copy link
Member

smichr commented Feb 19, 2014

btw, stating it the way I did will guarantee that self is returned in the case that both share the same source

@akshayah3
Copy link
Contributor Author

@smichr Sorry I got a little confused,But I think this does the job?
if self.xdirection == o.xdirection and self.ydirection == o.ydirection: if self.source in o:
return [self]
return [o]
With indentation.

return [o]
return [self]
return []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should raise an error in this case -- we already know that the rays are coincident. In fact, I would be surprised if this ever got triggered. I think return [self] if (self.source in o) else [o] is fine. Did you find otherwise?

@akshayah3
Copy link
Contributor Author

@smichr I have made the changes.

@smichr
Copy link
Member

smichr commented Feb 20, 2014

I opened modifications at #2940 -- can you take a look

@smichr smichr closed this Feb 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants