Skip to content

Add incidentEdgeTracker and indexCellData types and tests #180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add incidentEdgeTracker and indexCellData types and tests #180

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
40abfda
Add incidentEdgeTracker and indexCellData types and tests
rsned May 15, 2025
db83ebf
remove some extraneous bits
rsned May 15, 2025
394b634
Fix LaxPolygon to properly order vertices between shells and holes.
rsned May 16, 2025
8aa64ba
Merge branch 'golang:master' into internal
rsned Jul 5, 2025
475ef64
Update s2/index_cell_data.go
rsned Jul 5, 2025
2b1704c
Update s2/index_cell_data_test.go
rsned Jul 5, 2025
9dd01a8
Add missing body for laxPolygonFromLoops and missing test messages.
rsned Jul 5, 2025
d1c19d1
Merge branch 'internal' of ssh://github.com/rsned/geo into internal
rsned Jul 5, 2025
cbe002a
Update index_cell_data with review requests. Updated tests.
rsned Jul 5, 2025
8e8bb75
Updated comments based on Javas clearer comments.
rsned Jul 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 33 additions & 25 deletions s2/incident_edge_tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ func (i incidentEdgeKey) Cmp(o incidentEdgeKey) int {
return i.vertex.Cmp(o.vertex.Vector)
}

