Introduce Segment Intersection Primitive #778
Conversation
github-actions
bot
added
cmake
isVoid
added
2 - In Progress
isVoid
requested review from
harrism
and removed request for
trxcllnt and
jrhemstad
isVoid
added
3 - Ready for Review
…into feature/segment_primitives
There was a problem hiding this comment.
Docs only.
I do worry that operator< for vec_2d could be misused. The operators assume the vec_2ds are points. Using them on vectors might not do what users expect, since comparing points/vectors with < is not a canonical operation (one might expect to compare vectors by magnitude, for example). I don't expect you to change anything, I'm just making a note of this thought.
This definition is giving a total order to 2d points on a plane. Effectively lower left points are less than upper right points. cuSpatial currently still mixes up the concept of points and vectors and uses a single structure for both, which is problematic. I think we should introduce two separate type for these two distinct concepts. Still, there might be other ways how user want to sort points on a plane - I believe user can provide custom comparator for |
|
@harrism as discussed, I added additional code to condition large floating points. |
| // Must be on the same line, test if intersect | ||
| if ((a < c && c < b) || (a < d && d < b)) { | ||
| // Compute smallest interval between the segments | ||
| auto a_ = a, b_ = b, c_ = c, d_ = d; |
There was a problem hiding this comment.
How about just making this function pass a, b, c, d by value instead?
There was a problem hiding this comment.
I think the code is cleaner using pass by value.
cpp/include/cuspatial/vec_2d.hpp
Outdated
| * @brief Greater than operator for two 2D points. | ||
| */ | ||
| template <typename T> | ||
| bool CUSPATIAL_HOST_DEVICE operator>(vec_2d<T> const& lhs, vec_2d<T> const& rhs) |
There was a problem hiding this comment.
You could improve your compile time by making these operators hidden friends: https://www.justsoftwaresolutions.co.uk/cplusplus/hidden-friends.html
There was a problem hiding this comment.
Moved. I think a 2nd benefit of this is that, as @harrism pointed out, that the definition of these comparators is not canonical, and we want to avoid users who choose to use type convertible to vec_2d to misuse these operator. Making them hidden friend requires user to explicitly cast them before they can access these operators.
|
I have also replaced floating point |
harrism
added
c++
libcuspatial
|
I think the concern I raised in this comment is relevant. As I have found out in the on going intersection feature, using a custom struct can help make accessing element's type easier. |
|
@gpucibot merge |
#778 introduced segment intersection primitive, but there's a subtle bug in the code. This PR fixes that. Authors: - Michael Wang (https://github.com/isVoid) Approvers: - H. Thomson Comer (https://github.com/thomcom) - Mark Harris (https://github.com/harrism) URL: #808
Description
Closes #763 , this PR introduces device primitive for segment-segment intersection.
Checklist