#1231 Use polygon centroid for 'point' symbol placements #2678

blanchg · 2016-06-06T11:30:00Z

For Issue #1231. This is a first pass that uses a slightly modified centroid function from turf-centroid.js

It works great for simple polygons, but it doesn't handle holes or complex polygons.

For holes, maybe the solution would be to look at using the formula for geometric decomposition

For complex polygons (e.g. river polygons ) a more complex straight polygon tree could be used but I haven't found a nicie clean fast solution for that yet.

This is also my first PR for mapbox-gl-js, so please let me know if I have missed something.

mourner · 2016-06-06T20:52:51Z

js/util/util.js

+        xSum += coord.x - xC;
+        ySum += coord.y - yC;
+        len++;
+    });


Please replace with a simple for loop for performance and simplicity.

Also, I think we should use polygon centroid algorithm like this https://en.wikipedia.org/wiki/Centroid#Centroid_of_polygon, which will choose a better center.

Excellent thanks for pointing it out. There was an error in the function on wikipedia that has now been corrected. I have the additional changes but it causes tests to fail if we keep this change as symbol-placment: 'point'.

The side effect is that lineStrings will use the centroid which I believe is incorrect. I did ask if a symbol-placment: 'centroid' should be used in #1231. This would require changes to the style spec as well, I may need some help/pointers to understand how that is also updated/released.

dpieri · 2016-06-08T01:31:23Z

@blanchg thanks for starting this! Lack of this feature has been a huge pain point for us.

In my opinion, Pole of Inaccessibility is the ideal way to position labels on polygons. I can't speak to the performance implications of doing it in real time compared to a centroid. If they are negligible then I hope you would consider using this method instead.

Here's a Turf module that implements it, and some more background.

blanchg · 2016-06-08T03:04:01Z

That looks like a great solution, however that turf module uses turf-buffer which uses jsts to do the actual bufferring, that would increase the size of mapbox significantly. I will look to see by how much if we only include the polygon offset/buffer code from jsts.

blanchg · 2016-06-10T03:07:44Z

@dpieri I found a small library that performs the Pole Of Inaccessibility and extended it to support holes: https://github.com/blanchg/LabelPoint, however it appears to interfere when a lot of polygon draws so that on higher zoom levels the polygons stop rendering completely and the +/- zoom buttons never get displayed. Is there a way to find out why this is occuring (is there some sort of timeout maybe?) without going step by step through the code?

dpieri · 2016-06-10T21:42:08Z

I'm not sure, maybe @mourner could point you in the right direction.

blanchg · 2016-06-12T21:56:48Z

Thanks, I found that the simplification can produce zero area polygons with multiple points that was causing issues.

blanchg · 2016-06-13T03:07:01Z

This is ready now. It continues to use symbol-placment point.

If the ring is closed e.g. first and last point are the same and has an area != 0 then it chooses the Pole of Inaccessibility otherwise it falls back to the first point as it was before. This works then for points, lines and polygons that have been simplified down to a point/line.

It adds about 1-2kb to the output, fps benchmarks look very similar.

I am positive there are things I can do better so please let me know! Thanks

mourner · 2016-06-13T09:05:23Z

js/util/counter_clockwise.js

+// http://bryceboe.com/2006/10/23/line-segment-intersection-algorithm/
+module.exports = function (a, b, c) {
+    return (c.y - a.y) * (b.x - a.x) > (b.y - a.y) * (c.x - a.x);
+};


I don't think we need to cut out small functions like this into a separate file. Maybe move to util.js? Or maybe we should have geom_util.js for all functions like this.

mollymerp · 2016-06-13T20:54:58Z

@blanchg thank you for contributing!
could you please:

rebase your PR to squash your commits and remove commits from other PRs? you can use git rebase -i origin/master to do this.
add unit tests and/or render tests

thanks!

👓 @lucaswoj @mourner @mollymerp for review

blanchg · 2016-06-14T06:05:30Z

Thanks for the reviews @mollymerp and @mourner !

