Skip to content
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.

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

VectorTile Geometry Recentering #3036

Merged
merged 1 commit into from Sep 7, 2019

Conversation

@jbouffard
Copy link
Contributor

commented Jul 23, 2019

Overview

This PR fixes an error that can sometimes cause geometries to be dropped and/or shifted when reading them from a VectorTile and then writing them back. The issue was caused by using the top left corner of the pixel of the tile as the reference for the projected coordinates; as this would cause some projected points to either move, or fall outside of the tile when converting those projected coordinates back to VectorTile coordinates.

The solution was then to change the reference point from being in the corner of the pixel to being in the center.

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • Unit tests added for bug-fix or new feature

Notes

I think I will need to talk with someone more before I can remove the [WIP] from this PR. Because right now the changes introduced causes other tests to fail, and I'm not sure if that behavior is okay or not. Also, I've not been able to recreate the error this PR was intended to resolve, so a unit test needs to be created before this is ready for review.

Closes #2926

@mojodna

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Yeah, this feels wrong to me...

it("Point") {
val ns = Seq(9, 4, 4)
val p = implicitly[ProtobufGeom[Point, MultiPoint]].fromCommands(
Command.commands(ns),
topLeft,
resolution
)
p shouldBe Left(Point(2, 4094))
Command.uncommands(implicitly[ProtobufGeom[Point, MultiPoint]].toCommands(
p,
topLeft,
resolution
)) shouldBe ns
}
makes sense. moveTo(2, 2) on a 4096 grid should be (2, 4094).

The alternative is that topLeft should be adjusted (by resolution / 2) when passed into toProjection (in protoPoint and friends?) in order to align it to a pixel center rather than a pixel corner. That seems like it would fix the test failures that cropped up.

Does GT do something similar elsewhere to align to centers?

@pomadchin pomadchin changed the title [WIP] VectorTile Geometry Recentering VectorTile Geometry Recentering Jul 25, 2019

@jbouffard

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@mojodna To my knowledge, we don't do anything to recenter the geometries in GT, so I implemented it in toProjection. @jpolchlo do you know if we do actually do it elsewhere?

Though, it appears that the above implementation doesn't produce the exact result we're looking for. I created a unit test of the failing case, and it doesn't pass because the decimals aren't the same

[info] ProjectionSpec:
[info] VectorTile Projection Conversions
[info] - should read a point from a VectorTile and write it back *** FAILED ***
[info]   POINT (-61.34490966796874 10.409481792727018) did not equal POINT (-61.3422 10.4068) (ProjectionSpec.scala:38)

I'm not sure if that difference is okay, or if we need to implement something different.

@@ -121,9 +121,12 @@ package object internal {
*
*/
private[vectortile] def toProjection(point: (Int, Int), topLeft: Point, resolution: Double): Point = {
val translatedX: Double = topLeft.x + (resolution / 2)
val translatedY: Double = topLeft.y + (resolution / -2)

This comment has been minimized.

Copy link
@mojodna

mojodna Jul 25, 2019

Contributor

Changing the translation to a full point (from half a point here) further reduces the delta between expected and actual results. It's still ~4 meters (horizontally; ~2m vertically) off after that though.

This comment has been minimized.

Copy link
@mojodna

mojodna Jul 25, 2019

Contributor

(This presumes that the expected coordinate is correct, though (see below).)

class ProjectionSpec extends FunSpec {
describe("VectorTile Projection Conversions") {
it("should read a point from a VectorTile and write it back") {
val p = Point(-61.3422, 10.4068)

This comment has been minimized.

Copy link
@mojodna

mojodna Jul 25, 2019

Contributor

This coordinate was extracted from a live MVT using vt2geojson, so it uses this projection math:

https://github.com/mapbox/vector-tile-js/blob/58df1e9344ee64f26deee84a9f54cee11fb95ef6/lib/vectortilefeature.js#L137-L145

As a result, I'm not sure what value is correct for translation.

This comment has been minimized.

Copy link
@jbouffard

jbouffard Jul 25, 2019

Author Contributor

I see. That's good to know. It was suggested that I do a third writing/read of the point to see if it continues to shift. This is the code I used to run this test:

    it("should read a point from a VectorTile and write it back") {
      val p1 = Point(-61.3422, 10.4068)
      val reprojected = point.reproject(LatLng, WebMercator)
      val tileExtent =
        Extent(-6887893.4928338025, 1095801.2374962866, -6809621.975869782, 1174072.7544603087)
        //SpatialKey(168, 241).extent(ZoomedLayoutScheme(WebMercator).levelForZoom(9).layout)

      val f = MVTFeature(reprojected, Map.empty[String, Value])

      val layer = StrictLayer(
        name = "test",
        tileWidth = 128,
        version = 2,
        tileExtent = tileExtent,
        points = Seq(f),
        multiPoints = Seq.empty,
        lines = Seq.empty,
        multiLines = Seq.empty,
        polygons = Seq.empty,
        multiPolygons = Seq.empty
      )

      val vt = VectorTile(Map("test" -> layer), tileExtent)

      val vt2 = VectorTile.fromBytes(vt.toBytes, tileExtent)
      val p2 = vt2.layers("test").features.head.geom.reproject(WebMercator, LatLng)

      val vt3 = VectorTile.fromBytes(vt2.toBytes, tileExtent)
      val p3 = vt3.layers("test").features.head.geom.reproject(WebMercator, LatLng)

      println(s"\n\nThis was the initial point: $p1")
      println(s"This is the point after its been written: $p2")
      println(s"This is the point after its been written, read in, and written out again: $p3")
    }

And these are the results:

This was the initial point: POINT (-61.3422 10.4068)
This is the point after its been written: POINT (-61.33666992187498 10.401377554543565)
This is the point after its been written, read in, and written out again: POINT (-61.33117675781249 10.395974612177667)

So it appears that the geometry continues to be shifted to the up and left. This was using the transformation of a full point instead of a half point like you commented above. I wouldn't expect p1 to be equal to p2 or p3, but I'd expected p2 and p3 to be the same.

This comment has been minimized.

Copy link
@mojodna

mojodna Jul 25, 2019

Contributor

With further exploration here, Point(-61.347656249999986, 10.412183158667512) is a good starting point (ha!) that round trips with half a point as the offset.

Using that, p1 == p2 == p3.

@pomadchin pomadchin added on hold and removed in progress labels Aug 28, 2019

@jbouffard jbouffard force-pushed the jbouffard:fix/vectortile-recenter branch from 10da0a9 to 5904bc4 Sep 6, 2019

@jbouffard

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

So it appears that this issue has been fixed at somepoint these past few months, as the ProjectionSpec now passes without needing to change any of the code. Therefore, this PR just adds the spec to ensure that it continues to work in the future.

build.sbt Outdated Show resolved Hide resolved

@pomadchin pomadchin removed the on hold label Sep 6, 2019

Changed the ProjectionSpec to test VectorTile reading/writing of geom…
…etries

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Created the ProjectionSpec

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Updated the ProjectionSpec

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Continued work on resolving bug

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Fixed the ProjectionSpec so that it passed

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Cleaned up code

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

@jbouffard jbouffard force-pushed the jbouffard:fix/vectortile-recenter branch from 5904bc4 to e3e14ef Sep 6, 2019

@pomadchin

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Thank you @jbouffard, merging once Travis is green. 🎉

@pomadchin pomadchin merged commit 36ea0be into locationtech:master Sep 7, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
@mojodna

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

@jbouffard do you have any idea which commit fixed things? i'm curious how this got fixed in places we weren't looking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.