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.

Sign up for GitHub

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

Jump to bottom

Json_render is not accurate for osm node ID #7016

Open
fenwuyaoji opened this issue Aug 16, 2024 · 8 comments · May be fixed by #7096
Open

Json_render is not accurate for osm node ID #7016

fenwuyaoji opened this issue Aug 16, 2024 · 8 comments · May be fixed by #7096
Labels
Bug Report

Comments

@fenwuyaoji
Copy link
Contributor

fenwuyaoji commented Aug 16, 2024 •
edited
Loading

Issue

@Rejudge-F, @DennisOSRM, @SiarheiFedartsou Hi, all. We are facing an inaccurate OSM node ID format issue and I found it's related to #6531.

The case is:
original node ID: 11117421192, after format: 1.111742119e+10.

The reason I think is that PackedOSMIDs

osrm-backend/include/extractor/packed_osm_ids.hpp

Line 12 in 203314b

using PackedOSMIDs = util::detail::PackedVector<OSMNodeID, 34, Ownership>;
is 34 bits and the max length of it is 11. Why do we limit the number length to less than 10? I noticed that before #6531, we only limited the decimal to less than 10, but now we limit the whole number. should we rollback to the previous version or just relax the length to 11?

Steps to reproduce

Add below test case to unit_tests/util/json_render.cpp.

    output.clear();
    renderer(11117421192);
    BOOST_CHECK_EQUAL(output, "1.111742119e+10");
@Rejudge-F
Copy link
Contributor

If a rollback is performed, it will cause more serious problems, resulting in incorrect map match results. In addition, I found that the ID type used by OSRM elsewhere is int32, with a maximum of 10 digits. If only this place is modified, there seem to be problems elsewhere.
If you modify the type used by osrm, it would be a relatively large project, so I recommend that you could shrink the ID within the int32 range.

@fenwuyaoji
Copy link
Contributor Author

fenwuyaoji commented Aug 16, 2024 •
edited
Loading

thanks for your quick response. I see your point. how about just relax it to 11, like .11g? I think this part is only used for json format, right? then nothing will be affected I think.

@Rejudge-F
Copy link
Contributor

@fenwuyaoji I think you can fork this repository and modify it, but I don’t recommend doing so. Many parts of the code use the INT32 type for OSM IDs. Therefore, I suggest that you keep your IDs within the INT32 range and then use OSRM to process the data.
By the way, whether it’s 10-digit or 11-digit IDs, neither seems to be an ideal solution. If you can find a more universal way to solve this problem, that would be the best.

@fenwuyaoji
Copy link
Contributor Author

okay, I will look through the code and see whether I can do anything better to solve it.

@fenwuyaoji fenwuyaoji mentioned this issue Aug 19, 2024
Open
@fenwuyaoji
Copy link
Contributor Author

fenwuyaoji commented Aug 19, 2024 •
edited
Loading

Hi, I found that in the file

osrm-backend/include/util/typedefs.hpp

Line 71 in 203314b

using OSMNodeID = osrm::Alias<std::uint64_t, tag::osm_node_id>;
, should be proposed in #6020 , OSMNodeID is typed of uint64, while NodeID/EdgeID/NameID are uint32. As also mentioned in #6341, I think the internal osm ID should be max to 34-bit. what you said 'Many parts of the code use the INT32 type for OSM IDs.' I think it's NodeID, not OSMNodeID.
please see this summary: #6341 (comment)
many guys are facing the same issue, e.g. #6758, #6594. I think it's worth to relax it to 11 in json_render.

@mjjbell HI, do you have any idea on this issue?

@Rejudge-F
Copy link
Contributor

I thank you are right, it seems reasonable to expand to 11 digits and you can submit a PR to see the administrator's opinion.

@imannamdari imannamdari linked a pull request Jan 12, 2025 that will close this issue
Open
6 tasks
@imannamdari
Copy link

Hi @fenwuyaoji. We have the same issue. It converts 10003966158 node_id to 10003966160 as well. I opened a pr for it:
#7096
I think the approach implemented in this PR is better than expand to 11 digits.

@SiarheiFedartsou Could you please review and merge this pull request?

@afarber
Copy link
Contributor

afarber commented Jan 20, 2025

I see this issue with v5.28.0-unreleased too, with map matching (locations in Berlin, Germany):

wget -O- 'http://127.0.0.1:5000/match/v1/driving/13.416270,52.526382;13.416849,52.526115;13.416849,52.525750;13.41653
8,52.525430;13.416184,52.525123;13.415755,52.525306;13.415154,52.525567;13.414596,52.525802?overview=false&generate_hints=fal
se&gaps=ignore&annotations=nodes' | jq

Returns the response (please ignore the changed "name" property, where I injects tags):

{
  "code": "Ok",
  "matchings": [
    {
      "confidence": 0.857914283,
      "legs": [
        {
          "steps": [],
          "weight": 4.5,
          "summary": "",
          "duration": 4.5,
          "distance": 49.1
        },
        {
          "steps": [],
          "weight": 12.5,
          "summary": "",
          "duration": 12.5,
          "distance": 65.4
        },
        {
          "steps": [],
          "weight": 6.2,
          "summary": "",
          "duration": 6.2,
          "distance": 41.2
        },
        {
          "steps": [],
          "weight": 5.4,
          "summary": "",
          "duration": 5.4,
          "distance": 35.6
        },
        {
          "steps": [],
          "weight": 6.8,
          "summary": "",
          "duration": 6.8,
          "distance": 31.5
        },
        {
          "steps": [],
          "weight": 7.6,
          "summary": "",
          "duration": 7.6,
          "distance": 50
        },
        {
          "steps": [],
          "weight": 6.9,
          "summary": "",
          "duration": 6.9,
          "distance": 46
        }
      ],
      "weight_name": "routability",
      "weight": 49.9,
      "duration": 49.9,
      "distance": 318.8
    }
  ],
  "tracepoints": [
    {
      "waypoint_index": 0,
      "matchings_index": 0,
      "name": "{\"id\":4790813,\"highway\":\"primary\",\"surface\":\"asphalt\"}",
      "alternatives_count": 0,
      "location": [
        13.416327,
        52.52643
      ],
      "distance": 6.595175589
    },
    {
      "waypoint_index": 1,
      "matchings_index": 0,
      "name": "{\"id\":4790813,\"highway\":\"primary\",\"surface\":\"asphalt\"}",
      "alternatives_count": 0,
      "location": [
        13.416916,
        52.526174
      ],
      "distance": 7.986416714
    },
    {
      "waypoint_index": 2,
      "matchings_index": 0,
      "name": "{\"id\":276701303,\"highway\":\"residential\",\"surface\":\"asphalt\"}",
      "alternatives_count": 0,
      "location": [
        13.416843,
        52.525752
      ],
      "distance": 0.4640765274
    },
    {
      "waypoint_index": 3,
      "matchings_index": 0,
      "name": "{\"id\":276701303,\"highway\":\"residential\",\"surface\":\"asphalt\"}",
      "alternatives_count": 0,
      "location": [
        13.416492,
        52.52545
      ],
      "distance": 3.834130228
    },
    {
      "waypoint_index": 4,
      "matchings_index": 0,
      "name": "{\"id\":276701303,\"highway\":\"residential\",\"surface\":\"asphalt\"}",
      "alternatives_count": 0,
      "location": [
        13.416188,
        52.525189
      ],
      "distance": 7.349318314
    },
    {
      "waypoint_index": 5,
      "matchings_index": 0,
      "name": "{\"id\":173197451,\"highway\":\"residential\",\"surface\":\"asphalt\"}",
      "alternatives_count": 0,
      "location": [
        13.415811,
        52.525354
      ],
      "distance": 6.555592941
    },
    {
      "waypoint_index": 6,
      "matchings_index": 0,
      "name": "{\"id\":173197451,\"highway\":\"residential\",\"surface\":\"asphalt\"}",
      "alternatives_count": 1,
      "location": [
        13.415211,
        52.525615
      ],
      "distance": 6.595175589
    },
    {
      "waypoint_index": 7,
      "matchings_index": 0,
      "name": "{\"id\":173197451,\"highway\":\"residential\",\"surface\":\"asphalt\"}",
      "alternatives_count": 3,
      "location": [
        13.414661,
        52.525856
      ],
      "distance": 7.454564167
    }
  ]
}
Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Report
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix json rendering of large osm ids imannamdari/osrm-backend
4 participants
@afarber @imannamdari @fenwuyaoji @Rejudge-F