I managed to squish the changes and remove the unrelated ones thanks for pointing that out. I added some unit tests for polygon_poi and I have a render test but I am uncertain how to proceed to get the render test included.

I have a feature branch on a fork, I assume I do a PR on the test suite that should only be pulled in once this PR has been?

blanchg · 2016-06-14T12:43:04Z

I found that building polygons in the render test suite data have outer rings that don't match the tile specification (2.1). They have both positive and negative areas as outer rings. I also found util/classify_rings that handles both cases. Will update to use that.

blanchg · 2016-06-14T21:06:02Z

Updated to use util/classify_rings which now handles polygons and holes of any winding order. One raster test will fail until it is updated. I still need some information on how to get the test-suite aligned with this PR.

mollymerp · 2016-06-15T17:48:04Z

I still need some information on how to get the test-suite aligned with this PR.

You can see information on how to update the tests here. Once your tests are satisfactory, make a PR in mapbox/mapbox-gl-test-suite repo and then you can update the sha for that commit in the package.json for this PR.

I don't see the raster tests failing, I just see the debug/tile test failing. Any idea why that might be happening?

@mourner I think you should have final review here.

blanchg · 2016-06-15T19:41:10Z

@mollymerp Thanks for the information. I have that PR submitted now.

The debug/tile test fails because one of the labels is in a closed polygon so it is now rendered at the center of that polygon, every other test doesn't render labels on closed polygons.

I have added the line style and zoomed to the label that moved slightly in the debug/tile test to highlight why it is different.

blanchg · 2016-06-17T01:29:48Z

This is now up to date with the head, squashed, unit tests, there is a render test added and updated the failing debug test. PR in the test-suite and sha pointing to it in package.json.

I think finally it is ready to merge 😃 🎉 🎈

mourner · 2016-06-17T01:31:03Z

Fantastic! I'll do a final review tomorrow and merge if everything's good.

dpieri · 2016-06-29T19:47:52Z

Any idea when this will make it into a point release?

mourner · 2016-06-29T19:59:53Z

This got a bit sidetracked but hopefully we will merge this week, followed by a point release next week.

mourner · 2016-07-06T15:08:10Z

@blanche hey, can you rebase this branch and the test suite one on master?

blanche · 2016-07-06T15:44:57Z

i think you ment @blanchg ;)

lucaswoj · 2016-08-17T19:05:33Z

js/data/bucket/symbol_bucket.js

@@ -14,6 +14,8 @@ var clipLine = require('../../symbol/clip_line');
 var util = require('../../util/util');
 var loadGeometry = require('../load_geometry');
 var CollisionFeature = require('../../symbol/collision_feature');
+var poleOfInaccessibility = require('../../util/polygon_poi');


An (admittedly unwritten) convention in GL JS is that file names match the names of the classes / functions they export. Can we rename polygon_poi -> find_pole_of_inaccessibility or similar?

lucaswoj · 2016-08-17T19:07:30Z

This looks good to me! I'm excited to 🚢 .

@mourner

uses a copy of @mourner mapbox/polylabel to find it

blanchg · 2016-08-17T22:32:27Z

@mourner @mollymerp @lucaswoj Thank you all for your help, I think this is finally ready :D 👍

mourner · 2016-08-18T14:26:20Z

Set up a JSBin debug page to demonstrate how it works in practice, using the choropleth from examples as a base but adding a population label: http://output.jsbin.com/tajiwociha

Generally it's pretty cool, but there are a few issues that are worth noting, although they don't necessarily warrant an immediate fix, and not all are possible to fix (e.g. because of tiling or the the way algorithm works).

You have to use "symbol-avoid-edges": true, otherwise there will be collisions across tile boundaries because the neighboring points don't match (they represent different parts cut off from a polygon by tiling)
Due to buffers around polygons clipped by tile boundaries, labels in such polygons are placed closer to the boundary than needed; the polygons should probably be clipped before running the algorithm for more correct results.
Polygons split across several tiles will of course have several labels.
Label positions sometimes "jump" between zoom levels, because the algorithm can pick a different point in a higher-precision/bigger coords version of the same polygon; this is exaggerated on this particular example because of many rectangle-like shapes.

