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

Option dont_clip for layer #562

Closed
tomass opened this issue Dec 2, 2018 · 11 comments
Closed

Option dont_clip for layer #562

tomass opened this issue Dec 2, 2018 · 11 comments
Milestone

Comments

@tomass
Copy link

tomass commented Dec 2, 2018

It would be good to have a possibility to disable clipping for a specific layer. Something like dont_clip = true similar to dont_simplify.

While this could give speed and size improvements for simple geometries like buildings, main reason is use case described below:

I'm trying to create curved center line labels for water bodies. Algorithm on database side calculates a place where a name of waterbody in largest possible font would fit as well as letter spacing and stuff. This gives my a curved line for each label. Mapbox has already added a line label placement option label-center which should print a label in the center of the given line. But when center line overlaps more than one tile it is split by Tegola, so mapbox-gl-js is trying to put a center label on all parts separately which is not ok, because:

  • multiple labels are rendered (when they fit)
  • labels of large waterbodies could be too large to fit into one tile (because font size calculated could be 30 or even more)

Example:
image
Here a gray line in the middle of waterbody is a center line. Name "Kauno marios" should be written in the middle of it, but center line is split per tile (marked with red line). Also font size and letter spacing of "Kauno marios" should be larger, but in that case it does not fit in one tile and whole label is not rendered.

With clipping switched off I expect this center line to appear in all involved tiles. Label would be rendered multiple times but in the same spot, so should be ok (I have no way to test it now).

@gdey
Copy link
Member

gdey commented Dec 5, 2018

@justenPalmer Do you know if there is a way to do this via the client?

If you don't clip you will get errors from the client about geometries being out of the tile extent. If the geometry is already within the tile (doesn't need to be clipped), it will skip clipping as clipping is part of the make valid process. It sounds like this is something that should be handled on the client side?

@justenPalmer
Copy link

Yeah, this is something that's being addressed by the Mapbox GL JS team.

Issue was here: mapbox/mapbox-gl-js#5061

And this is the merge that closed it: mapbox/mapbox-gl-js#6821

@tomass
Copy link
Author

tomass commented Dec 6, 2018

I'm not sure I understand "handling on the client side" correctly. But from what I know Mapbox implemented possibility to place label at the centre of the line, because before that putting labels on "label-line" created ugly results with text being placed on one side or another of the line, sometimes text could be placed twice. line-center placement fixes that.
mapbox/mapbox-gl-js#6382

The issue here is that data in vector tile should not be clipped, so that client side would have a chance to calculate where the "centre" actually is. Errors on client side should only be popping up if vertexes in such line are waaaay too far away from the tile boundary (exceeding maxint?) and in my opinion it is responsibility of those creating queries for data to avoid such situations.

Also note: possibility to skip clipping would have general advantage, not connected to label placement: it would allow clipping geometry at data origin - on PostGIS side and then save time on Tegola side with skipping of clipping check.

@tomass
Copy link
Author

tomass commented Dec 8, 2018

I've done some "dirty programming" to try if this works after all. So I've copied the logic from dontsimplify to dontclip and then propagated the clip value down to the place where clipping is called.
Result looks promising, line does go through tile boundary, lines in different tiles do overlap perfectly thus creating one nice cross tile label:
paveikslas

@gdey
Copy link
Member

gdey commented Dec 11, 2018

@tomass Oh, cool. Do you want to send in a pull request with your changes? That way I can review it. Talking to Alex sounds like it is something we should go ahead and add.

@tomass
Copy link
Author

tomass commented Dec 13, 2018

I do not feel up to the task to provide the PR so Paulius has done that.
But we need your help. We do not know what has to be done with polygon/multipolygon geometries for "no clipping" in math/validate/validate.go function CleanGeometry.

@gdey
Copy link
Member

gdey commented Dec 14, 2018

@tomass I have not forgotten this issue, just been busy. I have to test this, but I think you can pass in a nil for the extent parameter of makePolygonValid, to disable clipping.

@tomass
Copy link
Author

tomass commented Dec 14, 2018

@gdey maybe nothing has to be done with polygons like nothing is done with lines?
Then a call of CleanGeometry could be skipped when dont_clip is true?
My reasoning is this: if no clipping is done in Tegola - then the only way geometry could be invalid is when it is invalid in source. And that (source) is the place where geometry should be fixed.

@gdey
Copy link
Member

gdey commented Dec 14, 2018

@tomass there is really only two things that make invalid geometries.

  1. Geometry is invalid at the source. (Not really worried about this one.)
  2. Geometry simplification -- this is the thing we need CleanGeometry for. Clipping doesn't normally invalidate the Geometries. Simplification does, because of point removal.

@tomass
Copy link
Author

tomass commented Dec 15, 2018

@gdey, @paumas has added setting of ext to nil when clipping is not required.
If idea/code looks fine I will test with polygons (waterbodies or more natural candidate for no clipping - buildings). So far I've only tested with lines.

ARolek added a commit that referenced this issue Apr 9, 2019
* implement dont_clip configuration option. closes #562.
@ARolek ARolek added this to the v0.9.0 milestone Apr 9, 2019
@ARolek
Copy link
Member

ARolek commented Apr 9, 2019

this is now in the v0.9.x branch. I'm working on cutting a version release.

@ARolek ARolek closed this as completed Apr 9, 2019
ARolek added a commit that referenced this issue Apr 9, 2019
* implement dont_clip configuration option. closes #562.
ARolek added a commit that referenced this issue Apr 9, 2019
* implement dont_clip configuration option. closes #562.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants