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

Box-segment distance for spherical and geographic coordinate systems #478

Merged
merged 39 commits into from Jul 4, 2018

Conversation

Projects
None yet
2 participants
@vissarion
Contributor

vissarion commented Apr 24, 2018

This PL implements the box-segment distance computation for spherical and geographic coordinate systems (CS).

The CS independent functionality is still in the algorithm which is generalized to support the new coordinate systems. The CS dependent functionality is implemented in a set of new strategies.

Along the way few fixes in envelope of segment and point-segment distance are performed.

Now, distance should be able to be computed between any pair of geometries and for any CS. A new PL will test this.

vissarion added some commits Dec 11, 2017

[algorithms] [tests] Fix distance segment-box for spehrical and geogr…
…aphic when segments is close to a box corner
[algorithms] [distance] Use the whole segment instead of starting poi…
…nt to vertex sub-segment for distance pt-seg computation

@vissarion vissarion changed the title from Feature box seg to Box-segment distance for spherical and geographic coordinate systems Apr 24, 2018

@vissarion vissarion added this to the 1.68 milestone Apr 24, 2018

@vissarion vissarion requested a review from awulkiew May 7, 2018

@awulkiew

Thanks for this PR!
I'm ok in general but I have few remarks about the structure of the code, interfaces of strategies etc. It's possible that I don't understand something and that the way how it's implemented is the best way. But for now I think strategies should be restructured slightly and that the responsibilities of various strategies are mixed up.

// 1 if disjoint (vertex not computed),
// 2 if disjoint (vertex computed)
template <typename Segment, typename Box, typename Strategy, typename P>
static inline std::size_t apply(Segment const& segment,

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

enum would be safer here, or taking output value of bool& as argument and return bool indicating that value was computed. Unless this integral value is used in some calculation while testing something but AFAIU it isn't.

diff1 = -diff1;
t_min1 = -t_min1;
t_max1 = -t_max1;
sign = -1;

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

It's a matter of preference but int sign = diff1 >= 0 ? 1 : -1; would be shorter.

@@ -334,7 +333,7 @@ class envelope_segment_impl
CalculationType lon2_rad = math::as_radian<Units>(lon2);
CalculationType lat2_rad = math::as_radian<Units>(lat2);
CalculationType alp1, alp2;
strategy.apply(lon1_rad, lat1_rad, lon2_rad, lat2_rad, alp1, alp2);
strategy.apply<true, true>(lon1_rad, lat1_rad, lon2_rad, lat2_rad, alp1, alp2);

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

I'm surprised that this compiles. Shouldn't it be strategy.template apply<true, true>(...)?
But anyway. What do you think about replacing template parameters with function overloads? E.g.:

strategy.apply(lon1_rad, lat1_rad, lon2_rad, lat2_rad, alp1, alp2);
strategy.apply(lon1_rad, lat1_rad, lon2_rad, lat2_rad, alp1);
strategy.apply_reverse(lon1_rad, lat1_rad, lon2_rad, lat2_rad, alp2);

or rather

T alp1 = strategy.apply(lon1_rad, lat1_rad, lon2_rad, lat2_rad);
T alp2 = strategy.apply_reverse(lon1_rad, lat1_rad, lon2_rad, lat2_rad);

or something similar. To make it more clear and to get rid of dummy argument. And in the latter case to encourage initialization of variable at the same line.

Btw, isn't it true that reverse azimuth should be equal to azimuth of segment with switched endpoints?

This comment has been minimized.

@vissarion

vissarion May 11, 2018

Contributor

I prefer the first proposed interface (which I didn't implement before because I wanted to stick on single "apply" name but I agree it is cleaner). The second interface above call two times the azimuth formula when both alph1 and alph2 needed. Regarding switched endpoints it is not always true and that is the reason for this change here. Sometimes you have to subtract/add pi etc but this is formula's job.

This comment has been minimized.

@awulkiew

awulkiew May 11, 2018

Member

The second interface only presents the two latter functions that are different. The one taking 2 output parameters would also be there. So no double calculation also in this case.

template <typename T>
static bool meridian_not_crossing_pole(T lat1, T lat2, T diff)
{
T half_pi = math::pi<T>()/T(2);

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

T const half_pi = math::half_pi<T>();
or even
static T const half_pi = math::half_pi<T>();

inline Strategy get_envelope_segment_strategy() const
{
return Strategy();
}

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

Strategy is Point-Point distance strategy passed into this Point-Segment distance strategy as template parameter.

  • get_distance_strategy() returns Pt-Pt distance strategy right? If that's the case then the name is ambiguous.
  • get_azimuth_strategy() should return the correct strategy, if there is no azimuth strategy in cartesian CS, or this strategy is not used, then this probably means that the code is not correctly divided into algorithm (CS-agnostic part) and strategy (CS-specific part) parts.
  • get_envelope_segment_strategy() it should be possible to return correct strategy here however the fact that you're ok with returning Pt-Pt distance strategy instead indicates that in cartesian CS this strategy is not used by the algrorithm somehow.

So this either means that the algorithm is dispatched by CS which is not correct or that these methods are required by some specific strategy, i.e. spherical and geographic Seg-Box strategies. I assume the latter is correct and this means that you moved the responsibility for some things to Pt-Segment strategy instead of doing those things in Seg-Box strategy. If azimuth and envelope is needed by a specific Seg-Box strategy it should be handled there, not here.

@@ -162,15 +162,15 @@ public :
<
Strategy, box_point_type1, box_point_type2
>::apply(ps_strategy, ps_strategy.get_distance_strategy()
.meridian(lat_min1, lat_max2));
.vertical_or_meridian(lat_min1, lat_max2));

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

Right, this function should be implemented in this strategy or some details, not in P/P strategy. It's neither needed in P/P algorithm nor P/S algorithm. It's only needed here in S/B strategy.

typedef envelope::spherical_segment<CalculationType> type;
};
inline typename envelope_segment_strategy::type get_envelope_segment_strategy() const

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

AFAIU this is not the place for these functions.

typedef envelope::spherical_segment<CalculationType> type;
};
inline typename envelope_segment_strategy::type get_envelope_segment_strategy() const

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