@samanpwbb can you play with the sample and tell us what you think? Should we go ahead with the feature in its current state?

samanpwbb · 2016-08-18T14:39:20Z

It's much better than what we have now, so I believe that it is worth shipping in it's current state.

You have to use "symbol-avoid-edges": true

That's fine – i know this is a hard / impossible problem to fix. Mapnik was similar.

Due to buffers around polygons clipped by tile boundaries, labels in such polygons are placed closer to the boundary than needed

This problem is noticeable – definitely worth following up on:

Polygons split across several tiles will of course have several labels.

That's fine, Mapnik had the same issue. vector tiles ¯_(ツ)_/¯

Label positions sometimes "jump" between zoom level

Not a big deal.

I'm also seeing an issue with labels on features I expect to be smaller parts of multipolygons. Is this just a data issue, or is mapbox gl unable to distinguish multipolygons?

mourner · 2016-08-18T14:43:34Z

I'm also seeing an issue with labels on features I expect to be smaller parts of multipolygons. Is this just a data issue, or is mapbox gl unable to distinguish multipolygons?

It currently just adds a label for every polygon in a multipolygon. We could alter the algororithm to filter out labels for polygon where anchor point has a distance to the polygon below a certain threshold, but this would probably require a new obscure style-spec property, or have a very good default. Another option is to always only add one label to a multipolygon, choosing an anchor in one of the polygons that fits the best.

mourner · 2016-08-18T14:53:52Z

It's also worth noting that the performance cost on the worker side is pretty high — on this example but without a basemap (just leaving the population map alone), placing polygon anchors takes ~18% of the worker time. If I switch from 2px to 4px precision, it takes ~13%. It may be acceptable though, since this is an opt-in feature, and doesn't affect our styles — only custom visualizations where you explicitly want to use auto polygon anchors.

samanpwbb · 2016-08-18T14:55:00Z

Another option is to always only add one label to a multipolygon, choosing an anchor in one of the polygons that fits the best.

I can see cases where both would be desireable, but most of the time a multipolygon should only get a single label. you'd almost never want to include more than one label on a choropleth data viz (and this could even result in a map that 'lies' – making it look like a place has double the value that it actually has).

On the other hand, a cartographer may want to place a tree icon in the center of each piece of a park multipolygon.

I vote to enforce 1 label per multipolygon, leaving open the option of adding a new property to give users control in the future.

mourner · 2016-08-18T15:01:38Z

@samanpwbb makes sense! Regarding polygons that have labels placed close to boundaries, I referred to a different case:

Here, the label is not further up because it calculates anchor based on the polygon geometry with a buffer, including an "invisible" part on the bottom.

And regarding the case below, — it's just a property of the algorithm — in this case a label in the center may have a few pixels smaller distance to the polygon, and considered worse than the one picked; to fix this, we would have to modify the algorithm to somehow prefer points closer to centroid even if they have smaller distance, but this is not trivial.

samanpwbb · 2016-08-18T15:28:39Z

We could alter the algororithm to filter out labels for polygon where anchor point has a distance to the polygon below a certain threshold

After thinking more about the problem and various edge cases, this is my favorite option for handling multi-poly labelling. It wouldn't add a new property and would ideally look good in most cases. I also recognize that this is the hard approach. Again, like I said in the original post, my take on this PR is that it's a major improvement over what we have now no matter what, and I think we should merge something imperfect sooner (and ticket follow up work) rather than wait until it's perfect.

mourner · 2016-08-18T15:31:18Z

It wouldn't add a new property and would ideally look good in most cases.

How do we pick the threshold then? E.g. something arbitrary like 2 "ems" assuming em = 24px?

samanpwbb · 2016-08-18T15:42:35Z

How do we pick the threshold then? E.g. something arbitrary like 2 "ems" assuming em = 24px?

