Implement `shapeutil_visit_crossing_edge_pairs` by missinglink · Pull Request #135 · golang/geo

missinglink · 2025-04-01T20:48:41Z

This PR implements shapeutil_visit_crossing_edge_pairs which is required to resolve #108.

If this is something you'd be interested in merging then let me know.
All the tests (so far) are passing, it just needs to be cleaned up a bit.

missinglink · 2025-04-01T20:49:29Z

Worth noting an oddity between the CPP and Go implementations, the edge crossings Crossing enum is in a different order, numeric comparisons from the CPP code don't port to Go 🤷‍♂️

DoNotCross = -1
MaybeCross = 0
Cross = 1

const (
	// Cross means the edges cross.
	Cross Crossing = iota
	// MaybeCross means two vertices from different edges are the same.
	MaybeCross
	// DoNotCross means the edges do not cross.
	DoNotCross
)

jmr

We would like to add this functionality.

Can you explain your basic process for converting this? Is it a straightforward translation of the C++ code?

Where did you do something differently from C++?

jmr · 2025-04-02T06:04:10Z

s2/shapeutil_visit_crossing_edge_pairs.go

+// VisitCrossingEdgePairs finds all crossing edge pairs in an index.
+func VisitCrossingEdgePairs(index *ShapeIndex, crossingType CrossingType, visitor EdgePairVisitor) bool {
+	needAdjacent := crossingType == CrossingTypeAll
+	for it := index.Iterator(); !it.Done(); it.Next() {


I don't understand why this doesn't call VisitCrossings like the C++ does.

https://github.com/google/s2geometry/blob/58de4ea1e2f8a294e0c072c602c22232fd1433ad/src/s2/s2shapeutil_visit_crossing_edge_pairs.cc#L149

Two lines below.

It calls visitCrossings in a loop. Could this be a single call to VisitCrossings, like the C++ does? It looks like you're reimplementing VisitCrossings.

missinglink · 2025-04-02T07:54:57Z

Where did you do something differently from C++

The main obstacle to a 1:1 port are CPP function overloading, this necessitates some function renaming as Golang doesn't support overloading.

There are numerous differences, I'm not going to even try to list them all, most notably is the one I commented about above, different struct definitions between the two, use of pointers in CPP but values in Go, porting stdlib fiction calls, etc.

Functionally the code is the same, you can probably ask an AI to summarize if you're not comfortable reviewing by eye.

jmr · 2025-04-02T08:28:01Z

s2/shapeutil_visit_crossing_edge_pairs.go

+				crosser.RestartAt(b.Edge.V0)
+			}
+			sign := crosser.ChainCrossingSign(b.Edge.V1)
+			// missinglink: enum ordering is reversed compared to C++


I'm not sure what to get out of "missinglink:"? Is this

// Note that enum ordering is reversed compared to C++ // TODO(missinglink): Make them match.

?

jmr · 2025-04-02T08:28:18Z

s2/shapeutil_visit_crossing_edge_pairs.go

+}
+
+func FindCrossingError(shape Shape, a, b ShapeEdge, isInterior bool) error {
+	ap := shape.ChainPosition(int(a.ID.EdgeID))


Is there a reason not to have is_polygon in the error messages?

jmr · 2025-04-02T08:31:48Z

s2/shapeutil_visit_crossing_edge_pairs_test.go

+		polygon := makePolygon(tc.polygonStr, true)
+		testHasCrossingPermutations(t, polygon.loops, 0, tc.hasCrossing)
+	}
+}


How do these test cases match up with the C++ coverage? I don't see a FindSelfIntersection test.

