Umbrella strategies improvements #855
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…the implementation. Use custom versions of enable_if for distance() getters. Use default keyword in default ctors. Remove unneeded strategy_converter specialization.
vissarion
approved these changes
May 28, 2021
There was a problem hiding this comment.
Todo (probably in a different PR or in this one if you insist):
* Removal of strategy includes in tests when they are not needed (since the defaults are included with algorithms) * There is still one umbrella strategy missing for `remove_spikes` (`convex_hull` strategy could be reused for that, e.g. as an alias) * Aliases of umbrella strategies for all algorithms so they are not confusing and the user knows that e.g. `within` algorithm expects `within` strategy (right now `relate` strategy is used for all relops and setops). * Relocation of legacy strategies into `strategy/` directory
I am OK with leaving them for separate PR.
Question:
* should we use `side_robust` strategy in all algorithms by default, e.g. also in relops and setops?
We should at least try. I can prepare a PR.
| // longitude difference was lesser than 360 (otherwise depending on the CS there would be | ||
| // no solution or there would be two possible solutions - segment going through one of | ||
| // the poles, at least in case of oblate spheroid, either way the answer would probably | ||
| // be "false"). |
barendgehrels
approved these changes
Jun 2, 2021
|
Thanks for the reviews. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the following changes:
is_simpleandis_validis_convexumbrella strategy is added and supported in the algorithm (using the same side strategy asconvex_hull)is_convexfor Polygon is implementedget_point_in_geometry_strategy()) are removed from legacy strategiesTodo (probably in a different PR or in this one if you insist):
remove_spikes(convex_hullstrategy could be reused for that, e.g. as an alias)withinalgorithm expectswithinstrategy (right nowrelatestrategy is used for all relops and setops).strategy/directoryQuestion:
side_robuststrategy in all algorithms by default, e.g. also in relops and setops?