Yep something like that sounds good. See how it feels.

Let me bring up another idea. I don't know if it's at all technically feasible to incorporate into your label placement algorithm. Archipelago-type features are really common in the world. Louisiana is mostly a single mass, but has an archipelago on it's southern tip. But what about the Philippines?

Is there a way to incorporate multipolygons into the label placement algorithm in such a way that the label still ends up getting placed inside a shape, so we are able to handle archipelagos well?

blanchg · 2016-08-18T19:57:02Z

I found if you ignore polygons that have a cellSize < precision * 2 it seems to handle the US quite well and fixes the label showing up on the islands off Louisiana and Texas when you are zoomed out. This could cause issues for archipelagos though as it won't show any labels until one of the islands is a few pixels big. Seems to speed up the rendering too.

http://jsbin.com/lopilikohe/1

`
var cellSize = Math.min(width, height);

if (cellSize < precision * 10) {
return null;
}
`

fedex1 · 2016-08-19T03:21:11Z

Here's an example that I'm using to test.

Before centroid change: https://city.tidalforce.org/nycschoolallocation_usm.html
See the "white label" "District 16"

With the centroid change (I cloned mapbox-gl-js and switched to the pull request branch and built)
https://city.tidalforce.org/nycschoolallocation_usm_centroid.html

It is definitely an improvement.

A few questions:

Is there supposed to be multiple labels for a small polygon? this may be a result of my data so I apologize if this is a data issue.
I noticed that the copyright style change


 map.style.sources['mapbox://cityregister.nyc_school_allocation_titlei'].attribution = [
                '&copy;BrooklynMarathon',
                '<a href="http://schools.nyc.gov/offices/d_chanc_oper/budget/dbor/allocationmemo/fy16_17/FY17_PDF/sam08_T2.xls">City of New York, Department of Education</a>'
            ].join(' | ');

no longer seems to work.

as it did in

<script src='https://api.tiles.mapbox.com/mapbox-gl-js/v0.19.1/mapbox-gl.js'></script>
<link href='https://api.mapbox.com/mapbox-gl-js/v0.19.1/mapbox-gl.css' rel='stylesheet' />

blanchg · 2016-08-19T03:34:56Z

@fedex1 I don't think this PR has any changes that impact the copyright maybe the others can comment on this
Those multiple labels are likely due to the tiles if you turn on the tile debug lines it should highlight this.

👍 for giving it a go and demonstrating feedback

fedex1 · 2016-08-19T04:49:42Z

@blanchg Yes that is correct. Thanks for the reply!

with debug lines on. https://city.tidalforce.org/nycschoolallocation_usm_centroid.html

lucaswoj · 2016-08-19T18:28:52Z

Rebased and squashed 👉 #3038

anna-geo · 2016-09-13T14:23:53Z

Could you please explain if it is possible now to show only one text label in polygon center even if it`s splited into several tiles? I would like to avoid situation like on the last image of @fedex1 where label "District 16" is repeated several times inside one district.

mourner · 2016-09-13T15:32:24Z

@anna-geo currently not possible unfortunately, and a very hard issue to address.

lucaswoj · 2016-09-13T18:17:46Z

@anna-geo I suggest that you do that processing during tile generation time and produce point geometries where the label should go.

anna-geo · 2016-09-15T12:21:17Z

@mourner @lucaswoj Thank you for reply! Will be waiting for this feature.

mourner reviewed Jun 6, 2016
View reviewed changes

mourner reviewed Jun 13, 2016
View reviewed changes

blanchg force-pushed the polygon-centroid-labels-1231 branch 3 times, most recently from 0262c40 to 36a7aeb Compare June 14, 2016 05:55

blanchg force-pushed the polygon-centroid-labels-1231 branch from 36a7aeb to d22feac Compare June 14, 2016 21:03

blanchg mentioned this pull request Jun 14, 2016

Added symbol-placement point tests for polygons mapbox/mapbox-gl-test-suite#111

Merged

blanchg force-pushed the polygon-centroid-labels-1231 branch 3 times, most recently from 8da9d99 to 0d3eaac Compare June 17, 2016 01:25

lucaswoj assigned mourner Jun 29, 2016

lucaswoj reviewed Aug 17, 2016
View reviewed changes

Issue mapbox#1231 Use polygon centroid for 'point' symbol placements

ae1b95a

uses a copy of @mourner mapbox/polylabel to find it

blanchg force-pushed the polygon-centroid-labels-1231 branch from 072f81d to ae1b95a Compare August 17, 2016 21:30

lucaswoj mentioned this pull request Aug 19, 2016

Use polygon centroid for 'point' symbol placements #3038

Merged

lucaswoj closed this Aug 19, 2016

lucaswoj mentioned this pull request Aug 22, 2016

Add support for pole-of-inaccessibility symbol placement mapbox/mapbox-gl-native#6115

Closed

lucaswoj mentioned this pull request Dec 21, 2016

[core] Polylabel-based "pole of inaccessibility" symbol placement mapbox/mapbox-gl-native#7465

Merged

stevage mentioned this pull request Sep 13, 2017

Text labels on polygons mapbox/mapbox-studio-classic#1043

Open

kmartastic mentioned this pull request Sep 17, 2020

[Maps] support labels for lines and polygons elastic/kibana#63267

Closed

palhal mentioned this pull request Jun 19, 2024

symbol layer icon image repeated #10725

Closed

#1231 Use polygon centroid for 'point' symbol placements #2678

#1231 Use polygon centroid for 'point' symbol placements #2678

Conversation

blanchg commented Jun 6, 2016 • edited Loading

mourner Jun 6, 2016

Choose a reason for hiding this comment

mourner Jun 6, 2016

Choose a reason for hiding this comment

blanchg Jun 7, 2016

Choose a reason for hiding this comment

dpieri commented Jun 8, 2016

blanchg commented Jun 8, 2016

blanchg commented Jun 10, 2016 • edited Loading

dpieri commented Jun 10, 2016

blanchg commented Jun 12, 2016

blanchg commented Jun 13, 2016

mourner Jun 13, 2016

Choose a reason for hiding this comment

mollymerp commented Jun 13, 2016

blanchg commented Jun 14, 2016

blanchg commented Jun 14, 2016

blanchg commented Jun 14, 2016

mollymerp commented Jun 15, 2016

blanchg commented Jun 15, 2016 • edited Loading

blanchg commented Jun 17, 2016

mourner commented Jun 17, 2016

dpieri commented Jun 29, 2016

mourner commented Jun 29, 2016

mourner commented Jul 6, 2016

blanche commented Jul 6, 2016

lucaswoj Aug 17, 2016

Choose a reason for hiding this comment

lucaswoj commented Aug 17, 2016

blanchg commented Aug 17, 2016

mourner commented Aug 18, 2016 • edited Loading

samanpwbb commented Aug 18, 2016

mourner commented Aug 18, 2016

mourner commented Aug 18, 2016

samanpwbb commented Aug 18, 2016

mourner commented Aug 18, 2016

samanpwbb commented Aug 18, 2016

mourner commented Aug 18, 2016

samanpwbb commented Aug 18, 2016 • edited Loading

blanchg commented Aug 18, 2016

fedex1 commented Aug 19, 2016 • edited Loading

blanchg commented Aug 19, 2016

fedex1 commented Aug 19, 2016

lucaswoj commented Aug 19, 2016

anna-geo commented Sep 13, 2016

mourner commented Sep 13, 2016

lucaswoj commented Sep 13, 2016

anna-geo commented Sep 15, 2016

blanchg commented Jun 6, 2016 •

edited

Loading

blanchg commented Jun 10, 2016 •

edited

Loading

blanchg commented Jun 15, 2016 •

edited

Loading

mourner commented Aug 18, 2016 •

edited

Loading

samanpwbb commented Aug 18, 2016 •

edited

Loading

fedex1 commented Aug 19, 2016 •

edited

Loading