search

mapbox / vt-pbf

Serialize Mapbox Vector Tiles to binary protobufs in javascript.
Other
186 stars 38 forks source link

Close polygon paths in writeGeometry #26

Closed joshhardy closed 6 years ago

joshhardy commented 6 years ago

Fixing issue in writeGeometry that leaves valid polygon geometries unclosed: https://github.com/mapbox/vt-pbf/issues/25

Was previously encoding the polygon closing path as lineto command rather than closepath, resulting in invalid vector tiles that cause feature display issues downstream. tippecanoe-decode was finding these polygons invalid with error: Ring does not end with closepath (ends with 2)

ryanbaumann commented 6 years ago

Cc @jfirebaugh @anandthakker

anandthakker commented 6 years ago

Thanks for this contribution @joshhardy!

The change looks 👍 to me. Ideally, we would include a unit test for this, but as of now I don't think there's a straightforward way to do this, so I'd say we go ahead with this change.

joshhardy commented 6 years ago

Great, thanks @anandthakker!

Ya, I didn't see any example tests verifying aspects of the serialized buffer - which is what we would ideally do. It seems like the only way the tests verify is to deserialize the buffer to make sure it matches what went in. With that approach, this change should be covered by the generic rectangle tests in test/basic.js.

What is the process/timeline to get this merged and published to npm?

anandthakker commented 6 years ago

Merged and released in 3.1.0 -- thanks again for the patch!

On Mon, Oct 30, 2017 at 7:17 PM Josh Hardy notifications@github.com wrote:

Great, thanks @anandthakker https://github.com/anandthakker!

Ya, I didn't see any example tests verifying aspects of the serialized buffer - which is what we would ideally do. It seems like the only way the tests verify is to deserialize the buffer to make sure it matches what went in. With that approach, this change should be covered by the generic rectangle tests in test/basic.js.

What is the process/timeline to get this merged and published to npm?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mapbox/vt-pbf/pull/26#issuecomment-340613360, or mute the thread https://github.com/notifications/unsubscribe-auth/AEvmR4Sc_PJp0BtewC3hguZ3xKNArFocks5sxlkbgaJpZM4QLs9D .