Fix the bounding box of a GeometryCollection in the TWKB output by shounakjoshi88-a11y · Pull Request #720 · peterstace/simplefeatures

shounakjoshi88-a11y · 2026-11-12T06:40:53Z

The bounding box of a GeometryCollection in the TWKB output was all zeros.

The sub-geometries of a collection are written by their own writers (copytwkbWriter), which do not write a bounding box of their own, so their coordinates never reached the bounding box of the collection. Every other type has the right box.

MarshalTWKB(GEOMETRYCOLLECTION(POINT(-1 -2)), 0, TWKBBoundingBoxHeader())

wrote the bbox bit with four zero varints, so UnmarshalTWKBEnvelope gave ENVELOPE(0 0, 0 0) for a geometry at (-1 -2). A consumer that filters a TWKB stream by its bounding boxes never found a collection.

The collection now widens its bounding box with the box of each sub-geometry.

The new test in geom/twkb_test.go marshals a collection with a bounding box header and checks the envelope. It fails without the change:

--- FAIL: TestTWKBBBoxOfGeometryCollection
    twkb_test.go:485: got:  {0 0}

and passes with it.

$ go test ./geom/
ok  	github.com/peterstace/simplefeatures/geom

(TestCatch and TestGeoJSONSyntaxError fail on this machine both with and without the change - they compare an error message that differs on Windows.)

The sub-geometries of a collection are written by their own writers, so their coordinates never reached the bounding box of the collection, and a collection was written with an all-zero box.

shounakjoshi88-a11y · 2026-11-12T06:41:57Z

The build workflow on this pull request is in action_required, which needs a maintainer to approve it before it can run on a fork pull request. If you approve it, it should pass - the same commands run green locally:

$ go test ./geom/
ok  	github.com/peterstace/simplefeatures/geom

TestCatch and TestGeoJSONSyntaxError fail on this machine both with and without the change - they compare an error message that differs on Windows.

gofmt reports nothing on the two changed files, and the change follows the CLAUDE.md instruction to add a CHANGELOG.md entry.

peterstace

Thanks for the PR! I've got some requests for change.

peterstace · 2026-11-13T05:20:12Z

+	for d := 0; d < w.dimensions; d++ {
+		switch {
+		case !w.bboxValid:
+			w.bboxMin[d] = other.bboxMin[d]
+			w.bboxMax[d] = other.bboxMax[d]
+		case other.bboxMin[d] < w.bboxMin[d]:
+			w.bboxMin[d] = other.bboxMin[d]
+		case other.bboxMax[d] > w.bboxMax[d]:
+			w.bboxMax[d] = other.bboxMax[d]
+		}
+	}


The switch runs at most one case per dimension, so when other extends past w on both sides only the min is updated. That works in writePointArray because a single value can't be below the min and above the max at once, but a child's box can.

GEOMETRYCOLLECTION(POINT(5 5),LINESTRING(0 0,10 10))

is written with a box of (0 0)-(5 5) instead of (0 0)-(10 10). Nested collections have the same problem: GEOMETRYCOLLECTION(POINT(1 1),GEOMETRYCOLLECTION(POINT(0 0),POINT(2 2))) gets (0 0)-(1 1).

Checking the min and the max independently fixes it:

Suggested change

for d := 0; d < w.dimensions; d++ {

switch {

case !w.bboxValid:

w.bboxMin[d] = other.bboxMin[d]

w.bboxMax[d] = other.bboxMax[d]

case other.bboxMin[d] < w.bboxMin[d]:

w.bboxMin[d] = other.bboxMin[d]

case other.bboxMax[d] > w.bboxMax[d]:

w.bboxMax[d] = other.bboxMax[d]

}

}

for d := 0; d < w.dimensions; d++ {

if !w.bboxValid || other.bboxMin[d] < w.bboxMin[d] {

w.bboxMin[d] = other.bboxMin[d]

}

if !w.bboxValid || other.bboxMax[d] > w.bboxMax[d] {

w.bboxMax[d] = other.bboxMax[d]

}

}

