[PATCH v1] Fix so that all polygons are really closed
As András Kolesár found out the PolygonSplitter did not close all polygons. There were several more places in the code where polygons were not closed. I am not really sure if that caused any problems in the map but definitely the coding was not clean. The patch fixes the conversion between mkgmap objects (List<Coord>) and Java2D objects (Polygon, Area). Additionally the StyleConverter ensures that lines which are assigned with polygonal garmin types are closed by adding the first coord to its list of points. Please test and inspect the polygons crossing the tile borders in particular. WanMil
Now only lines which endpoints are outside or on the bounding box are closed. WanMil
As András Kolesár found out the PolygonSplitter did not close all polygons. There were several more places in the code where polygons were not closed. I am not really sure if that caused any problems in the map but definitely the coding was not clean.
The patch fixes the conversion between mkgmap objects (List<Coord>) and Java2D objects (Polygon, Area). Additionally the StyleConverter ensures that lines which are assigned with polygonal garmin types are closed by adding the first coord to its list of points.
Please test and inspect the polygons crossing the tile borders in particular.
WanMil
Hi WanMil, The patch did not apply cleanly to src/uk/me/parabola/util/Java2DConverter.java revision 1862, because the file in the repository has CR+LF line endings. I believe that we should do "svn propset svn:eol-style native" to every (text) file in the repository. I am now testing the patch on Finland. Marko
On Sun, Feb 27, 2011 at 11:06:22AM +0200, Marko Mäkelä wrote:
I am now testing the patch on Finland.
I got 2386 additional warnings. Some are for thin man_made=pier that have been drawn as lines. There is man_made=* in the polygons style file. Could we suppress the warnings for certain minor unclosed polygons? E.g., something like this: building=* | man_made=* | amenity=* | tourism=* { add mkgmap:polygon-check=false } [0x13 resolution 24] If I disable this rule altogether, the warning count drops from 2386 to 476. That is, the selective suppression of the warnings would help a lot. I would demote the 270 messages about automatic closure to the INFO level. That would leave 206 messages that need to be checked. One more idea: you might add some extra treatment for polygons that are more than 1 "lap". For example, http://www.openstreetmap.org/browse/way/76559964 comprised of nodes 902322231, 902322232, ..., 902322231, 902322232. The second 902322232 triggered the error. Marko
On Sun, Feb 27, 2011 at 11:06:22AM +0200, Marko Mäkelä wrote:
I am now testing the patch on Finland.
I got 2386 additional warnings. Some are for thin man_made=pier that have been drawn as lines. There is man_made=* in the polygons style file. Could we suppress the warnings for certain minor unclosed polygons? E.g., something like this:
building=* | man_made=* | amenity=* | tourism=* { add mkgmap:polygon-check=false } [0x13 resolution 24]
If I disable this rule altogether, the warning count drops from 2386 to 476. That is, the selective suppression of the warnings would help a lot.
Mmh, I am not a big fan of suppressing warning messages. But I see that you have the problem that you only want to use man_made=pier with polygons. If you add a "dead" rule to the line style you will loose all man_made=pier. Would it help you if I add the tags of the way to the warning message? Then you could easily filter the message.
I would demote the 270 messages about automatic closure to the INFO level.
Ok.
That would leave 206 messages that need to be checked.
One more idea: you might add some extra treatment for polygons that are more than 1 "lap". For example, http://www.openstreetmap.org/browse/way/76559964 comprised of nodes 902322231, 902322232, ..., 902322231, 902322232. The second 902322232 triggered the error.
Is this really a big problem? It is an error in the OSM data and just in case this does not happen very often we should not support such errors by fixing them automatically. WanMil
Marko
On Sun, Feb 27, 2011 at 12:59:09PM +0100, WanMil wrote:
If I disable this rule altogether, the warning count drops from 2386 to 476. That is, the selective suppression of the warnings would help a lot.
Mmh, I am not a big fan of suppressing warning messages. But I see that you have the problem that you only want to use man_made=pier with polygons. If you add a "dead" rule to the line style you will loose all man_made=pier. Would it help you if I add the tags of the way to the warning message? Then you could easily filter the message.
I see that you implemented the tag list printout. It might be good enough.
One more idea: you might add some extra treatment for polygons that are more than 1 "lap". For example, http://www.openstreetmap.org/browse/way/76559964 comprised of nodes 902322231, 902322232, ..., 902322231, 902322232. The second 902322232 triggered the error.
Is this really a big problem? It is an error in the OSM data and just in case this does not happen very often we should not support such errors by fixing them automatically.
It is probably not a big problem, and I was not thinking that we should try to fix any garbage automatically. It could be nice to have the message distinguish incorrectly closed polygons from unclosed polygons. It took me some time to figure out the problem. I was expecting to see a gap between some points, but there was none. Only after I split the way in JOSM, the highlighting of the area hinted me where the starting point was. Marko
Marko Mäkelä (marko.makela@iki.fi) wrote:
On Sun, Feb 27, 2011 at 11:06:22AM +0200, Marko Mäkelä wrote:
I am now testing the patch on Finland.
I got 2386 additional warnings. Some are for thin man_made=pier that have been drawn as lines. There is man_made=* in the polygons style file. Could we suppress the warnings for certain minor unclosed polygons? E.g., something like this:
building=* | man_made=* | amenity=* | tourism=* { add mkgmap:polygon-check=false } [0x13 resolution 24]
In my custom style I have adjusted the polygons style rule to account for this type of case: man_made=pier & area=yes [etc] Though this depends, of course, on someone tagging the pier properly. A mkgmap error message would help me to identify where this hasn't been done. -- Charlie
Moin, Marko Mäkelä schrieb am 27.02.2011 10:32:
I got 2386 additional warnings. Some are for thin man_made=pier that have been drawn as lines. There is man_made=* in the polygons style file.
I have not tested the polygonclose_v2.patch but it sounds like it has the inverse effect of the previous restrict_polygon_v1.patch, which sounds like a very bad idea to me. In the OSM data we have sometimes identical tags for line objects and for area objects. The restrict_polygon_v1.patch was build, so that the polygon rules were only applied, if the OSM-way is closed. Now the new patch seems to close automatically all ambigous ways, making the previous problem even worse. Gruss Torsten
Moin,
Marko Mäkelä schrieb am 27.02.2011 10:32:
I got 2386 additional warnings. Some are for thin man_made=pier that have been drawn as lines. There is man_made=* in the polygons style file.
I have not tested the polygonclose_v2.patch but it sounds like it has the inverse effect of the previous restrict_polygon_v1.patch, which sounds like a very bad idea to me.
In the OSM data we have sometimes identical tags for line objects and for area objects. The restrict_polygon_v1.patch was build, so that the polygon rules were only applied, if the OSM-way is closed. Now the new patch seems to close automatically all ambigous ways, making the previous problem even worse.
Gruss Torsten
Torsten, it's the other way round. The patch tests for all unclosed ways that are assigned with a garmin polygon type if both endpoints are outside the bounding box. Only in this case the polygons are closed automatically. Before r1865 all lines that were assigned with a garmin polygon type were closed automatically. WanMil
WanMil schrieb am 27.02.2011 15:37:
it's the other way round. The patch tests for all unclosed ways that are assigned with a garmin polygon type if both endpoints are outside the bounding box. Only in this case the polygons are closed automatically.
Ok, perhaps take an example, so that I can understand what patch has which effect. There is an OSM-way with Tags: tagA=valueA and Nodes: node1 node2 node3 (node1 /= node3) In the polygon style there is a line tagA=valueA [0x01 continue] And in the lines style there is a line tagA=valueA [0x02 continue] With r1733 this will result in two elements in the garmin map: 1. A triangular shape with the type 0x01 and the corners node1, node2 and node3. 2. A line with the type 0x02 from node1 via node2 to node3. With r1733 and restrict_polygon_v1.patch this will result only in one element in the garmin map: A line with the type 0x02 from node1 via node2 to node3. What will be the result of r1733 and polygonclose_v2.patch? What will be the result of r1733 and polygonclose_v2.patch and restrict_polygon_v1.patch? (Would this make any sense at all?) Gruss Torsten
it's the other way round. The patch tests for all unclosed ways that are assigned with a garmin polygon type if both endpoints are outside the bounding box. Only in this case the polygons are closed automatically.
Ok, perhaps take an example, so that I can understand what patch has which effect.
There is an OSM-way with Tags: tagA=valueA
and Nodes: node1 node2 node3
(node1 /= node3)
In the polygon style there is a line
tagA=valueA [0x01 continue]
And in the lines style there is a line
tagA=valueA [0x02 continue]
With r1733 this will result in two elements in the garmin map: 1. A triangular shape with the type 0x01 and the corners node1, node2 and node3. 2. A line with the type 0x02 from node1 via node2 to node3.
With r1733 and restrict_polygon_v1.patch this will result only in one element in the garmin map: A line with the type 0x02 from node1 via node2 to node3.
What will be the result of r1733 and polygonclose_v2.patch?
What will be the result of r1733 and polygonclose_v2.patch and restrict_polygon_v1.patch? (Would this make any sense at all?)
Gruss Torsten
I have committed the (slightly changed) polygonclose_v2.patch which is now r1865. Your example will result in 1. line with 0x01 2. a) nothing more if node1 or node2 is within the tiles bounding box b) a polygon 0x02 if node1 and node2 is outside or on the tiles bounding box. Both patches do not make sense because if appliying restrict_polygon_v1.patch the code of polygonclose_v2.patch will never be reached. WanMil
WanMil schrieb am 27.02.2011 16:53:
Your example will result in 1. line with 0x01 2. a) nothing more if node1 or node2 is within the tiles bounding box b) a polygon 0x02 if node1 and node2 is outside or on the tiles bounding box.
Ok, I will try out r1865 when it is availbale as a download. But in my opinion 2.b) is a fault: If the polygon is not closed in the OSM data, it shouldn't be closed by mkgmap, no matter whether the start and end nodes are inside or outside the bounding box. Gruss Torsten
Am 27.02.2011 16:58, schrieb Torsten Leistikow:
WanMil schrieb am 27.02.2011 16:53:
Your example will result in 1. line with 0x01 2. a) nothing more if node1 or node2 is within the tiles bounding box b) a polygon 0x02 if node1 and node2 is outside or on the tiles bounding box. Ok, I will try out r1865 when it is availbale as a download.
But in my opinion 2.b) is a fault: If the polygon is not closed in the OSM data, it shouldn't be closed by mkgmap, no matter whether the start and end nodes are inside or outside the bounding box But correct polygons gets divided into two or more unclosed polygons by splitting the hole osm-datafile into smaller tiles. These polygons should be closed by mkgmap.
Maybe it could be managed if polygons gets only closed, then there isn't only a rule in polygons-file. Example: man_made=pier in polygons and lines-file -> no closing man_made=pier & area=yes in polygons-file and man_made=pier & area!=* in lines-file -> close, if nodes are outside man_made=pier only in polygons-file -> close, if nodes are outside Henning
WanMil schrieb am 27.02.2011 16:53:
I have committed the (slightly changed) polygonclose_v2.patch which is now r1865.
A first check of r1866 looked good. More thorough testing will certainly follow, when the merging of the index branch is finished. Gruss Torsten
participants (5)
-
charlie＠cferrero.net -
Henning Scholland -
Marko Mäkelä -
Torsten Leistikow -
WanMil