Builder foundation work by rsned · Pull Request #182 · golang/geo

rsned · 2025-05-19T07:08:25Z

Add the types in the builder system (graph, layer, various options and enums). The types are all set as package private for now so no one can use them until they are ready.
Add all the comments and documentation for the above.
Add Builder::Graph's EdgeProcessor embedded type and tests.

Next up will be working through Graphs other types like PolylineBuilder and its various methods.

Fix comments. Remove outdate maxEdgeDeviation methods. Add TODOs for a few more things to be done. Update some of the inline constant values to match the C++ values. (sqrt(2)/13 --> 0.548, etc. Remove removed methods from test.

… enums.

Convert the first of the nested types from S2Builer::Graph to Go adding some unit tests.

…ches.

flwyd

Thanks for tackling this!

flwyd · 2025-05-21T20:31:16Z

s2/builder.go

+// (including the case where there are no edges at all), then the layer
+// implementation calls the given predicate to determine whether the polygon
+// is empty or full except for those degeneracies. The predicate is given
+// an S2Builder::Graph containing the output edges, but note that in general


Change :: references to Go style.

s2/builder.go

flwyd · 2025-05-21T20:34:04Z

s2/builder.go

+//     or E7 lat/lng coordinates) while preserving the input topology and with
+//     guaranteed error bounds.
+//
+// 3. Simplifying geometry (e.g. for indexing, display, or storage).


Go doc lists need indentation before the number (this one's missing a space).

flwyd · 2025-05-21T20:42:10Z

s2/builder.go

+//	builder.StartLayer(NewPolygonLayer(output))
+//	builder.AddPolygon(input);
+//	if err := builder.Build(); err != nil {
+//	  fmt.Printf("error building: %v\n"), err


Nit: misplaced closing paren

flwyd · 2025-05-21T20:48:02Z

s2/builder.go

+//
+// TODO(rsned): Make the type public when Builder is ready.
+type builder struct {
+	opts *builderOptions


I recognize that the C++ and Java implementations also have a mutable Options member, but this seems like a weird use of the builder pattern. Someone could change state of the options part way through using the Builder, which seems like it would make reasoning about the semantic guarantees more difficult.

It also seems kind of odd that there's a builderOptions type, but also a lot of option-like fields in the builder itself. Does the builder need to hang on to the builderOptions that was used in init, or are the derived fields sufficient?

There seems to be only a couple places that read from it within Builder so making it non-mutable seems reasonable.

The primary use case from C++ is passing in the default options, or creating an options with an explicit snap function a la "S2Builder builder{S2Builder::Options(S2CellIdSnapFunction(snap_level))};"

flwyd · 2025-05-22T01:28:51Z

s2/builder_graph_options.go

+// to consist of four edges (two duplicate edges and their siblings), since
+// only two of these four edges will be used in the final output.
+//
+// Furthermore, since the options REQUIRE and CREATE guarantee that all


Change all-caps to Go identifiers here and below.

flwyd · 2025-05-22T01:31:11Z

s2/builder_graph_options.go

+)
+
+// graphOptions is only needed by Layer implementations. A layer is
+// responsible for assembling an Graph of snapped edges into the


s/an (Graph)/a \1/

flwyd · 2025-05-22T01:33:05Z

s2/builder_graph_options.go

+	// DEFAULT: edgeTypeDirected
+	edgeType edgeType
+
+	// DEFAULT: degenerateEdgesKeep


Could we make the default values the first iota value when declaring the constants?

I've reordered all the builder enums to have the first iota value be the expected default option.

flwyd · 2025-05-22T01:35:13Z

s2/builder_graph_options.go

+	// the graph passed to this layer will contain the full set of vertices.
+	// (This is not recommended when the number of layers could be large.)
+	//
+	// DEFAULT: true


Could we change the name so the default can be false, maybe disableVertexFiltering?

flwyd · 2025-05-22T01:36:22Z

s2/builder_options.go

+
+import "github.com/golang/geo/s1"
+
+// polylineType Indicates whether polylines should be "paths" (which don't


Lower-case indicates

Rename edge {int, int} to graphEdge to reduce confusablilty with Shape's Edge{point, point} In Run, fix a couple of logic errors with (x&1)+1. Don't fail on the first error encountered in run, keep processing edges and return any/the last error encountered to follow C++'s Run().

… builder-foundations

Move the enums and option types back into their main file. (graph_options-> graph) Re-order enums to ensure the default value is first in the iota list. Rename builder variables to get rid of the CA suffix. Clean up some comments

Incorporate the comments missing on builder and graph from the Java classes. Spelling fixes.

flwyd

Sorry for the delayed review. FYI, I'll be OOO next week.

Since the types are package-private, let me know if there are comments you'd prefer to address after submitting this first round.

flwyd · 2025-06-06T20:40:36Z

s2/builder_graph.go

+//
+// TODO(rsned): Consider pulling out the methods that are helper functions for
+// Layer implementations (such as getDirectedLoops) into a builder_graph_util.go.
+type graph struct {


In C++ this is S2Builder::Graph and in Java it's S2BuilderGraph. Does this function as a general-purpose graph, or should this be s2.BuilderGraph?

flwyd · 2025-06-06T20:43:47Z

s2/builder_graph.go

+	numVertices int32
+	// The vertices in this Graph. The index of a Vertex in this list is its VertexID.
+	vertices []Point
+	// The graphEdges in this Graph. The index of a graphEdge in this list is the EdgeID.


The constructor documentation in C++/Java wants this to be lexicographically sorted.

flwyd · 2025-06-06T20:49:04Z

s2/builder_graph.go

+}
+
+// newGraph returns a new graph instance initialized with the given data.
+func newGraph(opts *graphOptions,


Is this going to get additional logic, or would it be sufficient for callers to just initialize the struct?

flwyd · 2025-06-06T20:56:40Z

s2/builder_graph.go

+// edge pairs in several possible ways (e.g. discarding or creating them).
+// The output is suitable for passing to the newGraph method.
+//
+// If options.edgeType == EdgeTypeUndirected, then all input edges


s/options/opts/ here and below.

flwyd · 2025-06-06T21:22:14Z

s2/builder.go

+//     centers, E7 lat/lng points on the sphere, or simply a subset of the
+//     input vertices), rather than being limited to an integer grid.
+//
+// Here are some of its other features:


The Java version also has some documentation about edge labels that might be worth including.

flwyd · 2025-06-12T00:45:09Z

s2/builder_graph.go

+}
+
+// maxVertexID is the maximum possible vertex ID, used as a sentinel value.
+const maxVertexID = int32(^uint32(0) >> 1)


I'm not great with bit fiddling; is this the same as math.MaxInt32?

flwyd · 2025-06-12T00:46:12Z

s2/builder_graph.go

+
+// stableLessThan compares two graphEdges for stable sorting.
+// It uses the graphEdge IDs as a tiebreaker to ensure a stable sort.
+func stableLessThan(a, b graphEdge, aID, bID int32) bool {


Might want a type-specific name to avoid package-level collisions.

flwyd · 2025-06-12T00:53:08Z

s2/builder_graph.go

+
+// graphEdge is a tuple of edge IDs.
+type graphEdge struct {
+	first, second int32


Optional: I wonder if type edgeID int32 would make things more readable, or if it would just make it harder to match the other implementations.

flwyd · 2025-06-12T00:56:52Z

s2/builder_graph.go

+func (ep *graphEdgeProcessor) handleDegenerateEdge(edge graphEdge, outBegin, outEnd int, nOut, nIn, inBegin, in int) error {
+	// This is a degenerate edge.
+	if nOut != nIn {
+		return errors.New("inconsistent number of degenerate edges")


Is it worth including the counts?

flwyd · 2025-06-12T01:14:45Z

s2/builder_layer.go

+	// This makes it easier to write algorithms that gather the output graphs
+	// from several layers and process them all at once (such as
+	// closedSetNormalizer).
+	Build(g *graph) (bool, error)


Document the meaning of the returned boolean.

rsned and others added 18 commits May 4, 2025 11:20

Update builder_snapper files.

bcd6c35

Fix comments. Remove outdate maxEdgeDeviation methods. Add TODOs for a few more things to be done. Update some of the inline constant values to match the C++ values. (sqrt(2)/13 --> 0.548, etc. Remove removed methods from test.

Define the builder options type and its two helper methods.

3ab23c5

add header text

1d63448

Add builders layer interface.

09f7532

Add the base builder type with its extensive comment block.

51ac9e3

Add builder graph options and related enums.

a95103b

Add initial builder graph type and related enums.

0b2af37

Add more types to graph options.

af0ce50

Add another batch of builder internal variables and update init.

6084658

change polylinetype to package private like the other builder enums.

0b5e252

Add remaining builder datastructure elements and finish init().

6435633

update comments and move edge_type to graph_options next to the other…

45220f6

… enums.

minor enum shuffling

38bd88b

Add S2Builder Graph's EdgeProcessor class to Go with unit tests.

03f216d

Convert the first of the nested types from S2Builer::Graph to Go adding some unit tests.

fix some staticcheck lint errors.

4269fec

Update edgeProcessor and add a few more test cases to cover more bran…

d5f81d3

…ches.

simplify test merge input ids setup.

2ce78ad

Merge branch 'master' into builder-foundations

e2d1225

flwyd reviewed May 22, 2025

View reviewed changes

rsned added 6 commits May 28, 2025 16:57

Merge branch 'builder-foundations' of ssh://github.com/rsned/geo into…

463a9e0

… builder-foundations

Add all of the test cases for graphEdgeProcessor's Run from C++

7b462a4

Roll options back into the file of the same name.

a2bb0fe

Move the enums and option types back into their main file. (graph_options-> graph) Re-order enums to ensure the default value is first in the iota list. Rename builder variables to get rid of the CA suffix. Clean up some comments

Move graphEdgeProcessor and tests into builder_graph.go and tests.

d817c66

Add comments to remaining member variables in Builder and Graph.

b68663f

Incorporate the comments missing on builder and graph from the Java classes. Spelling fixes.

flwyd reviewed Jun 12, 2025

View reviewed changes


		import "github.com/golang/geo/s1"

		// polylineType Indicates whether polylines should be "paths" (which don't

Conversation

rsned commented May 19, 2025

Uh oh!

flwyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

flwyd 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants