Add OSM PBF encoder#52
Conversation
Add osm pbf encoder
Farm-Art
left a comment
There was a problem hiding this comment.
Hello, thank you for providing this fork. I've used it successfully in one of my internal projects, though not without some (easily fixed) issues. I've pointed them out in this review, didn't have time to submit a proper pull request, though I'm considering making my own fork eventually to alleviate a few other pet peeves I have with this library in general.
|
|
||
| blockData, err := proto.Marshal(block) | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
Looks like a typo, should return err instead of nil. This came up in my attempt to use this fork, concealing the issue above that quietly omitted all Nodes from the output file.
| } | ||
| } | ||
| groupDense.Denseinfo.Uid = append(groupDense.Denseinfo.Uid, int32(current.UserID-previous.UserID)) | ||
| groupDense.Denseinfo.Version = append(groupDense.Denseinfo.Version, int32(current.Version-previous.Version)) |
There was a problem hiding this comment.
Versions aren't delta-encoded in the spec, this causes issues with the current decoder. Should just append current.Version.
| for k := range e.reverseStringTable { | ||
| delete(e.reverseStringTable, k) | ||
| } | ||
|
|
There was a problem hiding this comment.
I ran into a few issues with Nodes that had empty stringtables, adding this snippet right here before proto.Marshal seemingly solved it for me. I'm assuming it has something to do with missing idx-0 delimiters, but I didn't have time to properly study the spec and figure out if that's the case.
if block.Stringtable == nil {
block.Stringtable = &osmpbf.ForkStringTable{}
}
if block.Stringtable.S == nil {
block.Stringtable.S = []string{""}
}| func EncodeLatLon(value float64, offset int64, granularity int32) int64 { | ||
| return (int64(value/.000000001) - offset) / int64(granularity) | ||
| } |
There was a problem hiding this comment.
| func EncodeLatLon(value float64, offset int64, granularity int32) int64 { | |
| return (int64(value/.000000001) - offset) / int64(granularity) | |
| } | |
| func EncodeLatLon(value float64, offset int64, granularity int32) int64 { | |
| // Multiply by 1e9 (exactly representable as int64) rather than dividing by | |
| // the float64 literal 0.000000001, which is not exactly 1e-9 in binary and | |
| // causes systematic off-by-one truncation for many coordinate values. | |
| return (int64(math.Round(value*1e9)) - offset) / int64(granularity) | |
| } |
EncodeLatLon as implemented converts a degree value to an OSM PBF integer using division by the float64 literal 0.000000001, followed by a truncating int64 cast:
func EncodeLatLon(value float64, offset int64, granularity int32) int64 {
return (int64(value/.000000001) - offset) / int64(granularity)
}Two problems compound to produce incorrect output:
1. 0.000000001 is not exactly 1e-9 in float64.
The binary float64 closest to 1e-9 is slightly larger than the true mathematical value, so dividing by it yields a result slightly less than the true value × 1e9. For example:
raw DB integer: 422476590 (latitude stored at 1e7 scale)
float64 input: 42.247659 (integer ÷ 1e7, as produced by export SQL)
÷ 0.000000001: 42247658999.9999... ← just below the true integer
2. int64() truncates toward zero, not rounds.
That slightly-low result floors to N-1:
int64(42247658999.9999) = 42247658999 ← off by 1
÷ granularity (100) = 422476589 ← wrong; should be 422476590
Test
Here is a unit test that illustrates the rounding behavior:
func TestEncodeLatLon(t *testing.T) {
const granularity = int32(100)
cases := []struct {
input float64
want int64
}{
// values that encode correctly with both implementations
{42.248671, 422486710},
{42.247937, 422479370},
{42.246574, 422465740},
{-113.925921, -1139259210},
// values that produce off-by-one with the truncating implementation
{42.247659, 422476590},
{42.247491, 422474910},
{42.247237, 422472370},
{42.246756, 422467560},
{42.246331, 422463310},
{42.246081, 422460810},
{-113.925918, -1139259180},
{-113.923779, -1139237790},
}
for _, tc := range cases {
got := EncodeLatLon(tc.input, 0, granularity)
if got != tc.want {
t.Errorf("EncodeLatLon(%v) = %d, want %d (delta %d)", tc.input, got, tc.want, tc.want-got)
}
}
} osm/osmpbf/encode_test.go:359: EncodeLatLon(42.247659) = 422476589, want 422476590 (delta 1)
osm/osmpbf/encode_test.go:359: EncodeLatLon(42.247491) = 422474909, want 422474910 (delta 1)
osm/osmpbf/encode_test.go:359: EncodeLatLon(42.247237) = 422472369, want 422472370 (delta 1)
osm/osmpbf/encode_test.go:359: EncodeLatLon(42.246756) = 422467559, want 422467560 (delta 1)
osm/osmpbf/encode_test.go:359: EncodeLatLon(42.246331) = 422463309, want 422463310 (delta 1)
osm/osmpbf/encode_test.go:359: EncodeLatLon(42.246081) = 422460809, want 422460810 (delta 1)
osm/osmpbf/encode_test.go:359: EncodeLatLon(-113.925918) = -1139259179, want -1139259180 (delta -1)
osm/osmpbf/encode_test.go:359: EncodeLatLon(-113.923779) = -1139237789, want -1139237790 (delta -1)
Description
This PR adds a PBF Encoder. It is inspired by
OsmSharp's encoder. Most likely this code can be simplified and made moregoidiomatic.The
Encoderhas a methodEncode(obj osm.Object) errorwhich can be used to encode structs of typeosm.Object.Resolves #48