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

[GEOT-5502] First implementation of Pole of inaccessibility. #1287

Merged
merged 3 commits into from Aug 28, 2016

Conversation

@chau-intl
Copy link
Contributor

chau-intl commented Aug 27, 2016

Vladimir Agafonkin came up with an algorithm for calculating poles of inaccessibility to give good labelling points for odd shaped polygons.

See https://www.mapbox.com/blog/polygon-center/ for a discussion of algorithm

This implements this functionality and makes it available as a function so it could be added to GeoServer SLD files.

*
*/
public class PolyLabeller {
private static final Logger LOGGER = Logging.getLogger(PolyLabeller.class.getName());

This comment has been minimized.

Copy link
@smithkm

smithkm Aug 27, 2016

Member

Using tabs, should be spaces.

super(Text.text("PolygonLabelProcess"), "polygonlabelprocess", PolygonLabelProcess.class);
}

@DescribeProcess(title = "Polygon label process", description = "Calculate the the Pole of accessibility, the most distant interior point i a polygon.")

This comment has been minimized.

Copy link
@smithkm

smithkm Aug 27, 2016

Member

Typo, 'i a polygon' Should be 'in'?

// outside)
private double pointToPolygonDist(Point point, MultiPolygon polygon) {
boolean inside = polygon.contains(point);
/* double minDistSq = Double.POSITIVE_INFINITY;

This comment has been minimized.

Copy link
@smithkm

smithkm Aug 27, 2016

Member

Best to remove commented out code before submitting. If it's really important that it be retained, use a comment to explain why.

private double d;
private double max;

Cell(double x, double y, double h, MultiPolygon polygon) {

This comment has been minimized.

Copy link
@smithkm

smithkm Aug 27, 2016

Member

Comment explaining the class would be nice. Even if it's only meant for implementation, it will help with maintaining.

@ianturton
Copy link
Member

ianturton commented Aug 27, 2016

Casper will sign CLA on Monday when he has checked his contract

@aaime
Copy link
Member

aaime commented Aug 27, 2016

Thinking out loud and without having checked the pull request.... is this worth turning into a text symbolizer vendor option too? If it's fast enough it could become the default poly center implementation, with the vendor option still allowing it to turn it off and get the old behavior, but most of the people would get the benefit without needing to alter all their SLDs.

@ianturton
Copy link
Member

ianturton commented Aug 27, 2016

Once we've tried this out on a some realistic polygons we could consider moving it into the main styling package and making it the default.

@smithkm
Copy link
Member

smithkm commented Aug 27, 2016

OK nitpicks addressed. Looks good.

@smithkm smithkm merged commit 30ccc3b into geotools:master Aug 28, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dr-jts
Copy link
Contributor

dr-jts commented Oct 18, 2018

A bit late to the party here.... is this algorithm in use at all?

There's a few places where it can be sped up dramatically:

  • Use IndexedPointInArea rather than contains
  • Use IndexedFacetDistance rather than plain Distance
  • With these in use there is no need to check for polygon validity

I have implemented a version like this and the performance goes from 153 s to 680 ms on a world polygon dataset. Will be posting it soon.

@aaime
Copy link
Member

aaime commented Oct 18, 2018

We tried it out hoping it would lead to better label points, the results have not been very satisfactory to our cartographers (and we verified implementation making QGIS compute the same using its pole of inaccesibility implementation, which returned the same points). Looks like MapBox is using something else now, as their maps have the labels in better positions than the pole. Still, good to have it improved, in some cases it's better than the built-in one we have.

@aaime
Copy link
Member

aaime commented Oct 18, 2018

And oh, one thing, don't expect valid polygons to go into the algorithm, the code has been already modified (just recently) to allow it working on non valid ones, and the check for polygon validity is gone. I'd suggest checking the latest version on master.

@dr-jts
Copy link
Contributor

dr-jts commented Oct 18, 2018

Thanks for pointing out the updated version. That should be a lot better (not requiring validity, and should be faster).

Interesting that this isn't considered ideal labelling. I did notice that sometimes the point is far from the centre of the polygon, when there's still enough room there for a label. Perhaps a combination of trying the centre and the furthest point would be better.

@dr-jts
Copy link
Contributor

dr-jts commented Oct 18, 2018

Testing indicates the latest version of PolyLabeller is indeed a lot faster. A version using indexed Point-in-Polygon and Distance is still faster, but only about 3x on a "typical" dataset. So probably not worth the (much) greater complexity (in terms of dependencies).

@dr-jts
Copy link
Contributor

dr-jts commented Oct 31, 2018

@aaime etc: Following up on this... It looks like Mapnik is now using a modified PolyLabeller algorithm, which provides some weighting from the polygon centroid to stop the compute point being too far away. See this:
mapnik/mapnik#3811

@giohappy
Copy link

giohappy commented Nov 6, 2018

Nice the centroid fix approach. It's something we also were thinking about to improve the PolyLabel efficacy for labeling. Indeed, from our experience, the pole of inaccessibility often doesn't give great visual results (and most of map services don't employ it straight).
We could consider porting it into GT one day...

@dr-jts
Copy link
Contributor

dr-jts commented Dec 5, 2018

I found a bug in the master version of this code. The code uses getCoordinates() on polygon geometries. This is incorrect if the polygon contains holes, since it merges all coordinates from all rings into a single let. (It's also slow for polygons with many holes).

The fix is to add a 3rd level to the loop to handle polygon shell and hole rings separately.

Even better, use IndexdFacetDistance to make it much faster.

@aaime
Copy link
Member

aaime commented Dec 5, 2018

Thanks for the report @dr-jts
Right now nobody is "owning" that code, it's in an unsupported module. Maybe @ianturton wants to have a look? Otherwise PRs welcomed :-) (if nobody takes it, maybe best to open a ticket to avoid losing the intel).

@dr-jts
Copy link
Contributor

dr-jts commented Dec 5, 2018

Sure, if no-one else picks up on this I will open a new issue.

For the record, I have an implementation using indexing here.

(I also have ideas about how to constrain the point to a rectangle as well, which might be more effective for labelling, as well as being faster again).

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.