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

Add fields to Taginfo file #4940

Merged
merged 10 commits into from
Mar 30, 2018
Merged

Add fields to Taginfo file #4940

merged 10 commits into from
Mar 30, 2018

Conversation

mmd-osm
Copy link
Contributor

@mmd-osm mmd-osm commented Mar 25, 2018

Gave it a first try.. not quite sure if I covered all combinations in the original json files. I should probably remove those duplicates in taginfo as well.

Things like "fields/gender.json" still look pretty strange in the taginfo result. There seem to be some cases, where we don't want a cross product of those string.options values and the keys array.

Supposed to eventually

@bhousel
Copy link
Member

bhousel commented Mar 26, 2018

Gave it a first try.. not quite sure if I covered all combinations in the original json files. I should probably remove those duplicates in taginfo as well.

Things like "fields/gender.json" still look pretty strange in the taginfo result. There seem to be some cases, where we don't want a cross product of those string.options values and the keys array.

Thanks @mmd-osm - I took a look and this seems right to me. I think it's pretty good.

What stands out to me is that we do have a lot of duplicates for things like:

        {
            "key": "ref"
        },
        {
            "key": "ref"
        },
        {
            "key": "ref"
        },
        ...

So maybe we should start adding "description": "" to each of these (for both presets and fields).. The description could say which presets and fields use the tag, and we could coalesce duplicate key/value pairs together like:

        {
            "key": "ref",
            "description": "Gate Number, Hole Number, Highway Number, Runway Number..."
        },

@mmd-osm
Copy link
Contributor Author

mmd-osm commented Mar 26, 2018

I think we're slowly getting there. I added descriptions for presets and fields now (where available) and also coalesced them like in the example above:

        {
            "key": "ref",
            "description": "Gate Number, Hole Number, Junction Number, Platform Number, Road Number, Route Number, Runway Number, Stop Number, Taxiway Name, Reference Code"
        },

@bhousel
Copy link
Member

bhousel commented Mar 26, 2018

Hey that's pretty good @mmd-osm !

What else is left?
Are you thinking to add support for "object_types": ["node", "way", "area", "relation"] ?

@mmd-osm
Copy link
Contributor Author

mmd-osm commented Mar 26, 2018

Are you thinking to add support for "object_types

I still have to figure out how the iD <--> taginfo mapping looks like.
point, line and relation seem quite obvious, for the others it's not quite clear yet.

          const mapping = { "point"    : "node",
                            "vertex"   : "node",
                            "line"     : "way",
                            "relation" : "relation",
                            "area"     : "area" };

Then there's some strange side effect issue when adding geometry to generateField. It ends up in data/presets.yaml as well.

        if (field.geometry) {
          t.geometry = field.geometry;
        }

Icons could be interesting as well. Can I assume that maki is used all over the place, which would be quite straightforward:

        if (preset.icon) {
          tag.icon_url = 'https://raw.githubusercontent.com/mapbox/maki/master/icons/' + preset.icon + '-11.svg';
        }

@bhousel
Copy link
Member

bhousel commented Mar 26, 2018

const mapping = { "point" : "node",
"vertex" : "node",
"line" : "way",
"relation" : "relation",
"area" : "area" };

Yes, looks good ..

Then there's some strange side effect issue when adding geometry to generateField. It ends up in data/presets.yaml as well.

Yeah it might not be straightforward which the fields can be used on which geometries. You could try to build an index of field -> geometry when you build the preset list (presets have geometry).

Icons could be interesting as well. Can I assume that maki is used all over the place, which would be quite straightforward:

Maki isn't quite used everywhere, but you can use code like this

iD/modules/svg/labels.js

Lines 221 to 222 in 523fb7f

var isMaki = dataFeatureIcons.indexOf(picon) !== -1;
return '#' + picon + (isMaki ? '-15' : '');

Because maki is a dependency, you can just require it and get this array of all the icon names, then use that to build an isMaki() function:

> const maki = require ('@mapbox/maki');
undefined
> var dataFeatureIcons = maki.layouts.all.all
[ 'aerialway',
  'airfield',
  'airport',
...

hope this helps!

mmd-osm added 3 commits March 27, 2018 19:41
+ Changed description duplicate handling
+ Removed geometry and icon from translation
@mmd-osm
Copy link
Contributor Author

mmd-osm commented Mar 27, 2018

Thanks again for all the hints.

I have to admit I was bit lazy to set up a separate field -> geometry index, and decided to remove geometry and icon instead, right towards the end of generateTranslations . As it is a bit of a hack, I added a small comment there. Hope that's ok. -> no longer needed, just don't store icon and geometry in translatable array t and all is good.

@bhousel bhousel merged commit 1da6d90 into openstreetmap:master Mar 30, 2018
@bhousel
Copy link
Member

bhousel commented Mar 30, 2018

Thanks again for all the hints.

Thank you, for working on it! Looks great, I just merged it 👍

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

Successfully merging this pull request may close these issues.

Add fields to Taginfo file as well Taginfo project file should include icons, names
2 participants