Skip to content

Define CS-specific behavior of algorithms only with strategy (part 2). #605

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

Merged
merged 23 commits into from
Jun 30, 2019

Conversation

awulkiew
Copy link
Member

@awulkiew awulkiew commented Jun 24, 2019

Followup of #533

The changes:

  • rtree may take a strategy as bgi::parameters<RtreeParams, Strategy>, not all possible operations are supported, only those required by other algorithms where the rtree is used, for other operations the default strategy is used as it was before. This needs further changes but I think it needs more general changes in strategies, i.e. introduction of "proper" umbrella strategies.
  • rescale_policy_type takes optional CSTag (previously CS-related behavior was defined by Geometry type).
  • the information about the CS taken from the strategy is propagated internally in various algorithms.
  • Point types are now optional in point_in_poly strategies (left as template arguments defaulted to void for backward compatibility).
  • getters for CS-specific strategies added to some strategies.

@awulkiew awulkiew added this to the 1.71 milestone Jun 24, 2019
Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. I am ok with merging.

@@ -453,7 +476,7 @@ class is_valid_polygon
// compute turns and check if all are acceptable
debug_phase::apply(3);

typedef has_valid_self_turns<Polygon> has_valid_turns;
typedef has_valid_self_turns<Polygon, typename Strategy::cs_tag> has_valid_turns;
Copy link
Member

Choose a reason for hiding this comment

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

why not typedefed as above or not typedef the above to look alike. Similar cases below. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, there is no specific cause. I think I prefer removing typedef above because it's used only in one place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! It is huge, and I could not check all details, but what I've seen looks to me. So I'm OK with merging this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I think I'll also add some tests for geometries with undefined CS before merging.

@awulkiew awulkiew force-pushed the feature/undefined_cs branch from 98d494d to b73a401 Compare June 28, 2019 21:30
@awulkiew
Copy link
Member Author

I addressed the issue raised by @vissarion, resolved the conflict with develop, added tests and fixed one more issue, i.e. passed the strategy into get_rescale_policy() which calls envelope() and expand(). I plan to merge it this weekend.

@awulkiew awulkiew force-pushed the feature/undefined_cs branch from 572f36a to 4a279fe Compare June 29, 2019 19:06
@awulkiew awulkiew merged commit cfb0f27 into boostorg:develop Jun 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants