Add incidentEdgeTracker and indexCellData types and tests #180

rsned · 2025-05-15T21:16:58Z

Add incidentEdgeTracker and indexCellData types and tests.
Update LaxPolygon to support proper vertex ordering on holes and shells.
Fix comments and variable names in LaxPolygon.
Add more of the missing LaxPolygon tests to validate fixes.

These are ports of the corresponding C++ s2/internal/ types that are needed for ValidationQuery for Loops and Polygons.

This PR works towards issues #72, #108.

These are ports of the C++ s2/internal/ types that are needed for ValidationQuery for Loops and Polygons. Work for issues golang#72, golang#108.

Clean up some comments and variable names. Update some of the methods to match current C++ logic. Add more unit tests for multiloop polygons to validate that holes are inverted ordering.

alan-strohm

Sending some initial thoughts since you've been waiting a while and some comments might need discussion. I'll look more tomorrow. Thanks for doing this!

alan-strohm · 2025-05-27T19:18:33Z

s2/incident_edge_tracker.go

@@ -0,0 +1,173 @@
+// Copyright 2025 The S2 Geometry Project Authors. All rights reserved.


I assume we need to resolve this discussion about what to do with Copyrights before proceeding: https://github.com/golang/geo/pull/178/files#r2094005567

alan-strohm · 2025-05-27T19:32:08Z

s2/incident_edge_tracker.go

+}
+
+// vertexEdge is a tuple of vertex and edgeID for processing incident edges.
+type vertexEdge struct {


In C++ this concept is private to the tracker. The s2 package namespace seems pretty big and "vertexEdge" very generic. Do we want to be more careful about adding concepts to it?

Options:

Leave it be until we get a collision.

Switch to an anonymous struct. addEdge becomes pretty painful but maybe it's worth it.

use a longer name vertexEdgeForIncidentTracking?

I lean towards 2. but I don't feel strongly.

Since this is also coming up in index_cell_data.go I think we should consider a 4th option as well:

Move this code to an "internal" package so that it can have a public interface without being a public part of the s2 package.

There are a ton of these little pair typedefs throughout the C++, many of which end up as int, int or int32, int32.

// 7 std::pair<Distance, Id <--- typedef int
// 1 std::pair<double, double
// 1 std::pair<InputVertexId, InputVertexId <--- typedef int32_t
// 2 std::pair<int32_t, int32_t
// 1 std::pair<int, bool
// 1 std::pair<int, Candidate*
// 1 std::pair<int, EdgeId <--- typedef int32_t
// 1 std::pair<int, int
// 1 std::pair<int, uint64_t
// 1 std::pair<S2CellId, InputVertexId <--- typedef int32_t
// 14 std::pair<S2CellId, S2CellId
// 1 std::pair<S2Point, int
// 5 std::pair<S2Point, S2Point
// 2 std::pair<ShapeEdgeId, ShapeEdgeId <--- typedef int32_t
// 1 std::pair<SiteId, SiteId <--- typedef int32_t
// 4 std::pair<TouchType, TouchType <--- typedef int
// 8 std::pair<S2Shape, int64_t
// 1 std::pair<VertexId, VertexId <--- typedef int32_t

Would it be easier to define a simple generic type "Pair" and then use that where its needed instead of making these 30+ one-off types? e.g.,

type pair[T1, T2 any] struct {
first T1
second T2
}

...

type incidentEdgeTracker struct {
currentShapeID int32
nursery []pair[Point, int32] <--- (was nursery []pointEdgeSomething)
}

And then

// Add non-degenerate edges to the nursery. t.nursery = append(t.nursery, pair[Point, int32]{first: e.V0, second: edgeID}) if !e.IsDegenerate() { t.nursery = append(t.nursery, pair[Point, int32]{first: e.V1, second: edgeID}) }

....

The problem with moving this to "internal" is that it still requires all the types and test helpers from the s2 package which would make circular dependency chain.

$ go build .

github.com/golang/geo/s2/internal

./incident_edge_tracker.go:20:10: undefined: Point
./incident_edge_tracker.go:45:9: undefined: Point
./incident_edge_tracker.go:89:17: undefined: Point
./incident_edge_tracker.go:115:55: undefined: Edge
./index_cell_data.go:40:2: undefined: Edge
./index_cell_data.go:85:13: undefined: ShapeIndex
./index_cell_data.go:86:13: undefined: ShapeIndexCell
./index_cell_data.go:87:12: undefined: CellID
./index_cell_data.go:93:13: undefined: Cell
./index_cell_data.go:95:13: undefined: Point

Ack. Let's take option 4 off the table. I'm guessing we want one of 1-3 for this pull request. I don't feel strongly between them. As long as you thought through all 3 options and are happy with your recommendations, let's go with your recommendation.

How many of those pairs already have struct versions due to previous porting? If there are still a lot of pairs remaining, I can start a discussion documenting options 1-3 and seeking feedback on your suggestion of a generic as a more ergonomic form of 2.

alan-strohm · 2025-05-27T20:13:36Z

s2/incident_edge_tracker.go

+
+package s2
+
+// incidentEdgeKey is a tuple of (shape id, vertex) that compares by shape id.


Nit: id -> ID throughout.

alan-strohm · 2025-05-27T20:15:34Z

s2/incident_edge_tracker.go

+// add edges for that shape, and ends with finishShape(). Those sequences do
+// not need to visit shapes or edges in order. Then, call incidentEdges() to get
+// the resulting map from incidentEdgeKeys (which are shapeId, vertex pairs) to
+// a set of edgeIds of the shape that are incident to that vertex..


Nit: remove duplicate period.

alan-strohm · 2025-05-27T20:18:22Z

s2/incident_edge_tracker.go

+// shape's edges may be defined with multiple sequences of startShape(),
+// addEdge()... , finishShape() calls.
+//
+// The reason for this is simple: most edges don't have more than two incident


Is this supposed to read "most vertices don't have more than two incident edges"? I'm having a hard time following these two paragraphs and I don't think it' s just my lack of subject-matter experience...

If we're OK with more divergence with the C++, I think it'd be clearer to make the 3rd paragraph about how we only look for vertices with 3 or more incident edges and that we don't remember vertices with fewer incident edges after a call to finishShape. Then in the 4th (and 5th if necessary) paragraph, I'd talk about how "a single shape's edges may be defined with multiple sequences..." as long as the caller ensures that all the edges on a given vertex are handled in the same sequence of calls via something like ShapeIndex cells.

But if you want to stick to correcting obvious errors, that seems like a very reasonable choice.

alan-strohm · 2025-05-27T20:21:58Z

s2/incident_edge_tracker.go

+// To use, instantiate and then add edges with one or more sequences of calls,
+// where each sequence begins with startShape(), followed by addEdge() calls to
+// add edges for that shape, and ends with finishShape(). Those sequences do
+// not need to visit shapes or edges in order. Then, call incidentEdges() to get


"Then, call incidentEdges()"... there's no function with that name. Update the sentence to describe golang usage. "Then, read the edgeMap member to get..."

alan-strohm · 2025-05-27T20:24:49Z

s2/incident_edge_tracker.go

+
+	nursery []vertexEdge
+
+	// We can and do encounter the same edges multiple times, so we need to


This comment should describe the data structure first. The C++ says "Map from keys of (shape id, vertex) to a set of edge_ids." That works fine for me. However, I'd find it helpful to also mention that this only includes vertices with 3 or more incident edge IDs.

alan-strohm · 2025-05-27T20:38:19Z

s2/incident_edge_tracker.go

+// all the edges in the same cell as the vertex, and, in general, it's possible
+// to aggregate edges before calling.
+//
+// The tracker maintains incident edges until it's cleared. If you call it with


nit: s/clear/reset/ since that's the name of the function and the word "clear" doesn't appear anywhere else in this file.

alan-strohm · 2025-05-27T20:50:37Z

s2/incident_edge_tracker.go

+// adds it second endpoint as well.
+func (t *incidentEdgeTracker) addEdge(edgeID int32, e Edge) {
+	if t.currentShapeID < 0 {
+		return


The error handling across these functions is pretty bad. Here, you silently drop edges. There's no checking to ensure that calls are made in the right sequence. Should we re-examine the API to make erroneous usage more difficult? All the internal calls to the C++ version look like:

incident_edge_tracker_.StartShape(shape_id); for (const auto& edge : CurrentCell().shape_edges(shape_id)) { incident_edge_tracker_.AddEdge(edge.id, edge); } incident_edge_tracker_.FinishShape();

So we could have an interface that looks like:

// Add all edges to the tracker. After calling, any vertices with multiple (> 2) incident // edges will appear in the incident edge map. Returns an error if any edges have a different // shape ID than shapeID. func (t *incidentEdgeTracker) addShapeEdges(shapeID int32, edges []ShapeEdge) error

This would also remove the need for a "nursery" and the associated type, resolving one of my other comments.

alan-strohm · 2025-05-27T23:06:43Z

s2/incident_edge_tracker.go

+	}
+}
+
+// startShape is used to start adding edges to the edge tracker. After calling,


This comment is copied from the C++ where the scope is ambiguous. "After calling" more accurately describes finishShape. If we stick to this API, let's move the comment there.

alan-strohm

OK, this should be a complete set of comments.

alan-strohm · 2025-05-28T18:21:21Z

s2/incident_edge_tracker_test.go

+		index string
+		want  int
+	}{
+		// These shapeindex strings came from validation query's test


Would it be better to align these tests with https://github.com/google/s2-geometry-library-java/blob/master/tests/tests/com/google/common/geometry/S2IncidentEdgeTrackerTest.java?

alan-strohm · 2025-05-28T19:23:52Z

s2/index_cell_data.go

+// cell returns the S2 Cell for the current index cell, loading it if it
+// was not already set.
+func (d *indexCellData) cell() Cell {
+	if !d.s2CellSet.Load() {


Java left it as a TODO to consider making it thread-safe. Do we want to do the same? https://github.com/google/s2-geometry-library-java/blob/4a6abc19e6db01a02ba9781552b08d8a93ec4e5d/library/src/com/google/common/geometry/S2IndexCellData.java#L284

alan-strohm · 2025-05-28T21:18:45Z

s2/incident_edge_tracker.go

+}
+
+// vertexEdge is a tuple of vertex and edgeID for processing incident edges.
+type vertexEdge struct {


Since this is also coming up in index_cell_data.go I think we should consider a 4th option as well:

Move this code to an "internal" package so that it can have a public interface without being a public part of the s2 package.

alan-strohm · 2025-05-28T21:48:54Z

s2/index_cell_data.go

+				edge := shape.ChainEdge(position.ChainID, position.Offset)
+				d.edges = append(d.edges, edgeAndIDChain{
+					Edge:   edge,
+					ID:     int32(edgeID),


why are you using int32 when the shape APIs are returning int?

alan-strohm · 2025-05-28T22:09:36Z

s2/index_cell_data_test.go

+			data.dimWanted[j] = test.dimWanted[j]
+		}


Suggested change

data.dimWanted[j] = test.dimWanted[j]

}

data.setDimWanted(j, test.dimWanted[j])

}

alan-strohm · 2025-05-28T22:10:24Z

s2/index_cell_data_test.go

+		dimEmpty  [3]bool
+	}{
+		{
+			// Check that we get all dimensions by default.


This doesn't actually test the default behavior since it forces the dimWanted to match the test case.

alan-strohm · 2025-05-28T22:12:52Z

s2/index_cell_data.go

+	}
+}
+
+// newindexCellDataFromCell creates a new indexCellData and loads the given


Suggested change

// newindexCellDataFromCell creates a new indexCellData and loads the given

// newIndexCellDataFromCell creates a new indexCellData and loads the given

alan-strohm · 2025-05-28T22:17:18Z

s2/lax_polygon.go

+func LaxPolygonFromLoops(l []Loop) *LaxPolygon {
+	return nil


Was this intentional?

alan-strohm · 2025-05-28T23:06:31Z

s2/lax_polygon_test.go

+
+	for _, edge := range edges {
+		if shape.ChainPosition(edge.e) != (ChainPosition{edge.i, edge.j}) {
+			t.Errorf("addasdaa")


Add a real failure message (also for the next condition in this loop).

rsned added 3 commits May 15, 2025 13:07

Add incidentEdgeTracker and indexCellData types and tests

40abfda

These are ports of the C++ s2/internal/ types that are needed for ValidationQuery for Loops and Polygons. Work for issues golang#72, golang#108.

remove some extraneous bits

db83ebf

Fix LaxPolygon to properly order vertices between shells and holes.

394b634

Clean up some comments and variable names. Update some of the methods to match current C++ logic. Add more unit tests for multiloop polygons to validate that holes are inverted ordering.

rsned force-pushed the internal branch from 172e7fa to 394b634 Compare May 19, 2025 07:27

alan-strohm self-assigned this May 20, 2025

alan-strohm requested changes May 27, 2025

View reviewed changes

alan-strohm requested changes May 28, 2025

View reviewed changes

		@@ -0,0 +1,173 @@
		// Copyright 2025 The S2 Geometry Project Authors. All rights reserved.


		package s2

		// incidentEdgeKey is a tuple of (shape id, vertex) that compares by shape id.


		nursery []vertexEdge

		// We can and do encounter the same edges multiple times, so we need to

	// newindexCellDataFromCell creates a new indexCellData and loads the given
	// newIndexCellDataFromCell creates a new indexCellData and loads the given

Add incidentEdgeTracker and indexCellData types and tests #180

Are you sure you want to change the base?

Add incidentEdgeTracker and indexCellData types and tests #180

Uh oh!

Conversation

rsned commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alan-strohm left a comment

Choose a reason for hiding this comment

Uh oh!

alan-strohm May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

github.com/golang/geo/s2/internal

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!

alan-strohm 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!

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!

Uh oh!

rsned commented May 15, 2025 •

edited

Loading

alan-strohm May 27, 2025 •

edited

Loading