Both bounds are now checked independently, with your suggestion. The test is now three entries at the end of TWKBTestCases with your values, and the standalone test function that caused the lint failure and the misplaced comment is removed. The two new cases fail with the old widenBBox and pass with the new one.

peterstace · 2026-11-13T05:20:12Z

+
+func TestTWKBBBoxOfGeometryCollection(t *testing.T) {
+	// The bounding box of a collection has to cover its sub-geometries.
+	g := test.FromWKT(t, "GEOMETRYCOLLECTION(POINT(-1 -2))")
+	twkb, err := geom.MarshalTWKB(g, 0, geom.TWKBBoundingBoxHeader())
+	test.NoErr(t, err)
+
+	env, ok, err := geom.UnmarshalTWKBEnvelope(twkb)
+	test.NoErr(t, err)
+	test.True(t, ok)
+
+	min, max, ok := env.XYEnvelope.MinMaxXYs()
+	test.True(t, ok)
+	test.Eq(t, min, geom.XY{X: -1, Y: -2})
+	test.Eq(t, max, geom.XY{X: -1, Y: -2})
+}
+


This only covers a collection with one point, so it never merges boxes across children, and it passes with the bug in widenBBox.

Please replace it with entries at the end of TWKBTestCases. The table already has hasBBox and listedBBox, checks the exact bytes on encode, and checks the envelope on decode. The three entries below pass with the suggested widenBBox change, and the second and third fail without it:

{ description: "geometry collection with bbox", twkbHex: "0701010003000101000103", wkt: "GEOMETRYCOLLECTION(POINT(-1 -2))", hasBBox: true, listedBBox: [2]string{"POINT(-1 -2)", "POINT(-1 -2)"}, }, { description: "geometry collection with bbox widened on both sides by a later child", twkbHex: "0701001400140201000a0a02000200001414", wkt: "GEOMETRYCOLLECTION(POINT(5 5),LINESTRING(0 0,10 10))", hasBBox: true, listedBBox: [2]string{"POINT(0 0)", "POINT(10 10)"}, }, { description: "nested geometry collection with bbox", twkbHex: "07010004000402010002020700020100000001000404", wkt: "GEOMETRYCOLLECTION(POINT(1 1),GEOMETRYCOLLECTION(POINT(0 0),POINT(2 2)))", hasBBox: true, listedBBox: [2]string{"POINT(0 0)", "POINT(2 2)"}, },

The suggestion below deletes the function and the blank lines added around it. That also fixes two smaller problems:

make lint fails on line 483 with variable min has same name as predeclared identifier (predeclared).

The function sits between the doc comment for TestWriteTWKBSQLFile and the function it documents, so the comment is no longer attached to it.

Suggested change

func TestTWKBBBoxOfGeometryCollection(t *testing.T) {

// The bounding box of a collection has to cover its sub-geometries.

g := test.FromWKT(t, "GEOMETRYCOLLECTION(POINT(-1 -2))")

twkb, err := geom.MarshalTWKB(g, 0, geom.TWKBBoundingBoxHeader())

test.NoErr(t, err)

env, ok, err := geom.UnmarshalTWKBEnvelope(twkb)

test.NoErr(t, err)

test.True(t, ok)

min, max, ok := env.XYEnvelope.MinMaxXYs()

test.True(t, ok)

test.Eq(t, min, geom.XY{X: -1, Y: -2})

test.Eq(t, max, geom.XY{X: -1, Y: -2})

}

The predeclared linter rejects the min and max variables of the TWKB bounding box test, as they shadow the builtins of the same name. They are renamed to gotMin and gotMax, which is what the other tests of the package use.

The switch in widenBBox ran at most one case per dimension, so a child that extended past the collection on both sides only updated the min. The min and the max are now checked independently. The test moves into the TWKBTestCases table, which checks the exact bytes on encode and the envelope on decode. It covers a collection that is widened on both sides by a later child and a nested collection. The two of them fail without the change.

shounakjoshi88-a11y · 2026-11-13T05:38:51Z

