Hi, I think the method is incorrect. It should not use Coord.equal() to check if first and last point is equal, instead it should test if the points are identical. Sample where the current implementation fails: http://www.openstreetmap.org/way/59994654 The way has a closed loop, but first and last point are not identical because the way starts with a short segment that is not part of the loop. Gerd
because the way starts with a short segment that is not part of the loop.
And so the way is NOT closed in OSM-sense.
Chris
Yep, Way.isClosed() is meant to detect if the way might be a polygon. And that's the case if the first and the last point are equal.
WanMil
Argh, read once more your original post. I think you are right that the first and the last point should be identical. Coord.equals(otherCoord) checks only if the two coords have the same (garmin) coordinates. Before fixing that all calls to this method need to be checked if they rely on the current behaviour. WanMil
Hi WanMil,
Before fixing that all calls to this method need to be checked if they rely on the current behaviour.
I agree. The problem is that the method is used in rather complex algos, so it's difficult for me to answer this question, esp. when we also have to consider the changes in the high prec branch. Gerd
Hi Gerd, I've gone through all calls of isClosed(). I am quite sure all in all calls it should be checked for identity. But there are some other changes that need to be done (not complete!!): SeaGenerator beginMap (line 770) need to be IdentityHashMap BoundaryRelation line 387 should be an IdentityHashMap? line 474+477: Coord.equals(..) maybe check for identity? MultipolgyonRelation line 468 should be an IdentityHashMap? line 553+556: Coord.equals(..) maybe check for identity? I guess there are a lot of other places where Coord.equals is used instead of identity. WanMil
Hi WanMil,
Before fixing that all calls to this method need to be checked if they rely on the current behaviour.
I agree. The problem is that the method is used in rather complex algos, so it's difficult for me to answer this question, esp. when we also have to consider the changes in the high prec branch.
Gerd
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi WanMil, I agree that also Coord.equals() is a method which has to be handled with great care. The meaning of "p1.equals(p2) == true" is that both points have coordinates that were rounded to equal map unit values. The original OSM points can have a distance of > 2m, on the other hand, two points with a distance < 0.2m can have different map unit coordinates. I know one case where the Coord.equals() has a meaning: When creating the road network, we must make sure that a RouteArc connects two points that have different coordinates. (high prec branch r2861 still contains an error here) I think equals() should be avoided in all algos that try to calculate something. problem is that we use Coord.equals() whenever we use collections like Map<Coord,xyz> or even List<Coord> in combinations with List.contains(). Most of these usages are candidates for errors when input data allows that p1.equals(p2) == true and p1 != p2. The higher precision values in the high prec branch are no solution for that, they just make it less likely to happen. Maybe a good way to find potential errors in the code is to create test data that contains these special cases ? Gerd
Date: Thu, 5 Dec 2013 21:59:42 +0100 From: wmgcnfg@web.de To: mkgmap-dev@lists.mkgmap.org.uk Subject: Re: [mkgmap-dev] Way.isClosed()
Hi Gerd,
I've gone through all calls of isClosed(). I am quite sure all in all calls it should be checked for identity.
But there are some other changes that need to be done (not complete!!): SeaGenerator beginMap (line 770) need to be IdentityHashMap
BoundaryRelation line 387 should be an IdentityHashMap? line 474+477: Coord.equals(..) maybe check for identity?
MultipolgyonRelation line 468 should be an IdentityHashMap? line 553+556: Coord.equals(..) maybe check for identity?
I guess there are a lot of other places where Coord.equals is used instead of identity.
WanMil
Hi WanMil,
Before fixing that all calls to this method need to be checked if they rely on the current behaviour.
I agree. The problem is that the method is used in rather complex algos, so it's difficult for me to answer this question, esp. when we also have to consider the changes in the high prec branch.
Gerd
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
aargh... GerdP wrote
I know one case where the Coord.equals() has a meaning: When creating the road network, we must make sure that a RouteArc connects two points that have different coordinates.
I am probably wrong here. It is not a problem as long as the two points have different "node ids" so that different RouteNode instances are created. StyledConverter takes care about this. I just wonder if we can use this information to avoid the splitting of roads. If I got this right, we don't have to split the road if it has identical points (like in roundabouts), instead it should be sufficient to replace the point with a new Coord instance. I'll try that. Gerd Gerd -- View this message in context: http://gis.19327.n5.nabble.com/Way-isClosed-tp5788396p5788819.html Sent from the Mkgmap Development mailing list archive at Nabble.com.
I just wonder if we can use this information to avoid the splitting of roads. If I got this right, we don't have to split the road if it has identical points (like in roundabouts), instead it should be sufficient to replace the point with a new Coord instance. I'll try that. no, (of course) that doesn't work, as it will prohibit to route across that point in the roundabout.
But splitting can be improved when we try to split at points that are already connected to other roads. This creates fewer routing nodes. Gerd
Hi WanMil, my work on the algo to fix wrong angles in roads is done for now, so I will now try to fix the problems related to the use of Coords.equals(). I've already found two errors in Java2DConverter which can cause areas to disappear when the closing point is equal to another one. Gerd GerdP wrote
Hi WanMil,
I agree that also Coord.equals() is a method which has to be handled with great care. The meaning of "p1.equals(p2) == true" is that both points have coordinates that were rounded to equal map unit values. The original OSM points can have a distance of > 2m, on the other hand, two points with a distance < 0.2m can have different map unit coordinates.
I know one case where the Coord.equals() has a meaning: When creating the road network, we must make sure that a RouteArc connects two points that have different coordinates. (high prec branch r2861 still contains an error here)
I think equals() should be avoided in all algos that try to calculate something. problem is that we use Coord.equals() whenever we use collections like Map<Coord,xyz> or even List <Coord> in combinations with List.contains(). Most of these usages are candidates for errors when input data allows that p1.equals(p2) == true and p1 != p2.
The higher precision values in the high prec branch are no solution for that, they just make it less likely to happen.
Maybe a good way to find potential errors in the code is to create test data that contains these special cases ?
Gerd
Date: Thu, 5 Dec 2013 21:59:42 +0100 From:
wmgcnfg@
To:
mkgmap-dev@.org
Subject: Re: [mkgmap-dev] Way.isClosed()
Hi Gerd,
I've gone through all calls of isClosed(). I am quite sure all in all calls it should be checked for identity.
But there are some other changes that need to be done (not complete!!): SeaGenerator beginMap (line 770) need to be IdentityHashMap
BoundaryRelation line 387 should be an IdentityHashMap? line 474+477: Coord.equals(..) maybe check for identity?
MultipolgyonRelation line 468 should be an IdentityHashMap? line 553+556: Coord.equals(..) maybe check for identity?
I guess there are a lot of other places where Coord.equals is used instead of identity.
WanMil
Hi WanMil,
Before fixing that all calls to this method need to be checked if they rely on the current behaviour.
I agree. The problem is that the method is used in rather complex algos, so it's difficult for me to answer this question, esp. when we also have to consider the changes in the high prec branch.
Gerd
_______________________________________________ mkgmap-dev mailing list
mkgmap-dev@.org
_______________________________________________ mkgmap-dev mailing list
mkgmap-dev@.org
_______________________________________________ mkgmap-dev mailing list
mkgmap-dev@.org
-- View this message in context: http://gis.19327.n5.nabble.com/Way-isClosed-tp5788396p5789926.html Sent from the Mkgmap Development mailing list archive at Nabble.com.
Chris66 wrote
Am 04.12.2013 10:53, schrieb Gerd Petermann:
because the way starts with a short segment that is not part of the loop.
And so the way is NOT closed in OSM-sense.
I agree. Question is why the unit test for the is_closed() style function doesn't agree. It seems to expect that the value depends on the data that is available (and might be incomplete). @WanMil: I think we need three different methods in class Way: isClosedInOSM(), hasIdenticalEndPoints(), and hasEqualEndPoints() My understanding is that the style function is_closed() should just return the result of isClosedInOSM() which in turn returns the boolean field isClosed. Do you agree? -- View this message in context: http://gis.19327.n5.nabble.com/Way-isClosed-tp5788396p5790077.html Sent from the Mkgmap Development mailing list archive at Nabble.com.
Chris66 wrote
Am 04.12.2013 10:53, schrieb Gerd Petermann:
because the way starts with a short segment that is not part of the loop.
And so the way is NOT closed in OSM-sense.
I agree. Question is why the unit test for the is_closed() style function doesn't agree. It seems to expect that the value depends on the data that is available (and might be incomplete). @WanMil: I think we need three different methods in class Way: isClosedInOSM(), hasIdenticalEndPoints(), and hasEqualEndPoints() My understanding is that the style function is_closed() should just return the result of isClosedInOSM() which in turn returns the boolean field isClosed.
Do you agree?
Yes. I wonder if hasEqualEndPoints() is required? But you need to take care about any way copy made within mkgmap. I think the closed flag is copied very seldom (which is wrong). WanMil
Do you agree?
Yes. I wonder if hasEqualEndPoints() is required?
not If all routines that create shapes make sure that they add the identical point to close the shape. In trunk, this is not always the case, so I use it find those cases. I'll post a patch later to show my results.
But you need to take care about any way copy made within mkgmap. I think the closed flag is copied very seldom (which is wrong).
hmm, yes, and the docu for is_closed says "+true+ the way is closed. +false+ the way is not closed and cannot be processed as polygon." that would imply that we should use something like way.getPoints.size() > 2 && way.hasIdenticalEndPoints() to evalute is_closed() Gerd
Do you agree?
Yes. I wonder if hasEqualEndPoints() is required?
not If all routines that create shapes make sure that they add the identical point to close the shape. In trunk, this is not always the case, so I use it find those cases. I'll post a patch later to show my results.
But you need to take care about any way copy made within mkgmap. I think the closed flag is copied very seldom (which is wrong).
hmm, yes, and the docu for is_closed says "+true+ the way is closed. +false+ the way is not closed and cannot be processed as polygon." that would imply that we should use something like way.getPoints.size() > 2 && way.hasIdenticalEndPoints() to evalute is_closed()
The docu only tells you that the way cannot be processed as polygon when isClosed() returns false. It does not give any polygon related statement when it returns true. (This is nit-picking but in this case I think it was the intention of the author (me?!??!)). So I vote for making the docu more precise and continue with the investigations. WanMil
Gerd
Hi, I read again this discussion http://www.mkgmap.org.uk/pipermail/mkgmap-dev/2012q4/015264.html I still don't have a good idea what is_closed() should return in case of special cases. Assume these different ways in the input file: input file contains nodes : n1 to n9 , nx is missing in the file but exists in OSM planet, n1 is equal to n9, means, the coordinates rounded to map units are equal way 1 : n1,n2,n3...,n8,n9 way 2 : n1,n2,n3...,n8,n1 way 3: nx,n1,n2,n3,nx way 4: n1,n2,n9 way 5: n1,n2,n1 way 6: nx,n2,nx What result do you expect from the is_closed() style function? I really don't know what to say for the special cases. My understanding is this: a) the style author should not be bothered with the question about incomplete data. If the way is not complete, mkgmap might try to close it, but when style processing starts the way is either closed or not, and closed means first and last point are identical. If the incomplete ways (3 and 6) cannot be closed by mkgmap, they should be dropped beforre style processing. b) A way should not be passed to the polygon rules if it is not closed or has less than four points (three points form a line) c) the style function is_closed() should return true for all ways passed to the polygon style rules d) is_closed() should not return true for a way that cannot be passed to the polygon rules. In other words, I'd prefer to have a rule is_polygon() which returns true if the way has more than 3 points AND first and last point are identical. This can be used in the lines rules to decide that a polygon rule might match better. Comments? Gerd From: gpetermann_muenchen@hotmail.com To: mkgmap-dev@lists.mkgmap.org.uk Date: Tue, 17 Dec 2013 17:29:36 +0100 Subject: Re: [mkgmap-dev] Way.isClosed()
Do you agree?
Yes. I wonder if hasEqualEndPoints() is required?
not If all routines that create shapes make sure that they add the identical point to close the shape. In trunk, this is not always the case, so I use it find those cases. I'll post a patch later to show my results.
But you need to take care about any way copy made within mkgmap. I think the closed flag is copied very seldom (which is wrong).
hmm, yes, and the docu for is_closed says "+true+ the way is closed. +false+ the way is not closed and cannot be processed as polygon." that would imply that we should use something like way.getPoints.size() > 2 && way.hasIdenticalEndPoints() to evalute is_closed() Gerd _______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
My understanding is that is_closed() should return:
way 1 : n1,n2,n3...,n8,n9 false: n1 != n9
way 2 : n1,n2,n3...,n8,n1 true : n1 == n1
way 3: nx,n1,n2,n3,nx true: nx == nx
way 4: n1,n2,n9 false: n1 != n9
way 5: n1,n2,n1 true: n1 == n1 way 6: nx,n2,nx true: nx == nx (StyledConverter will throw it away because it has less than 2 points)
WanMil
Hi WanMil, okay, I've created test files to verify what trunk (r2894) does. Result: way 1: true (n1.equals(n9) == true way 2: true way 3: true way 4: false way 5: true way 6: (not evaluated, is thrown away as expected) way 1 is passed to the polygon rules and is added as a shape to the img file if the rules say so (as they do in my test style) Gerd
Date: Thu, 19 Dec 2013 10:54:21 +0100 From: wmgcnfg@web.de To: mkgmap-dev@lists.mkgmap.org.uk Subject: Re: [mkgmap-dev] Way.isClosed()
My understanding is that is_closed() should return:
way 1 : n1,n2,n3...,n8,n9 false: n1 != n9
way 2 : n1,n2,n3...,n8,n1 true : n1 == n1
way 3: nx,n1,n2,n3,nx true: nx == nx
way 4: n1,n2,n9 false: n1 != n9
way 5: n1,n2,n1 true: n1 == n1 way 6: nx,n2,nx true: nx == nx (StyledConverter will throw it away because it has less than 2 points)
WanMil _______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
participants (4)
-
chris66 -
Gerd Petermann -
GerdP
-
WanMil