-
Notifications
You must be signed in to change notification settings - Fork 214
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
Support covered_by() for box, geometry combinations #1046
Support covered_by() for box, geometry combinations #1046
Conversation
box_type box_areal; | ||
geometry::envelope(geometry, box_areal, strategy); | ||
return strategy.covered_by(box_areal, box).apply(box_areal, box); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach while giving the correct result is suboptimal because it cannot be interrupted if e.g. one of the points/segments of a geometry is outside the box. The whole envelope is calculated each time. If that's acceptable then ok. But if it isn't but for whatever reason you decide to not implement this now maybe you could put TODO here and create another issue about this.
using is_cartesian = std::is_same | ||
< | ||
typename traits::coordinate_system<typename traits::point_type<Box>::type>::type, | ||
cs::cartesian | ||
>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably use Strategy::cs_tag
to distinguish between CSes. It's because the CS of a geometry may be undefined and it's the Strategy that defines CS-related operations.
BOOST_GEOMETRY_STATIC_ASSERT_FALSE( | ||
"Not implemented for this coordinate system.", | ||
typename traits::coordinate_system<typename traits::point_type<Box>::type>::type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change? AFAIU previously box was silently converted to ring in non-cartesian CSes, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are OK since this operation was not supported in the past, i.e. covered_by(box, polygon)
do not compile in any cs before this PR.
@awulkiew thanks for your comments I addressed them by pushing new commits. |
@barendgehrels are you OK with this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some comments, still pending, but basically I'm OK. Thanks!
return strategy.covered_by(box1, box2).apply(box1, box2); | ||
box_type box_areal; | ||
geometry::envelope(geometry, box_areal, strategy); | ||
return strategy.covered_by(box_areal, box).apply(box_areal, box); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fox box/box , calculating the envelope is actually not necessary.
For box/point neither.
Maybe the replaced code should still stay, and this new case to support other combinations.
You covered that already below, fine.
} | ||
}; | ||
using point_type = typename point_type<Geometry>::type; | ||
using box_type = model::box<point_type>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should use a helper geometry here, in case point_type is not mutable
See my review remark, basically yes, but can you look at my comments I made earlier? |
Thanks for the review! Somehow your comments appeared to me yesterday. I addressed them and now I can merge this PR. |
This PR implements support for
covered_by()
for the following combinations of a box and a geometry:convered_by(box, areal)
(only cartesian CS)convered_by(geometry, box)
The first case resolves issue #604