Skip to content
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

feat: major rewrite of traversal #1369

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

barendgehrels
Copy link
Collaborator

@barendgehrels barendgehrels commented Jan 29, 2025

As per #1366

This is a big PR. Current stats:
85 files changed, 3955 insertions(+), 6767 deletions(-)

Fixes:

Summary:

  • it started using Boost.Graph to determine biconnected components, instead of our own isolation detection
  • that's still there - but we can use it also for traversal
  • that needed some time but it is now in a good shape.

Tests (as also reported earlier - but these are runs of today):

  • random_bitset_grids:
    • develop: iterations: 100000 errors: 8 time: 48.776
    • this pr: iterations: 100000 errors: 0 time: 53.875 (better but still a bit slower, as reported in the discussion page)
  • recursive_polygons_buffer:
    • develop: geometries: 148743 errors: 307 type: d time: 8.806 (miter)
    • this pr: geometries: 149509 errors: 111 type: d time: 7.817 (miter)
    • but with join round it is different:
    • develop: geometries: 147743 errors: 518 type: d time: 8.541 (round)
    • this pr: geometries: 146474 errors: 762 type: d time: 7.875 (round)
    • I investigated some cases - they were perfect, these were (3 cases) false negatives from valid. We should fix them.
  • robustness_recursive_polygons
    • this pr: geometries: 1269839 errors: 9 type: d time: 86.551
    • develop: geometries: 1270000 errors: 0 type: d time: 92.921 - so a bit of regression here, should be investigated

Some things could not yet be fixed:

  • the behaviour of start turns (removing then when not necessary) is not yet OK. There is a "concept fix" included (not active)
  • the behaviour of get_turn_info is not always fine, especially where block_q is used. There is a "concept fix" included (not active)
  • the behaviour of segment ratio at arrival is not always fine, also here is a "concept fix" included
  • all these concept fixes should be worked out, as in this PR it's not fine everywhere. I can also omit it - but it is for your information
  • also, the buffer often considers a turn (of two pieces, say A and B) inside another piece (say C), and another (say: of A and C) as well (say: in B). This is conceptually not possible and should be fixed
  • and there are false negatives for invalidity, as mentioned above

@barendgehrels barendgehrels self-assigned this Jan 29, 2025
@barendgehrels barendgehrels force-pushed the major_feature/use-graph-for-traversal branch 2 times, most recently from 98eeaac to 15cc7ef Compare January 29, 2025 21:40
@barendgehrels
Copy link
Collaborator Author

With respect to:

They were perfect, these were (3 cases) false negatives from valid

So, in some cases, the previous version delivered one polygon for a cluster of UX/XU turns. The current delivers two, which makes sense, it's a touching point (a cluster). Though there is around 5.0e-16 between them - so if you consider it really close, it should be one polygon.

When it is changed (it's relatively easy), the statistics are:
# geometries: 149315 errors: 221 type: d time: 8.063 (round) (so, also better than in develop now)

PostGIS: it finds both variants fine.

SQL Server: it considers only the one with two polygons as valid. So clustered turn. The other is invalid.

image

@barendgehrels barendgehrels force-pushed the major_feature/use-graph-for-traversal branch 4 times, most recently from 2e61003 to 494bbd6 Compare January 30, 2025 21:37
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.

In general this is great, it simplifies things a lot and it seems that resolves several issues.
I have a few inline comments.
Moreover, have you tested with robust side strategy?

// The same for intersection - but it needs turns for the same target ahead check.
template <typename Operation, typename Turns>
bool is_better_collinear_for_intersection(Operation const& op, Operation const& other_op,
turn_operation_id const& toi, turn_operation_id const& other_toi, Turns const& turns)
Copy link
Member

Choose a reason for hiding this comment

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

I think indentation is inconsistent, one tab or two tabs or aligned with "(" in some cases.

@@ -439,6 +442,21 @@ struct buffered_piece_collection
detail::section::overlaps_section_box<Strategy>(m_strategy));
}

// This fixes the fact that sometimes wrong ix or xi turns are generated.
Copy link
Member

Choose a reason for hiding this comment

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

It is possible to not generate them at all for the buffer case? In which sense they are wrong? Are they also wrong for intersection/diffrence and how is this handled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for approval and all the comments!
I will address them next week, or next-next.
The question is good. In the meantime I forgot some details so I have recheck it.

return zone_counts;
}

void assign_zone_counts(std::vector<connection_item<point_type>>& item_vector, std::vector<std::size_t> const& zone_counts, std::size_t rank_size)
Copy link
Member

Choose a reason for hiding this comment

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

this is probably a too long line

// This can differ.
// It should be possible to fix it in another way.
void change_reversed_operations(signed_size_type const cluster_id, cluster_info const& cluster,
point_type const& point_turn, point_type const& point_origin)
Copy link
Member

Choose a reason for hiding this comment

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

indentation could be fixed here


if (OverlayType != overlay_buffer)
{
// Increase right-count for self-buffers. This should not be called for buffers
Copy link
Member

Choose a reason for hiding this comment

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

What is a self-buffer and what is the difference to a buffer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an error, I meant: self-intersections

auto it_target = state.vertex_map.find(target(*ei, graph));
if (it_source == state.vertex_map.end() || it_target == state.vertex_map.end())
{
// THIS SHOULD BE AN ERROR
Copy link
Member

Choose a reason for hiding this comment

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

should it raise an exception or something?

using kind = edge_property_tag;
};

// It appears that in an undirected graph, the components for two edges are sometimes different.
Copy link
Member

Choose a reason for hiding this comment

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

Could be useful to list a unit test name here where this happens, to be easily reproduced in the future.


if (op.enriched.ahead_side != other_op.enriched.ahead_side)
{
// If one of them goes left (1), this one is preferred above collinear or right (-1),
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reasoning for that tie breaking or is it just arbitrary?


if (! is_self_turn<OverlayType>(turn) && can_block)
{
turn_info_map[ring_id].has_blocked_turn = true;
continue;
}

Copy link
Member

Choose a reason for hiding this comment

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

trailing whitespace

std::size_t finalized_ring_size = boost::size(rings);
Turns, Clusters,
IntersectionStrategy
> trav(geometry1, geometry2, turns, clusters,
Copy link
Member

Choose a reason for hiding this comment

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

maybe traverse_graph is better than trav

@tinko92
Copy link
Collaborator

tinko92 commented Mar 5, 2025

I think this looks like a great set of changes. Due to the significant number of lines, I will need a lot of time to review it and hope to do it by ~mid April, I hope that works, assuming such a major change is not targetting the current release window.

@barendgehrels
Copy link
Collaborator Author

In general this is great, it simplifies things a lot and it seems that resolves several issues. I have a few inline comments. Moreover, have you tested with robust side strategy?

Yes, I tested with robust side strategy - especially in the parts of determining open space - but I don't see (big/any) difference. I did it halfway, when not all was ready. I can test again.

@barendgehrels
Copy link
Collaborator Author

I think this looks like a great set of changes. Due to the significant number of lines, I will need a lot of time to review it and hope to do it by ~mid April, I hope that works, assuming such a major change is not targetting the current release window.

Yes, that works fine, thanks. Indeed it's so major that we cannot target 1.88 anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants