NetTopologySuite / NetTopologySuite.IO.VectorTiles

A package that can be used to generate vector tiles using NTS.
Other
19 stars 13 forks source link

Mapbox Vector Tiles: Integer local tile coordinates #23

Closed kolyanch closed 1 year ago

kolyanch commented 1 year ago

I found a problem when converting some objects. When reading objects through MapboxTileReader written through MapboxTileWriter, some objects may become invalid. The tile cannot be read with the error "No shell defined" in the reader. After much research, I found that the numbers were incorrectly rounded when recalculating from "global" tile coordinates to "local" ones. This led to the fact that some complex objects could have self-intersections after MapboxTileWriter, which would lead to an incorrect definition of the winding order and, as a result, to the above error.

The problem was found in the following code: https://github.com/NetTopologySuite/NetTopologySuite.IO.VectorTiles/blob/b75d308e1d5ba35683a7d7ec29273526696f6984/src/NetTopologySuite.IO.VectorTiles.Mapbox/TileGeometryTransform.cs#L68

In the test, the global tile coordinates were equal to (2669673, 2819412.0000000005) Calculating the local coordinates inside the tile by subtraction resulted in a shift of one pixel due to .0000000005.

To get rid of this problem, it was possible to add another type conversion and truncate tile coordinates. It would look like this:

int localX = (int) ((long)pixels.x - _left);
int localY = (int) (_top - (long)pixels.y);

Since it all looks very strange (double -> long -> int), I decided to redo the methods for calculating tile coordinates so that they immediately return integer values.

Also moved the IsGreaterThanOnePixelOfTile method. It should also have given incorrect results, since the tile size was always 512

rbrundritt commented 1 year ago

Overall, this looks fine, however I think some of the changes may break existing implementations. Most notably going from

public static (double x, double y) MetersToPixels((double x, double y) m, int zoom, int tileSize = 512) 

to

public static (long x, long y) MetersToPixels((double x, double y) m, int zoom, int tileSize = 512)

as existing code would need to be updated to handle long's being returned rather than double which will likely require some casting somewhere further down the line.

Similarly, going from:

public static (double x, double y) PixelsToMeters((double x, double y) p, int zoom, int tileSize = 512)

to

public static (double x, double y) PixelsToMeters((long x, long y) p, int zoom, int tileSize = 512)

would require existing code to be updated to cast the input numbers to long.

Not big issues and likely only a few extra minutes of work for a dev updating a code base but this could be an issue if any code bases automatically pick up the latest version in a build pipeline. Note sure if anyone is doing that yet as this library as it is still fairly new.

It might be good to have this pushed out through a new major version number.

kolyanch commented 1 year ago
  1. Renamed the modified coordinate transformation methods in the WebMercatorHandler class.
  2. Returned the old ones for compatibility.
  3. Added some comments to the code

The implementation is currently without any generics, especially since it is difficult to abstract overloads here, since methods sometimes differ only in return values.