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

LUCENE-8775: Tessellator: Improve the election of diagonals when splitting the polygon #652

Merged
merged 19 commits into from
Jun 6, 2019

Conversation

iverase
Copy link
Contributor

@iverase iverase commented Apr 23, 2019

This PR introduces:

  • Make sure that resulting polygons from splitting logic are valid CW polygons.

  • Make sure an error is thrown if a polygon cannot be splitted.

  • Adds several test to the tessellator.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with the tessellation logic, so I mostly left questions. :)

double a = Math.min(pNodeA.previous.getY(), pNodeA.next.getY());
double b = Math.min(pNodeB.previous.getY(), pNodeB.next.getY());
diff = a - b;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to look at previous and next nodes? Is it for the case that a vertex is shared by multiple holes, or by a hole and the enclosing polygon?

Copy link
Contributor Author

@iverase iverase Apr 26, 2019

Choose a reason for hiding this comment

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

This is the case when two holes share a vertex and that vertex is the left most one in each case. We need to order them so the one at the top is bigger than the one at the bottom.

In oder to do that we use the neighbour points. Probably it is enough to just use the next point but this is to work even in the case where holes are in different orientation.

}
next = next.next;
} while (next.next != connection);
return candidate;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can run in linear time with the number of vertices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is 0(n). It does not seem to degrade the performance of my tests.

@iverase
Copy link
Contributor Author

iverase commented May 21, 2019

The algorithm seems to have problems when a hole shares a vertex with the polygon. I added a change that reduces the likelihood of failing but there are still corner cases.

One way to address it is to merge those holes using the shared vertex before looking for the bridges. I have done so and it seems to work but performance is hurt so I need to find a better way of doing so.

@Darune
Copy link

Darune commented May 21, 2019

Any news on this ?

@iverase
Copy link
Contributor Author

iverase commented May 21, 2019

I am using the hole bounding box to filter out candidate vertex to be share between a polygon and a hole. With this change all the shapes I am working with tessellate properly and performance looks ok.

@iverase
Copy link
Contributor Author

iverase commented May 21, 2019

@nknize I think this PR is ready, finally changes are not very invasive and benefits are good as it seems to handle most of corner cases.

@nknize
Copy link
Member

nknize commented Jun 5, 2019

Sorry for the delay on this @iverase
This LGTM!

@iverase iverase merged commit 05ea0f2 into apache:master Jun 6, 2019
@iverase iverase deleted the tessellator branch June 6, 2019 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants