Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix Tagged Value Support in EdgeInfo #3262

Closed
kevinkreiser opened this issue Aug 11, 2021 · 3 comments · Fixed by #3264
Closed

Fix Tagged Value Support in EdgeInfo #3262

kevinkreiser opened this issue Aug 11, 2021 · 3 comments · Fixed by #3264
Assignees
Labels

Comments

@kevinkreiser
Copy link
Member

back in #2608 we added the ability to put arbitrary way tag and value pairs into the tileset. unfortunately, as discribed in this comment https://github.com/valhalla/valhalla/pull/2608/files#r687122174, we made a huge mistake. we used to_string to turn the enum value into a string for use in the tiles text list. this is very unfortunate, see the enum supports 256 values, which may be more than we will ever need, but we should have written that number into just one byte of the string, by converting the number to a character we limit the total number of these supported extra tags to just 9 (a single digit excluding 0 for the moment). thats a huge bummer but i do think we can patch our way back to glory...

what i think we can do is:

  1. renumber tunnel and bride to be the integer equivalent of the characters 1 and 2, this lets old code continue to parse them as a character
  2. change the parsing code in edgeinfo to actually not convert them from char to int, but just cast them to it directly, new code will also parse them fine this way

after that i think we can get back the full usage of all the possible values for tags that we want to support. whew crisis averted...

@SiarheiFedartsou
Copy link
Member

SiarheiFedartsou commented Aug 12, 2021

It seems I cannot finish #3261 without fixing this issue. @kevinkreiser what do you think if I try to fix it in that PR? I think we can go this way:

renumber tunnel and bride to be the integer equivalent of the characters 1 and 2, this lets old code continue to parse them as a character

@kevinkreiser
Copy link
Member Author

Can you do that in another pr, I wanted to last night but didn't find the time. If you can wait a few hours I can do then as well. The important thing is testing old code with new data and new code with old data to verify the change

@SiarheiFedartsou
Copy link
Member

SiarheiFedartsou commented Aug 12, 2021

Waiting for CI :) #3264

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants