Builder foundation work #182
Conversation
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.
Convert the first of the nested types from S2Builer::Graph to Go adding some unit tests.
s2/builder.go
Outdated
| // (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 |
There was a problem hiding this comment.
Change :: references to Go style.
s2/builder.go
Outdated
| @@ -33,3 +39,345 @@ const ( | |||
| // when MaxEdgeDeviation is exceeded. | |||
| maxEdgeDeviationRatio = 1.1 | |||
| ) | |||
|
|
|||
| // isFullPolygonPredicate is an interface for determining if Polygons are | |||
There was a problem hiding this comment.
Any reason this is the first thing in the file? The java version has it at the end, and it seems like a pretty minor part of the builder API.
There was a problem hiding this comment.
A lot of it is just the order I bumped into it in the C++. Will massage the orderings.
s2/builder.go
Outdated
| // 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). |
There was a problem hiding this comment.
Go doc lists need indentation before the number (this one's missing a space).
s2/builder.go
Outdated
| // builder.StartLayer(NewPolygonLayer(output)) | ||
| // builder.AddPolygon(input); | ||
| // if err := builder.Build(); err != nil { | ||
| // fmt.Printf("error building: %v\n"), err |
There was a problem hiding this comment.
Nit: misplaced closing paren
s2/builder.go
Outdated
| // | ||
| // TODO(rsned): Make the type public when Builder is ready. | ||
| type builder struct { | ||
| opts *builderOptions |
There was a problem hiding this comment.
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 was a problem hiding this comment.
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))};"
s2/builder_graph_options.go
Outdated
| // 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 |
There was a problem hiding this comment.
Change all-caps to Go identifiers here and below.
s2/builder_graph_options.go
Outdated
| ) | ||
|
|
||
| // graphOptions is only needed by Layer implementations. A layer is | ||
| // responsible for assembling an Graph of snapped edges into the |
s2/builder_graph_options.go
Outdated
| // DEFAULT: edgeTypeDirected | ||
| edgeType edgeType | ||
|
|
||
| // DEFAULT: degenerateEdgesKeep |
There was a problem hiding this comment.
Could we make the default values the first iota value when declaring the constants?
s2/builder_graph_options.go
Outdated
| // 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 |
There was a problem hiding this comment.
Could we change the name so the default can be false, maybe disableVertexFiltering?
s2/builder_options.go
Outdated
|
|
||
| import "github.com/golang/geo/s1" | ||
|
|
||
| // polylineType Indicates whether polylines should be "paths" (which don't |
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
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.