Thanks for the review - all three points are addressed, and you were right about the bug in widenBBox.

The switch bug. The min and the max are now checked independently, with your suggestion:

	for d := 0; d < w.dimensions; d++ {
		if !w.bboxValid || other.bboxMin[d] < w.bboxMin[d] {
			w.bboxMin[d] = other.bboxMin[d]
		}
		if !w.bboxValid || other.bboxMax[d] > w.bboxMax[d] {
			w.bboxMax[d] = other.bboxMax[d]
		}
	}

GEOMETRYCOLLECTION(POINT(5 5),LINESTRING(0 0,10 10)) now gets the box (0 0)-(10 10), and the nested collection gets (0 0)-(2 2).

The test. My standalone test is gone, and the three entries are at the end of TWKBTestCases with your values. The second and third fail with the old widenBBox and pass with the new one:

--- FAIL: TestTWKBUnmarshalMarshalValid/geometry_collection_with_bbox_widened_on_both_sides_by_a_later_child
--- FAIL: TestTWKBUnmarshalMarshalValid/nested_geometry_collection_with_bbox

The lint failure and the misplaced comment. Both were caused by the test function that I removed, so they are gone with it - make lint no longer complains about the predeclared min and max, and the doc comment of TestWriteTWKBSQLFile is attached to it again.

I ran the linter of the CI image locally to be sure:

$ golangci-lint run --max-same-issues 100 ./geom/...

golangci-lint v1.61.0 with the repository's own .golangci.yaml reports nothing for the geom package. The unit tests are green as well, apart from the failures that are on the base commit here as well (TestGeoJSONSyntaxError with the newer Go version, and the packages that need cgo, PostGIS or PostgreSQL).

One note: the build workflow on the new head (c133be5) is in action_required again, so it needs approving before it can run.

…geometry-collection

shounakjoshi88-a11y · 2026-11-13T05:43:20Z

One more thing: the branch was behind master, so I merged it in - the head is now 829c30e. The build workflow needs approving for that commit (the earlier approval was for the previous head).

On that head the geom package is green and golangci-lint v1.61.0 with the repository's own configuration reports nothing for the package.

Fix the bounding box of a GeometryCollection in the TWKB output

cbd3f38

The sub-geometries of a collection are written by their own writers, so their coordinates never reached the bounding box of the collection, and a collection was written with an all-zero box.

peterstace requested changes Nov 13, 2026

View reviewed changes

This was referenced Nov 13, 2026

MarshalTWKB writes incorrect output for empty geometries #721

Open

Share one bounding box accumulator across TWKB sub-writers #722

Open

shounakjoshi88-a11y added 2 commits November 13, 2026 10:57

Merge remote-tracking branch 'upstream/master' into fix/twkb-bbox-of-…

829c30e

…geometry-collection

Navigation Menu

Repository navigation

Fix the bounding box of a GeometryCollection in the TWKB output - #720

Fix the bounding box of a GeometryCollection in the TWKB output#720
shounakjoshi88-a11y wants to merge 4 commits into
peterstace:masterfrom
shounakjoshi88-a11y:fix/twkb-bbox-of-geometry-collection

shounakjoshi88-a11y commented Nov 12, 2026

Uh oh!

shounakjoshi88-a11y commented Nov 12, 2026

Uh oh!

peterstace left a comment

Uh oh!

peterstace Nov 13, 2026

Uh oh!

shounakjoshi88-a11y Nov 13, 2026

Uh oh!

peterstace Nov 13, 2026

Uh oh!

shounakjoshi88-a11y commented Nov 13, 2026

Uh oh!

shounakjoshi88-a11y commented Nov 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

shounakjoshi88-a11y commented Nov 12, 2026

Uh oh!

shounakjoshi88-a11y commented Nov 12, 2026

Uh oh!

peterstace left a comment

Choose a reason for hiding this comment

Uh oh!

peterstace Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

shounakjoshi88-a11y Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

peterstace Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

shounakjoshi88-a11y commented Nov 13, 2026

Uh oh!

shounakjoshi88-a11y commented Nov 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants