-
Notifications
You must be signed in to change notification settings - Fork 223
Fix/issue 1410 repair dissolve #1411
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
base: develop
Are you sure you want to change the base?
Fix/issue 1410 repair dissolve #1411
Conversation
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.
The changes look good to me (make all calls consistent with the removal of rescale policies, replace svg mapper with geojson in testing). I noticed no new failures in the main test suite, as expected based on the changes.
Two things I noticed, though:
- line 13, run dissolve.cpp is still commented out in geometry/extensions/test/algorithms/Jamfile in your branch. If I run that test, I get 63 failures on my only tested configuration linux, aarch64 with gcc (7, 8, 9, 12, 13, star_{a, b, c}, mail_2017_09_24_{e,f}, mail_2017_09_30_a, reallife, gitter_2013_04_b, ggl_list_denis, mail_2018_08_19 all with types f and d). That does confirm that it compiles, though.
- The compilation issues of dissolve would have been caught by a minimal test. Would we want such a test as part of the CI for extensions which are intended for future inclusion and active usage?
Thanks for your remarks!
Indeed. The results are different than from before, I still have to work on that. That does confirm that it compiles, though. Indeed
Good question. We don't have them for extensions. They are meant to be
Also they should not be part of @vissarion what are your thoughts? |
487f5d3
to
dda5e9f
Compare
I first wanted to make a follow-up. But because it is not merged (neither approved) anyway, it can be done in this PR |
static bool const include_start_turn = true; | ||
}; | ||
|
||
template <typename Ring, typename PseudoVertices> |
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.
The structure (incl. alignment) of the original file is kept.
What is changed is:
template<
typename point_t = boost::geometry::model::d2::point_xy<double>,
typename ring_t = boost::geometry::model::ring<point_t>
>
-> (in this case, and also everywhere below)
template <typename Ring, typename PseudoVertices>
sometimes using point_type_t
, sometimes another small change. Such that it can be used for any type.
And some minor related changes.
i = pseudo_vertices.begin(); | ||
} | ||
|
||
// Repeat until back at starting point |
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 believe this while
can lead to infinite loops. I had it once when changing a thingy.
But in the current unit test, it is always fine.
} // impl | ||
|
||
template <typename Polygon, typename MultiPolygon, typename Area> | ||
inline void correct_non_zero(Polygon const &input, MultiPolygon &output, Area remove_spike_min_area = 0.0) |
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.
it is renamed because boost::geometry has already a function called correct
} | ||
|
||
template<typename Polygon, typename MultiPolygon, typename Area> | ||
inline void correct_odd_even(Polygon const &input, MultiPolygon &output, Area remove_spike_min_area = 0.0) |
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 function is not unit tested, in this PR (it can and gives other results obviously)
|
||
|
||
template <typename MultiPolygon, typename Area> | ||
inline void correct_non_zero(MultiPolygon const &input, MultiPolygon &output, Area remove_spike_min_area = 0.0) |
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 is an overload, and I kept it.
If we productize it we should use the normal dispatch
mechanism.
I will add some pictures later this weekend. |
dda5e9f
to
92ec5bf
Compare
…t count and float
92ec5bf
to
2301a68
Compare
This PR:
dissolve
betterdissolve
Test results:
original case
It is mostly as it was before. Note that the versions 1.85 did not run without errors either. Removing duplicate cases,
there are just two errors now.
using buffer
The expectations are a for some cases adapted, because the version using buffer always removes any interior ring.
*** No errors detected
using correct
The expectations are as in the original cases.
*** No errors detected