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

MGCP translation should return error for relations tags that don't map to valid feature code #4971

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

brianhatchl
Copy link
Contributor

@brianhatchl brianhatchl commented Aug 23, 2021

To run the failing test illustrating the bug:

mocha test/multipolygon_relation.js

fixes #4970

@brianhatchl brianhatchl requested a review from mattjdnv August 24, 2021 13:43
@mattjdnv
Copy link
Contributor

mattjdnv commented Aug 26, 2021

I put comments in the ticket as well.

Findings so far:
NOTE: An "empty" multi polygon is one that doesn't have all of it's elements. A "full" one has all of the nodes and ways

  • Converting an "empty" multi polygon:
    • Without any tags streams the input to the output without calling the translation script. Therefore, no error tag
    • Converting to shapefile also doesn't call the translation script. It creates the shapefile directory and no files
  • Converting a "full" multi polygon:
    • Without any tags streams the input to the output without calling the translation script. Therefore, no error tag
    • With a "good" tag it translates correctly
    • With a "bad" tag it translates and sends back the error tag

Since the translator isn't run when there are no tags to translate - I.e. only having the "type" tag - I think we could modify the core to add an error tag if the translator isn't run on a feature. I don't know what this would impact.

I tried using an osm file with two multi polygons, one with a tag (building=yes) and one without. Converting them resulted in:

  • Exporting to OGR (shape etc) basically drops the feature that doesn't have any tags.
    • Two features in, One feature out
  • Converting to XML just passes the feature without tags through
    • Two features in, two features out.

Trying the same file but changing building=yes to be papa=smurf was interesting.

  • The shapefile had a feature in the O2S_A layer which is expected
  • The XML had no tags at all! It dropped the invalid tag even though the debug output said that it translated it to o2s_a

Getting curious, I created another OSM file that has:

  1. two ways with valid tags and a relation with valid tags joining them
  2. two ways, one with valid tags, one with invalid tags and a relation with valid tags joining them
  3. two ways without tags and a relation with valid tags joining them
  4. two ways without tags and a relation with invalid tags joining them
  5. two ways without tags and a relation without tags joining them
  6. two ways with valid tags and a relation with invalid tags joining them

Results:

  1. Both roads exported with the tags from the relation
  2. Both roads exported with the tags from the relation. The road with the invalid tags is also in the o2s_l layer
  3. Both roads exported with the tags from the relation
  4. Did not export - I expected it to go to o2s_l but with no tags the translator didn't run
  5. Did not export - as above
  6. Both roads exported with their tags.

Looking at this, it seems that tags on a relation stomp on tags on members.

@brianhatchl
Copy link
Contributor Author

So the upshot is the translation server clients need to send "full" relations to get the proper behavior?
I'm pretty sure I set the test up with exactly what's being sent currently ("empty").

As for what the proper behavior should be. I don't think we want type=multipolygon to appear in the translated tags. Wouldn't it be more accurate for it to get folded into OSMTAGS?

As for whether a relation with only type=multipolygon should generate and error. My thought was until a feature can be mapped to an FCODE that's what should happen. Maybe I could be convinced otherwise.

@brianhatchl
Copy link
Contributor Author

The other thing we could do (which is done for area=yes tag) is to not translate. There is UI code that checks tags and considers whether they are "interesting" or that their presence indicates the feature is "tagged". We could probably use this, as I would think a relation with only a type tag would be considered "untagged".

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.

Relations not returning error in MGCP translation when unable to translate
2 participants