EdgeQuery.initCovering: Fix bug causing skipped faces

[benguild]

I was doing some benchmarking and testing with this and found that seemingly due to a break statement it's skipping entire faces.

Pretty bad bug. Added a test that fails and is now fixed by removing the break.

I'm taking a look at the TODO right now that's there but wanted to post the simplest change and test first.

[benguild]

If I'm understanding this correctly… the loop condition id != lastID correctly excludes the last face (which is handled by addInitialRange(next, last) after the loop). However, the break inside the loop contradicts this intent by exiting after only the first face instead of visiting each face?

[jmr]

It's hard for me to understand what's wrong. What test case was failing before? Can you minimize the test case to just that? Then it's easier to analyze.

I also don't get much out of the PR description other than "something's broken". https://google.github.io/eng-practices/review/developer/cl-descriptions.html

Did you look at the C++ implementation / try the test cases there?

[benguild]

It's hard for me to understand what's wrong.

If you read the comments in the test, it very clearly explains that it's verifying that the results from the "optimized" approach are the same as the "brute force" one.

What's wrong is in the first sentence of the PR:

it's skipping entire faces.

I'm about to commit a resolution for the TODO that was in the code there as well, so I'm keeping these commits separate, but both pass the test where the current code does not… and unless skipping faces is intended functionality or I'm missing something here, this may be breaking other cross-face implementations.

[jmr]

1. Did you compare to C++?
2. Can the test be further minimized?

[jmr]

It looks like this cap is only used for its radius.

[benguild]

f57cef1

[benguild]

1. Did you compare to C++?

The bug is not in C++, it doesn't have the break statement…
https://github.com/google/s2geometry/blob/e0de7f7a18f77ac86cbaaf3daefddbdc8051661a/src/s2/s2closest_edge_query_base.h#L938-L947

[jmr]

@rsned added in a7649ee.

[jmr]

What's wrong is in the first sentence of the PR:

it's skipping entire faces.

Which face(s) are missing?

[benguild]

What's wrong is in the first sentence of the PR:

it's skipping entire faces.

Which face(s) are missing?

I think I answered that here: #247 (comment)

[jmr]

I think I answered that here: #247 (comment)

No. Maybe it's simpler if you just paste the test output with the break.

[benguild]

Maybe it's simpler if you just paste the test output with the break.

     edge_query_test.go:598: Optimized found 1 shapes, brute force found 4 shapes
     edge_query_test.go:609: Optimized algorithm missed shapes that brute force found: [Face1 Face3 Face4]
     edge_query_test.go:616: Optimized found 1 shapes, brute force found 4 shapes; should be equal

8010dfb updates this to be the same as the C++ version, the test passes as do the others. I went ahead and removed the test since mirroring it upstream to C++ which doesn't have the bug wouldn't add any value, but we can add it back in if you want.

https://github.com/google/s2geometry/blob/master/src/s2/s2closest_edge_query_base.h#L926-L950

I also fixed the comment typo on the C++ repo: google/s2geometry#490

[jmr]

I went ahead and removed the test since mirroring it upstream to C++ which doesn't have the bug wouldn't add any value, but we can add it back in if you want.

Please add the test back.

The test clearly adds value to the Go library.

Adding the test to C++ would improve test coverage there. C++ tests fail in debug mode when adding the break, but not in opt mode, so we're missing something.

[jmr]

When you add the test back, can you use two shapes? Can you make the old code fail with just one?

[benguild]

When you add the test back, can you use two shapes? Can you make the old code fail with just one?

Hmm, I think the minimum is 4 shapes across 4 faces (finds 1/4) or 5 shapes as 3+1+1 for example across 3 faces (finds 3/5) …?

I turned this into a table and tried to come up with some other combinations in 6a3ea40. Some fail pre-fix, some don't. Before removing break …

=== RUN TestEdgeQueryOptimized
--- PASS: faces_0,4
--- PASS: faces_0,4_(3+1_shapes)
--- PASS: faces_0,1,4
--- FAIL: faces_0,1,4_(3+1+1_shapes): optimized found 3 shapes, brute force found 5
--- FAIL: faces_0,1,3,4: optimized found 1 shapes, brute force found 4
--- FAIL: all_6_faces: optimized found 1 shapes, brute force found 6
--- PASS: faces_1,3,4
--- FAIL: faces_1,2,3,4: optimized found 1 shapes, brute force found 4
FAIL

After removing break to match C++ …

=== RUN TestEdgeQueryOptimized
--- PASS: faces_0,4
--- PASS: faces_0,4_(3+1_shapes)
--- PASS: faces_0,1,4
--- PASS: faces_0,1,4_(3+1+1_shapes)
--- PASS: faces_0,1,3,4
--- PASS: all_6_faces
--- PASS: faces_1,3,4
--- PASS: faces_1,2,3,4
PASS

@jmr Are you perhaps able to draft a similar test for C++? I don't have the env setup right now to really dive into C++ on this machine so I'd have to get back to it later.

[rsned]

I don't recall why the break was added there when I wrote it 5 years ago. Looking at the C++ from approximately the same time there wasn't a break there either.

https://github.com/google/s2geometry/blob/98f435222f24cd3865619f2df9e02f630b8d5d53/src/s2/s2closest_edge_query_base.h#L814

I think removing the break is reasonable especially if it also helps resolve some broken tests.

[benguild]

Thanks @rsned. If someone approves the changes, we could merge.