Project-OSRM / osrm-backend

Open Source Routing Machine - C++ backend
http://map.project-osrm.org
BSD 2-Clause "Simplified" License
6.42k stars 3.4k forks source link

Json_render is not accurate for osm node ID #7016

Open fenwuyaoji opened 3 months ago

fenwuyaoji commented 3 months ago

Issue

@Rejudge-F, @DennisOSRM, @SiarheiFedartsou Hi, all. We are facing an inaccurate OSM node ID format issue and I found it's related to https://github.com/Project-OSRM/osrm-backend/pull/6531.

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

The reason I think is that PackedOSMIDshttps://github.com/Project-OSRM/osrm-backend/blob/203314b1aa5a4cbbd32b8bd47a5c68399bd9d04e/include/extractor/packed_osm_ids.hpp#L12 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 https://github.com/Project-OSRM/osrm-backend/pull/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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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

fenwuyaoji commented 3 months ago

Hi, I found that in the file https://github.com/Project-OSRM/osrm-backend/blob/203314b1aa5a4cbbd32b8bd47a5c68399bd9d04e/include/util/typedefs.hpp#L71, should be proposed in https://github.com/Project-OSRM/osrm-backend/pull/6020 , OSMNodeID is typed of uint64, while NodeID/EdgeID/NameID are uint32. As also mentioned in https://github.com/Project-OSRM/osrm-backend/pull/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: https://github.com/Project-OSRM/osrm-backend/pull/6341#issuecomment-1229547523 many guys are facing the same issue, e.g. https://github.com/Project-OSRM/osrm-backend/issues/6758, https://github.com/Project-OSRM/osrm-backend/issues/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 commented 3 months ago

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.