TEST(GetCrossingEdgePairs, NoIntersectionsOneIndex) { TEST(GetCrossingEdgePairs, NoIntersectionsTwoIndexes) { TEST(GetCrossingEdgePairs, EdgeGridOneIndex) { TEST(GetCrossingEdgePairs, EdgeGridTwoIndexes) { TEST(FindSelfIntersection, Basic) {

rsned · 2025-04-02T08:33:24Z

s2/shapeutil_visit_crossing_edge_pairs.go

+
+import "fmt"
+
+type ShapeEdgeVector []ShapeEdge


should be package private since it's only used in private methods

rsned · 2025-04-02T08:35:34Z

s2/shapeutil_visit_crossing_edge_pairs.go

+
+import "fmt"
+
+type ShapeEdgeVector []ShapeEdge


Go slice types of types should generally be named as the plural of the type. e.g. shapeEdges []ShapeEdge.

(also Vector is used through s2 already as an X,Y,Z object, so the name would be somewhat confusing that the defined type has no ShapeEdge but no Vector part.)

rsned · 2025-04-02T08:36:55Z

s2/shapeutil_visit_crossing_edge_pairs.go

+	return true
+}
+
+// Visits all pairs of crossing edges in the given S2ShapeIndex, terminating


https://go.dev/doc/comment#func

Func comments should start with the name of the function. (here and all others)

rsned · 2025-04-02T08:41:10Z

s2/shapeutil_visit_crossing_edge_pairs.go

+
+type EdgePairVisitor func(a, b ShapeEdge, isInterior bool) bool
+
+// getShapeEdges returns all edges in the given S2ShapeIndexCell.


Replace all C++ type names with the Go counterparts in comments.

e.g. S2ShapeIndexCell -> ShapeIndexCell

rsned · 2025-04-02T08:42:57Z

s2/shapeutil_visit_crossing_edge_pairs.go

+	return true
+}
+
+func FindCrossingError(shape Shape, a, b ShapeEdge, isInterior bool) error {


should be private and needs the corresponding doc comments from c++

rsned · 2025-04-02T08:59:46Z

s2/shapeutil_visit_crossing_edge_pairs_test.go

+	}
+}
+
+func testHasCrossingPermutations(t *testing.T, loops []*Loop, i int, hasCrossing bool) {


add the c++ function comments to this

rsned · 2025-04-02T09:03:20Z

s2/shapeutil_visit_crossing_edge_pairs_test.go

+}
+
+func TestGetCrossingEdgePairsGrid(t *testing.T) {
+	kGridSize := 10.0


const gridSize = 10.0

rsned · 2025-04-02T09:04:44Z

s2/shapeutil_visit_crossing_edge_pairs_test.go

+		t.Errorf("Fail")
+	}
+}
+


There are none of the two index versions of the tests. Can you add a TODO for them to be added?

rsned · 2025-04-02T09:05:24Z

s2/shapeutil_visit_crossing_edge_pairs_test.go

+
+	index.Add(&shape)
+	if len(getCrossingEdgePairsBruteForce(index, CrossingTypeAll)) != 112 {
+		t.Errorf("Fail")


error messages should be more descriptive than just 'fail'

rsned · 2025-04-02T09:06:57Z

s2/shapeutil_visit_crossing_edge_pairs_test.go

+	"testing"
+)
+
+type EdgePair struct {


should be private

flwyd · 2025-04-05T06:25:40Z

s2/shapeutil_visit_crossing_edge_pairs.go

+	if isInterior {
+		if ap.ChainID != bp.ChainID {
+			return fmt.Errorf(
+				"Loop %d edge %d crosses loop %d edge %d",


Here and elsewhere: Error messages should start with lower-case, since they may be wrapped with more context (https://google.github.io/styleguide/go/decisions.html#error-strings)

flwyd · 2025-04-05T06:25:56Z

s2/shapeutil_visit_crossing_edge_pairs_test.go

+  // However, the vertical lines do not reach the top line as it curves on the
+  // surface of the sphere: despite "epsilon" those 9 are not even very close
+  // to intersecting. Thus 9 * 12 = 108 interior and four more at the corners
+  // when CrossingType::ALL is used.


CrossingTypeAll

flwyd · 2025-04-05T06:26:06Z

s2/shapeutil_visit_crossing_edge_pairs_test.go

+			index.Add(polygon)
+
+			if hasCrossing != FindSelfIntersection(index) {
+					t.Error("Test failed: expected and actual crossing results do not match")


Idiomatic Go style is like

if got := FindSelfIntersection(index); got != hasCrossing { t.Errorf("FindSelfIntersection got %v, want %v", got, hasCrossing) }

It would be nice to include something about the loops (FindSelfIntersection(%s)), though there isn't a loopToString textformat function yet.

Add shapeutil_visit_crossing_edge_pairs

db74c8c

jmr requested changes Apr 2, 2025

View reviewed changes

rsned reviewed Apr 2, 2025

View reviewed changes

flwyd reviewed Apr 5, 2025

View reviewed changes


		type EdgePairVisitor func(a, b ShapeEdge, isInterior bool) bool

		// getShapeEdges returns all edges in the given S2ShapeIndexCell.

Conversation

missinglink commented Apr 1, 2025

Uh oh!

missinglink commented Apr 1, 2025

Uh oh!

jmr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

missinglink commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

missinglink commented Apr 2, 2025 •

edited

Loading