AFAIU this is not the place for these functions.

@@ -100,14 +100,16 @@ public :
return geometry::strategy::distance::services::result_from_distance
<
Strategy, Point, box_point_type
>::apply(ps_strategy, ps_strategy.get_distance_strategy().meridian(plat, lat_max));
>::apply(ps_strategy, ps_strategy.get_distance_strategy()
.vertical_or_meridian(plat, lat_max));

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

Same here.

template <typename T1, typename T2>
inline radius_type meridian(T1 lat1, T2 lat2) const
inline radius_type vertical_or_meridian(T1 lat1, T2 lat2) const

This comment has been minimized.

@awulkiew

awulkiew May 10, 2018

Member

Same here. Is this element of the interface requried by the distance P/P algorithm? If it's a CS specific optimization used in some other strategy it should be implemented there, not here.

@vissarion

This comment has been minimized.

Contributor

vissarion commented May 17, 2018

@awulkiew thanks for the comments.

I created an S/B distance strategy that provides all the functionality needed from the S/B distance algorithm; e.g. it provides the utilities segment_below_of_box and mirror.

Vertical_or_meridian method is moved to P/S strategy from P/P. This is the right place since it is used by higher order strategies like P/B and S/B that derive a P/S strategy.

Regarding the interface of strategies I changed it a bit in order to have a unique interface for spherical distance strategies (apart from P/P one) i.e. the number type as a first parameter and a P/P strategy as the second. In general I think it would make things clearer to have an "as unique as possible" interface for all distance strategies but that should be done in a separate PR.

};
disjoint_info(type t) : t_(t){}
operator type () const {return t_;}
type t_;

This comment has been minimized.

@awulkiew

awulkiew Jul 2, 2018

Member

Typically we put m_ before member variable.

This comment has been minimized.

@awulkiew

awulkiew Jul 4, 2018

Member

I saw that you did this change in merge commit. Not the best way but ok. Btw my point was not to name it m_ but m_t but ok this can be done later.

disjoint_vertex
};
disjoint_info(type t) : t_(t){}
operator type () const {return t_;}

This comment has been minimized.

@awulkiew

awulkiew Jul 2, 2018

Member

Why is this class needed if you're converting it to enum? Why not just use enum instead?

@awulkiew

This comment has been minimized.

Member

awulkiew commented Jul 2, 2018

Thanks! I'm ok with merging afer resolving conflicts.

@awulkiew awulkiew closed this Jul 2, 2018

@awulkiew awulkiew reopened this Jul 2, 2018

@awulkiew awulkiew merged commit f711e7b into boostorg:develop Jul 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment