-
Notifications
You must be signed in to change notification settings - Fork 819
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
[WIP] Switch to ST_PointOnSurface #3636
[WIP] Switch to ST_PointOnSurface #3636
Conversation
Resolves gravitystorm#1644. This is a necessary step towards vector rendering (see gravitystorm#1644). This is a second trial of gravitystorm#3228, based on the word by @pnorman in gravitystorm#3233. This is a first step, once we have this than we can probably merge the point and poly layers (for example amenity-points and amenity-points-poly).
@pnorman Would you be able to review this? |
The comments from #1644 regarding label placement quality still apply. |
I’ve read through the comments on #1644, where @imagico concluded with:
“My main point here is that ultimately ST_PointOnSurface() is going to be a
dead end regarding quality label placement but Mapnik likely is as well so
it really does not matter.”
Is this still correct?
…On Sun, Jan 13, 2019 at 5:53 AM Christoph Hormann ***@***.***> wrote:
The comments from #1644
<#1644>
regarding label placement quality still apply.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3636 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AoxshImxNHZwoLMhzphlkRq-BNeejoF9ks5vCktBgaJpZM4Z81bN>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a fair amount of diff noise from unrelated whitespace fixes. Can you move those to their own PR?
The polygon layer/point layer distinction should be gone, e.g. stations-poly
should be removed and all stations part of the stations
layer.
I recommend splitting into smaller PRs for easier review. It will make it easier to match up MSS and MML changes to make sure that nothing was missed.
This PR was made and merged - can you rebase this PR to remove that noise? |
Can we close this since most of it is superseded by #3712 and the rest would probably be easier to implement with new PRs? |
Resolves #1644.
This is a necessary step towards vector rendering (see #1644).
This is a second trial of #3228, based on the word by @pnorman in #3233.
This is a first step, once we have this than we can probably merge the point and poly layers (for example amenity-points and amenity-points-poly).