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

LineTo(0,0) and Invalidation of POLYGON and LINESTRING geometries #51

Closed
flippmoke opened this issue Dec 8, 2015 · 5 comments
Closed

Comments

@flippmoke
Copy link
Member

With mapnik-vector-tile implementation as I am upgrading to the stricter interpretation of V2, I have noticed an issue perhaps in our wording.

Within our decoder of geometries, I ignore lineto commands that do not move the cursor -- LineTo(0,0).

The LINESTRING geometry states only that the command count must be at least one. However, the logic in my decoder is that a lineto(0,0) doesn't contribute to the point count and therefore, I throw errors about invalid linestrings in situations where the lineto command count might be 1 or more.

The same is true in POLYGON geometries. However, this might be caught by the area requirement for rings.

The fix might be to clarify this situation in the polygon and linestring types.

@jfirebaugh
Copy link
Contributor

So the geometry in question is a LINESTRING consisting of MoveTo x,y; LineTo 0,0, and nothing else, and the implementation discards the zero-length LineTo command, and then throws an error because there are no more LineTo commands? I don't think this is in the spirit of the specification. The command count is not zero, it's one. There is no violation of a MUST requirement of the specification.

If you want this geometry to be prohibited by the specification, we should change 4.3.3.2 to a MUST NOT in "For any pair of (dX, dY) the dX and dY MUST NOT both be 0."

@flippmoke
Copy link
Member Author

I think the intent in 4.3.4.3 and 4.3.4.4 is to have a valid geometries, I do not think a MoveTo x,y; LineTo 0,0 creates a valid line?

@artemp
Copy link

artemp commented Dec 8, 2015

@jfirebaugh @flippmoke : I agree re:MUST NOT - "For any pair of (dX, dY) the dX and dY SHOULD NOT both be 0." - reads too vague, implying it can be 0.

@flippmoke
Copy link
Member Author

@artemp @jfirebaugh we probably would have to release 2.1 if we changed this wording? Unless I get some buy in to change current spec as we just released it?

@jfirebaugh
Copy link
Contributor

We should change 4.3.3.2 to a MUST NOT in "For any pair of (dX, dY) the dX and dY MUST NOT both be 0" for 2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants