Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

New text placement algorithm gives an error #494

Closed
kocio-pl opened this issue Aug 9, 2018 · 19 comments
Closed

New text placement algorithm gives an error #494

kocio-pl opened this issue Aug 9, 2018 · 19 comments

Comments

@kocio-pl
Copy link

kocio-pl commented Aug 9, 2018

Mapnik 3.0.19+ adds new new text placement algorithm - grid:

https://github.com/mapnik/mapnik/blob/v3.0.19/CHANGELOG.md#3019

CartoCSS gives an error when it is used and does not produce XML configuration file. I checked it with 0.18.2 and 1.0.1, here's a 0.18.2 error output, since it gives more debugging details:

Error: placenames.mss:56:4 Invalid value for text-placement, the type keyword (options: point, line, vertex, interior) is expected. grid (of type keyword) was given.
    at Object.env.error (/media/hdd/home/kocio/devel/kosmtik/node_modules/carto/lib/carto/parser.js:249:55)
    at Rule.tree.Rule.toXML (/media/hdd/home/kocio/devel/kosmtik/node_modules/carto/lib/carto/tree/rule.js:86:28)
    at Definition.tree.Definition.symbolizersToXML (/media/hdd/home/kocio/devel/kosmtik/node_modules/carto/lib/carto/tree/definition.js:145:39)
    at Definition.tree.Definition.toXML (/media/hdd/home/kocio/devel/kosmtik/node_modules/carto/lib/carto/tree/definition.js:218:33)
    at /media/hdd/home/kocio/devel/kosmtik/node_modules/carto/lib/carto/tree/style.js:40:27
    at Array.map (<anonymous>)
    at Object.tree.StyleXML (/media/hdd/home/kocio/devel/kosmtik/node_modules/carto/lib/carto/tree/style.js:39:29)
    at Renderer.render (/media/hdd/home/kocio/devel/kosmtik/node_modules/carto/lib/carto/renderer.js:180:43)
    at compile (/media/hdd/home/kocio/devel/kosmtik/node_modules/carto/bin/carto:100:31)
    at MML.load (/media/hdd/home/kocio/devel/kosmtik/node_modules/carto/lib/carto/mml.js:68:20)

See also gravitystorm/openstreetmap-carto#1391 (comment).

@kocio-pl
Copy link
Author

kocio-pl commented Aug 9, 2018

It looks like adding the value is needed in proper .rst file, but CartoCSS lacks API reference 3.0.20:

https://github.com/mapbox/carto/tree/master/docs/api/mapnik

and even this version lacks grid as acceptable value for text-placement:

http://mapnik.org/mapnik-reference/#3.0.20/

@talaj Could you update Mapnik API documentation to include it? And is there a simple way to compare the difference for example between 3.0.6 and 3.0.20 to produce new .rst file?

@talaj
Copy link
Contributor

talaj commented Aug 10, 2018

@kocio-pl I've updated the documentation in mapnik/mapnik-reference#149.

And is there a simple way to compare the difference for example between 3.0.6 and 3.0.20 to produce new .rst file?

I put that .rst file to https://gist.github.com/talaj/bcf03fe9a3912ce42ab246a769e443b8.

I've generated it by pointing to the latest mapnik-reference:

diff --git a/package.json b/package.json
index 06bf606..a641771 100644
--- a/package.json
+++ b/package.json
@@ -37,7 +37,7 @@
     "hsluv": "~0.0.1",
     "js-yaml": "~3.10.0",
     "lodash": "~4.17.2",
-    "mapnik-reference": "~8.8.1",
+    "mapnik-reference": "git+https://github.com/mapnik/mapnik-reference.git",
     "semver": "~5.5.0",
     "yargs": "~11.0.0"
   },

Then I made:

$ npm install 
$ node docs/generate.js

@nebulon42
Copy link
Collaborator

@talaj mapnik-reference needs a new release. An up to date documentation is not enough for carto since it also has to use an updated mapnik-reference. For stability reasons I will not switch to master of mapnik-reference. Once there is a new release of mapnik-reference carto can be updated.

@kocio-pl I just said that carto didn't need any update in its code. But it still needs an up-to-date mapnik-reference. For that an 1.1.0 release of carto is necessary.

@talaj
Copy link
Contributor

talaj commented Aug 11, 2018

@nebulon42 OK, I can do it if you don't mind. I would also like to rebase and merge those two pull requests there.

@kocio-pl
Copy link
Author

Great! I just wanted to be sure that there won't be some unforseen problem that wouldn't allow to use grid in OSM Carto and no one will touch CartoCSS. This one is unforseen indeed, but if you're going to fix it, it's not really important for me if it's in the code or in documentation.

@nebulon42
Copy link
Collaborator

@talaj I have created releases of mapnik-reference in the past, but I'm no longer able to do it since I have removed myself from the organization when I stepped down from maintaining openstreetmap-carto and Kosmtik (and I actually also want to step down from maintaining CartoCSS). So I don't mind, no I highly appreciate it if you do it. :) If you cannot publish on NPM because of lacking rights I still can do that because I still have the rights for it.

@talaj
Copy link
Contributor

talaj commented Aug 12, 2018

@nebulon42 I've cut a release v8.9.0 of mapnik-reference, but don't have rights to publish the Node package. Can you please do that or can you give me rights to do that with simple description how to proceed?

@nebulon42
Copy link
Collaborator

Thanks! Looks like I can grant you rights on NPM for mapnik-reference, but you would need to create a user at https://www.npmjs.com/ and tell me its name (also per mail if you like, mail addr is in my profile). There is https://github.com/mapnik/mapnik-reference/blob/gh-pages/contributing.md which has the steps in it. Basically it is just npm publish once you are logged in at the registry.

@kocio-pl
Copy link
Author

kocio-pl commented Aug 12, 2018

What do you think about granting permissions to my account (kocio-pl) too for respective npm and GitHub repos? My work at OSM Carto relies on these tools and packages and CartoCSS doesn't seem to be actively maintained by a healthy team any more.

@nebulon42
Copy link
Collaborator

This was not about carto but about mapnik-reference. I'm happy to accept new maintainers here, but they would need to do some work in the form of PRs/patches first as it is the case in almost all open source projects. You might still get some rights granted if you contact Mapbox directly.

@kocio-pl
Copy link
Author

There's a long chain of interdependencies, including code, but also releasing, packaging, releasing packages, deployment rules (see gravitystorm/openstreetmap-carto#2962 (comment))... I took it for granted, until fixing problem with label placement took almost a year because of these inter-project dependencies (and the code itself was fixed pretty soon!). Now I'm aware that some parts (like CartoCSS) are not maintained in practice, and there's another unexpected repo/package in this chain, I'm beginning to really worry.

In case of non-maintained things (in practice) I'd like to play a role of accidental (backup) release manager, to avoid problems with multiple points of failure in the chain. It's hard for me to imagine typical patch/PR work that can be done - this is what works when the project is still alive. I don't want to be a coder and add functionalities myself, just to keep the chain alive and updated. But if you find some task that I could prove that I won't screw the project, I'm ready to learn and do it.

It all might sound like a hollow fear, but my experience says that it's good to avoid problems as soon as they become probable, like 1 person active on the deck. This is for example how I became forum admin after one person keeping it became inactive without try to pass it along. It took months for OMSF to get in touch with him and migrate the forum to a community curated space - and it was not even hard "hit by bus" problem, because he was reachable eventually... Now we have 3 persons team there and it's safe for me.

This is kind of problem nobody talks about. People are mostly voluntary working on a single project or two, but OSM ecosystem is sometimes big and connected in multiple ways. The problem in one point can hurt another one in strange ways. Until there are multiple persons, it is less of an issue, but at some point nobody even knows how the real pipeline looks like, let alone has all the tools to fix if something is broken.

I'm aware this is not easy thing. That's why I ask you what do you think would be safe to do to avoid such problems. Granting permissions is just how I think it could be done, but maybe there's some other way for community to not fall into trap of single project problems influencing the rest of ecosystem. You're not active, but I can still talk with you, which is kind of red light flashing situation for me - small failure warns us about possible future problems.

@nebulon42
Copy link
Collaborator

I think I made it clear: If you want to become maintainer start submitting patches. Otherwise please stay on topic.

@kocio-pl
Copy link
Author

Do you think it's better to open another ticket to talk about such project issues? I have some questions about it and I'm not sure where to go with them.

@nebulon42
Copy link
Collaborator

Yes, please. Questions about project governance should go into another ticket.

@talaj
Copy link
Contributor

talaj commented Aug 13, 2018

I have published mapnik-reference v8.9.1 finally. I had to make a new minor version bump as I did not correctly update datasource templates previously. Carto tests are passing against this version.

@nebulon42
Copy link
Collaborator

I have updated mapnik-reference, generated API documentation and released v1.1.0.

@kocio-pl
Copy link
Author

Thanks for both of you!

@kocio-pl
Copy link
Author

BTW - there's another Mapnik documentation update (mapnik/mapnik-reference#151), which might need another packages update to avoid errors with text-offset and shield-offset options.

@talaj
Copy link
Contributor

talaj commented Aug 17, 2018

@kocio-pl I just released mapnik-reference 8.9.2 and published it on npmjs.com.

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

No branches or pull requests

3 participants