// vertexEdge is a tuple of vertex and edgeID for processing incident edges.
type vertexEdge struct {
// incidentVertexEdge is a tuple of vertex and edgeID for processing incident edges.
type incidentVertexEdge struct {
vertex Point
edgeID int32
}
Expand All @@ -46,15 +46,15 @@ type vertexEdge struct {
// but lookup is by shape id and vertex: there is no facility to get all
// edges of all shapes at a vertex. Edge vertices must compare exactly equal
// to be considered the same vertex, no tolerance is applied as this isn't
// intended for e.g.: snapping shapes together, which Builder does better
// intended for actions like snapping shapes together, which Builder does better
// and more robustly.
//
// 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
// the resulting map from incidentEdgeKeys (which are shapeId, vertex pairs) to
// a set of edgeIds of the shape that are incident to that vertex..
// To use, instantiate and then add edges with one or more sequences of calls;
// each sequence begins with startShape(), followed by repeated 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, read the edgeMap 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.
//
// This works on a block of edges at a time, meaning that to detect incident
// edges on a particular vertex, we must have at least three edges incident
Expand All @@ -63,62 +63,66 @@ type vertexEdge struct {
// 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
// The reason for this is simple: most vertices don't have more than two incident
// edges (the incoming and outgoing edge). If we had to maintain information
// between calls, we'd end up with a map that contains every vertex, to no
// benefit. Instead, when finishShape() is called, we discard vertices that
// contain two or fewer incident edges.
//
// In principle this isn't a real limitation because generally we process a
// ShapeIndex cell at a time, and, if a vertex has multiple edges, we'll see
// ShapeIndex one cell at a time, and, if a vertex has multiple edges, we'll see
// 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
// The tracker maintains incident edges until it's reset. If you call it with
// each cell from an ShapeIndex, then at the end you will have all the
// incident edge information for the whole index. If only a subset is needed,
// call reset() when you're done.
// call reset() when you're done edding edges.
type incidentEdgeTracker struct {
currentShapeID int32

nursery []vertexEdge
// nursery is used to store points being processed for the map.
nursery []incidentVertexEdge

// We can and do encounter the same edges multiple times, so we need to
// deduplicate edges as they're inserted.
// edgeMap tracks edgeIDs for each incidentEdgeKey since we can, and do,
// encounter the same edges multiple times. This map gives us easy
// deduplication of edges as they're inserted.
edgeMap map[incidentEdgeKey]map[int32]bool
}

// newIncidentEdgeTracker returns a new tracker.
// newIncidentEdgeTracker returns a new incidentEdgeTracker.
func newIncidentEdgeTracker() *incidentEdgeTracker {
return &incidentEdgeTracker{
currentShapeID: -1,
nursery: []vertexEdge{},
nursery: []incidentVertexEdge{},
edgeMap: make(map[incidentEdgeKey]map[int32]bool),
}
}

// startShape is used to start adding edges to the edge tracker. After calling,
// any vertices with multiple (> 2) incident edges will appear in the
// incident edge map.
// startShape is used to start a new shape.
func (t *incidentEdgeTracker) startShape(id int32) {
t.currentShapeID = id
t.nursery = t.nursery[:0]
}

// addEdge adds the given edges start to the nursery, and if not degenerate,
// adds it second endpoint as well.
// addEdge adds the given edge for the current shape to the nursery. If the
// edge is not degenerate, add its second endpoint as well.
func (t *incidentEdgeTracker) addEdge(edgeID int32, e Edge) {
if t.currentShapeID < 0 {
return
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Add non-degenerate edges to the nursery.
t.nursery = append(t.nursery, vertexEdge{vertex: e.V0, edgeID: edgeID})
t.nursery = append(t.nursery, incidentVertexEdge{vertex: e.V0, edgeID: edgeID})
if !e.IsDegenerate() {
t.nursery = append(t.nursery, vertexEdge{vertex: e.V1, edgeID: edgeID})
t.nursery = append(t.nursery, incidentVertexEdge{vertex: e.V1, edgeID: edgeID})
}
}

// finishShape is called once finished adding edges so they can be processed.
//
// After calling, only vertices with 2 or more incident edges will appear in
// the edge map.
func (t *incidentEdgeTracker) finishShape() {
// We want to keep any vertices with more than two incident edges. We could
// sort the array by vertex and remove any with fewer, but that would require
Expand Down Expand Up @@ -165,9 +169,13 @@ func (t *incidentEdgeTracker) finishShape() {
t.edgeMap[key][t.nursery[start].edgeID] = true
}
}

// We are finished with this shape, clear the nursery.
t.nursery = t.nursery[:0]
}

// reset removes all incident edges from the tracker.
func (t *incidentEdgeTracker) reset() {
t.nursery = t.nursery[:0]
t.edgeMap = make(map[incidentEdgeKey]map[int32]bool)
}
103 changes: 46 additions & 57 deletions s2/index_cell_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,17 @@

package s2

import (
"sync"
"sync/atomic"
)

// indexCellRegion represents a simple pair for defining an integer valued region.
type indexCellRegion struct {
// indexCellRange represents a simple pair for defining an integer valued range.
type indexCellRange struct {
start int
size int
}

// shapeRegion is a mapping from shape id to the region of the edges array
// shapeRange is a mapping from shapeID to the range of the edges array
// it's stored in.
type shapeRegion struct {
id int32
region indexCellRegion
type shapeRange struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this one too generic? Would indexShapeRange be better?

id int32
cellRange indexCellRange
}

// edgeAndIDChain is an extension of Edge with fields for the edge id,
Expand Down Expand Up @@ -82,16 +77,16 @@ type edgeAndIDChain struct {
// shapes _within a dimension_ are in the same order they are in the index
// itself, and the edges _within a shape_ are similarly in the same order.
type indexCellData struct {
// index is the ShapeIndex the currently loaded Cell belongs to.
index *ShapeIndex
indexCell *ShapeIndexCell
cellID CellID

// Computing the cell center and Cell can cost as much as looking up the
// edges themselves, so defer doing it until needed.
mu sync.Mutex
s2CellSet atomic.Bool
s2CellSet bool
s2Cell Cell
centerSet atomic.Bool
centerSet bool
cellCenter Point

// Dimensions that we wish to decode, the default is all of them.
Expand All @@ -100,11 +95,11 @@ type indexCellData struct {
// Storage space for edges of the current cell.
edges []edgeAndIDChain

// Mapping from shape id to the region of the edges array it's stored in.
shapeRegions []shapeRegion
// Mapping from shape id to the ranges of the edges array it's stored in.
shapeRanges []shapeRange

// Region for each dimension we might encounter.
dimRegions [3]indexCellRegion
// Range for each dimension we might encounter.
dimRanges [3]indexCellRange
}

// newIndexCellData creates a new indexCellData with the expected defaults.
Expand All @@ -114,7 +109,7 @@ func newIndexCellData() *indexCellData {
}
}

// newindexCellDataFromCell creates a new indexCellData and loads the given
// newIndexCellDataFromCell creates a new indexCellData and loads the given
// cell data.
func newIndexCellDataFromCell(index *ShapeIndex, id CellID, cell *ShapeIndexCell) *indexCellData {
d := newIndexCellData()
Expand All @@ -129,7 +124,7 @@ func (d *indexCellData) reset() {
d.index = nil
d.indexCell = nil
d.edges = d.edges[:0]
d.shapeRegions = d.shapeRegions[:0]
d.shapeRanges = d.shapeRanges[:0]
d.dimWanted = [3]bool{true, true, true}
}

Expand All @@ -144,27 +139,21 @@ func (d *indexCellData) setDimWanted(dim int, wanted bool) {
// 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() {
d.mu.Lock()
if !d.s2CellSet.Load() {
d.s2Cell = CellFromCellID(d.cellID)
d.s2CellSet.Store(true)
}
d.mu.Unlock()
// TODO(rsned): Consider if we need to add mutex here for thread safety.
if !d.s2CellSet {
d.s2Cell = CellFromCellID(d.cellID)
d.s2CellSet = true
}
return d.s2Cell
}

// center returns the center point of the current index cell, loading it
// if it was not already set.
func (d *indexCellData) center() Point {
if !d.centerSet.Load() {
d.mu.Lock()
if !d.centerSet.Load() {
d.cellCenter = d.cellID.Point()
d.centerSet.Store(true)
}
d.mu.Unlock()
// TODO(rsned): Consider if we need to add mutex here for thread safety.
if !d.centerSet {
d.cellCenter = d.cellID.Point()
d.centerSet = true
}
return d.cellCenter
}
Expand All @@ -177,7 +166,7 @@ func (d *indexCellData) center() Point {
// loadCell, loading is not performed since we already have the data decoded.
func (d *indexCellData) loadCell(index *ShapeIndex, id CellID, cell *ShapeIndexCell) {
// If this is still the same ShapeIndexCell as last time, then we are good.
if d.index == index && d.cellID == id {
if d.index == index && d.cellID == id && cell == d.indexCell {
return
}

Expand All @@ -187,17 +176,17 @@ func (d *indexCellData) loadCell(index *ShapeIndex, id CellID, cell *ShapeIndexC
d.indexCell = cell
d.cellID = id

// Reset atomic flags so we'll recompute cached values.
d.s2CellSet.Store(false)
d.centerSet.Store(false)
// Reset flags so we'll recompute cached values.
d.s2CellSet = false
d.centerSet = false

// Clear previous edges
d.edges = d.edges[:0]
d.shapeRegions = d.shapeRegions[:0]
d.shapeRanges = d.shapeRanges[:0]

// Reset per-dimension region information.
for i := range d.dimRegions {
d.dimRegions[i] = indexCellRegion{}
// Reset per-dimension range information.
for i := range d.dimRanges {
d.dimRanges[i] = indexCellRange{}
}

minDim := 0
Expand Down Expand Up @@ -239,7 +228,7 @@ func (d *indexCellData) loadCell(index *ShapeIndex, id CellID, cell *ShapeIndexC

// Materialize clipped shape edges into the edges
// slice. Track where we start so we can add
// information about the region for this shape.
// information about the range for this shape.
shapeStart := len(d.edges)
for _, edgeID := range clipped.edges {
// Looking up an edge requires looking up
Expand All @@ -258,17 +247,17 @@ func (d *indexCellData) loadCell(index *ShapeIndex, id CellID, cell *ShapeIndexC
}

// Note which block of edges belongs to the shape.
d.shapeRegions = append(d.shapeRegions, shapeRegion{
d.shapeRanges = append(d.shapeRanges, shapeRange{
id: shapeID,
region: indexCellRegion{
cellRange: indexCellRange{
start: shapeStart,
size: len(d.edges) - shapeStart,
},
})
}

// Save region information for the current dimension.
d.dimRegions[dim] = indexCellRegion{
// Save range information for the current dimension.
d.dimRanges[dim] = indexCellRange{
start: dimStart,
size: len(d.edges) - dimStart,
}
Expand All @@ -277,11 +266,11 @@ func (d *indexCellData) loadCell(index *ShapeIndex, id CellID, cell *ShapeIndexC

// shapeEdges returns a slice of the edges in the current cell for a given shape.
func (d *indexCellData) shapeEdges(shapeID int32) []edgeAndIDChain {
for _, sr := range d.shapeRegions {
for _, sr := range d.shapeRanges {
if sr.id == shapeID {
region := sr.region
if region.start < len(d.edges) {
return d.edges[region.start : region.start+region.size]
cellRange := sr.cellRange
if cellRange.start < len(d.edges) {
return d.edges[cellRange.start : cellRange.start+cellRange.size]
}
return nil
}
Expand All @@ -296,9 +285,9 @@ func (d *indexCellData) dimEdges(dim int) []edgeAndIDChain {
return nil
}

region := d.dimRegions[dim]
if region.start < len(d.edges) {
return d.edges[region.start : region.start+region.size]
dimRange := d.dimRanges[dim]
if dimRange.start < len(d.edges) {
return d.edges[dimRange.start : dimRange.start+dimRange.size]
}
return nil
}
Expand All @@ -310,12 +299,12 @@ func (d *indexCellData) dimRangeEdges(dim0, dim1 int) []edgeAndIDChain {
return nil
}

start := d.dimRegions[dim0].start
start := d.dimRanges[dim0].start
size := 0

for dim := dim0; dim <= dim1; dim++ {
start = minInt(start, d.dimRegions[dim].start)
size += d.dimRegions[dim].size
start = minInt(start, d.dimRanges[dim].start)
size += d.dimRanges[dim].size
}

if start < len(d.edges) {
Expand Down
Loading