Hi, I think we should clean the code regarding the usage of highwayCount, treatAsNode, onBoundary and other attributes of Coord. My understanding is this: 1) highwayCount should be increased for each coord instance that is part of a road. I think the right place to do that is in an extra loop at the beginning of StyledConverter.removeShortArcsByMergingNodes(). A coord instance that is part of more than one road is considered to become a road junction. 2) in many places I see something like this: if(co.getOnBoundary()) { // this point lies on a boundary // make sure it becomes a node co.incHighwayCount(); } I think this is an abuse of the highwayCount. The highwayCount is used to determine junctions of roads (e.g. in the frigRoundabout() method) and I don't see why the attribute "onBoundary" should change the "highwayCount" If "onBoundary = true" and "highwayCount > 1" both have a meaning like "isRoadJunction" I suggest that we create a new flag with this name. Does that make sense? If yes, how can I verify the results of my changes ? With my logic, I see ~20% fewer coords with highwayCount > 1^, so the change is big. The unit tests do not help much, SimpleRouteTest failes because the NOD size is different, but I doubt that the expected size is a must? ciao, Gerd -- View this message in context: http://gis.19327.n5.nabble.com/highwayCount-tp5748434.html Sent from the Mkgmap Development mailing list archive at Nabble.com.
Hi Gerd Some background: The term "highwayCount" is inaccurate and the count is unimportant the only important concept is if a point on a line is a "node" or not. The term "node" is used in lots of different contexts of course, so what I mean in this context is: 1. a point that is used in more than one line is a "node" 2. a boundary node is a "node" - it is joined to a line in another tile. 3. house numbers are tied to "node"s so you can have a "node" in the middle of a line, just to support a house number range. I used to say "routing node", but case (3) which I just discovered recently kind of makes that name incorrect too.
My understanding is this: 1) highwayCount should be increased for each coord instance that is part of a road.
Not necessarily, it is just an implementation, a one bit counter would be enough.
I think the right place to do that is in an extra loop at the beginning of StyledConverter.removeShortArcsByMergingNodes(). A coord instance that is part of more than one road is considered to become a road junction.
Its not just actual road junctions, two lines that join end to end as part of a single road in real life are still connected by a "node".
2) in many places I see something like this: if(co.getOnBoundary()) { // this point lies on a boundary // make sure it becomes a node co.incHighwayCount(); } I think this is an abuse of the highwayCount. The highwayCount is used to determine junctions of roads (e.g. in the frigRoundabout() method) and I don't see why the attribute "onBoundary" should change the "highwayCount"
Agreed that the name is not appropriate and is confusing in this case. But the functionality is correct, a boundary node should be a "node".
If "onBoundary = true" and "highwayCount > 1" both have a meaning like "isRoadJunction"
isRoadJunction() is too narrow a name, all road junctions are nodes but not all nodes are road junction. But perhaps isNode()
I suggest that we create a new flag with this name.
Does that make sense?
I'm fine with changing to better names + better comments so it doesn't cause confusion. Not so sure about algorithm changes that produce different results until you have identified a specific bug.
If yes, how can I verify the results of my changes ? With my logic, I see ~20% fewer coords with highwayCount > 1^, so the change is big.
The unit tests do not help much, SimpleRouteTest failes because the NOD size is different, but I doubt that the expected size is a must?
Correct, the size checks are just there to prevent accidental breakage, if you are fixing a bug that is expected to change the size then the test has to be changed too after very careful checking that the results are correct. We don't have many good ways of checking results. The ultimate check is of course is testing how well routing works on an actual device (or range of devices, since they are all a bit different). All there is at the moment is NetDisplay and NodDisplay in the display repo. We need better tools to display things are a higher level and automatically cross check between the different sections. ..Steve
Hi Steve,
The term "highwayCount" is inaccurate and the count is unimportant the only important concept is if a point on a line is a "node" or not.
The term "node" is used in lots of different contexts of course, so what I mean in this context is:
1. a point that is used in more than one line is a "node"
Ah, so you think that it doesn't matter whether the line is a road or not?
2. a boundary node is a "node" - it is joined to a line in another tile. Again: All lines or just roads?
3. house numbers are tied to "node"s so you can have a "node" in the middle of a line, just to support a house number range.
I used to say "routing node", but case (3) which I just discovered recently kind of makes that name incorrect too. Ok, I think this is not yet handled when reading OSM data.
My understanding is this: 1) highwayCount should be increased for each coord instance that is part of a road.
Not necessarily, it is just an implementation, a one bit counter would be enough.
I think the right place to do that is in an extra loop at the beginning of StyledConverter.removeShortArcsByMergingNodes(). A coord instance that is part of more than one road is considered to become a road junction.
Its not just actual road junctions, two lines that join end to end as part of a single road in real life are still connected by a "node".
okay, understood.
2) in many places I see something like this: if(co.getOnBoundary()) { // this point lies on a boundary // make sure it becomes a node co.incHighwayCount(); } I think this is an abuse of the highwayCount. The highwayCount is used to determine junctions of roads (e.g. in the frigRoundabout() method) and I don't see why the attribute "onBoundary" should change the "highwayCount"
Agreed that the name is not appropriate and is confusing in this case. But the functionality is correct, a boundary node should be a "node".
If "onBoundary = true" and "highwayCount > 1" both have a meaning like "isRoadJunction"
isRoadJunction() is too narrow a name, all road junctions are nodes but not all nodes are road junction. But perhaps isNode()
Well, Node.isNode() looks not much better. May isGarminNode() ?
I suggest that we create a new flag with this name.
Does that make sense?
I'm fine with changing to better names + better comments so it doesn't cause confusion. Not so sure about algorithm changes that produce different results until you have identified a specific bug.
I am not sure if it is a bug. Many roads share the OSM nodes with e.g. shapes or railway lines or something else. With the current implementation these OSM nodes have highwayCount > 1 and therefor a CoordNode is created for them. If this is intended then we don't have to change something, else we could create smaller img files (e.g. 11.9 MB instead of 12.4 MB)
If yes, how can I verify the results of my changes ? With my logic, I see ~20% fewer coords with highwayCount > 1^, so the change is big.
The unit tests do not help much, SimpleRouteTest failes because the NOD size is different, but I doubt that the expected size is a must?
Correct, the size checks are just there to prevent accidental breakage, if you are fixing a bug that is expected to change the size then the test has to be changed too after very careful checking that the results are correct.
OK
We don't have many good ways of checking results. The ultimate check is of course is testing how well routing works on an actual device (or range of devices, since they are all a bit different).
OK, up to now I did only check in BaseCamp, and I did probably not check special cases like boundary nodes, CoordPOIs and so on.
All there is at the moment is NetDisplay and NodDisplay in the display repo. We need better tools to display things are a higher level and automatically cross check between the different sections.
Yes, that would be great, but I still don't know enough to help here. Ciao, Gerd
Hi Gerd,
1. a point that is used in more than one line is a "node" Ah, so you think that it doesn't matter whether the line is a road or not?
Oh, no sorry, I did not mean that. A road can be made of several lines, but I am only talking about lines that are part of a road.
isRoadJunction() is too narrow a name, all road junctions are nodes but not all nodes are road junction. But perhaps isNode() Well, Node.isNode() looks not much better. May isGarminNode() ?
isConnection() ? doesn't quite fit with house numbers, but that is rare and we don't even use it yet.
produce different results until you have identified a specific bug. I am not sure if it is a bug. Many roads share the OSM nodes with e.g. shapes or railway lines or something else. With the current implementation these OSM nodes have highwayCount > 1 and therefor a CoordNode is
Ah OK, non-routable lines should not be used when determining if something is a CoordNode or not. So sounds like a bug. ..Steve
Hi Steve,
Oh, no sorry, I did not mean that. A road can be made of several lines, but I am only talking about lines that are part of a road.
OK
isConnection() ? doesn't quite fit with house numbers, but that is rare and we don't even use it yet.
fine with me. Maybe I will not need that new flag, but if I do isConnection() is good.
produce different results until you have identified a specific bug. I am not sure if it is a bug. Many roads share the OSM nodes with e.g. shapes or railway lines or something else. With the current implementation these OSM nodes have highwayCount > 1 and therefor a CoordNode is
Ah OK, non-routable lines should not be used when determining if something is a CoordNode or not. So sounds like a bug.
OK. That's what I thought. Gerd
participants (3)
-
Gerd Petermann -
GerdP
-
Steve Ratcliffe