shortbread: Bring boundaries processing in line with definitions #10
Conversation
pnorman
added
the
shortbread
|
Sorry, I can not figure out from your description or the code what the problem here is. Maybe the problem is the description in the shortbread definition which is rather terse and not worded unambiguously and we need to fix this first. Also the change contains some Can you split this PR into smaller ones somehow? |
…lues Shortbread says 2 and 4 are the valid values of admin_level, so this doesn't look at a relation that is only tagged with type=boundary boundary=administrative and has no admin_level tagging. This doesn't strictly comply with shortbread because a member of a admin_level 3 and admin_level 4 relation should get admin_level set to 3 but I don't believe that's the intent of the spec.
Previously a member of multiple relations would take its disputed status
from the last relation processed. For example, processing a way that is
a member of two relations, with
type=boundary boundary=administrative admin_level=2
type=boundary boundary=disputed admin_level=2
would lead to order-dependent results, where disputed only comes from
the last relation processed. Another issue was that a relation with
type=boundary boundary=disputed
and no other tags would not be considered as tonumber(tags.admin_level)
is not truth-like.
Instead, what is done is to get the disputed status for the relation,
and then set it to true for the rinfos in that relation, not setting
any existing rinfos to false. They are only set false when creating the
entry in rinfos for the first time.
|
I've split it into two commits. Most of the changes are bug fixes for a clear bug where the OSM IDs of relations would change how objects are processed because order mattered. I find the only ambiguity is handling boundary=administrative with no admin_level. There are some parts that are silly, and anything to do with admin_level=3 is not implemented. According to the spec a way that is a member of a admin_level=3 and admin_level=4 relation needs to be in the table with admin_level=3. but if you delete the admin_level=4 relation it is not present. Another case is that admin_level=3 boundary=disputed type=boundary is supposed to be handled the same as admin_level=2 since it is between 2 and 4. |
|
I'd still like to go with this because it fixes what is a clear bug and is a reasonable interpretation of what's written. When we define it more precisely in shortbread we can change the code here to match. |
|
Sorry, I currently don't have the bandwidth to understand all the details here. Lets concentrate on clearly defining this in the standard and then fix the implementation. |
|
I'll bring the boundaries processing into my code then. |
|
This still needs work here at some point, so I'll reopen this as a reminder. |
This removes the bug where boundaries would be processed differently depending on order of relations. Ref osm2pgsql-dev/osm2pgsql-themepark#10
The way disputed boundaries are implemented was not correct. Generally boundaries should be processed as if disputed boundaries do not exists. But then all boundary lines created from ways that have the disputed flag set or that are in a relation tagged boundary=disputed will be marked as disputed. Note that the relation that marks a way as disputed is not the same relation that marks a way as part of some specific admin boundary. The first is tagged boundary=disputed, the second is tagged boundary=administrative. This is based on the work in #10 but rewrites the code to be (hopefully) easier to understand. Note that this only fixes the version for shortbread_v1, the version for shortbread_v1_gen also needs updating which will come later.
|
I have finally spent some time to dive deep into this issue. I believe the code could be simpler, so I made PR #22 which should do the same but hopefully in a way that's easier to understand. |
The way disputed boundaries are implemented was not correct. Generally boundaries should be processed as if disputed boundaries do not exists. But then boundary lines are flagged as disputed if they are created from ways * that have the tag disputed=yes, or * that are in a relation tagged boundary=disputed with no admin_level set or an admin_level smaller or equal to the admin_level that the boundary line has (which is the smallest of any boundary relation the way is a member of) Note that the relation that marks a way as disputed is not the same relation that marks a way as part of some specific admin boundary. The first is tagged boundary=disputed, the second is tagged boundary=administrative. This is based on the work in #10 but rewrites the code to be (hopefully) easier to understand. Note that this only fixes the version for shortbread_v1, the version for shortbread_v1_gen also needs updating which will come later.
The way disputed boundaries are implemented was not correct. Generally boundaries should be processed as if disputed boundaries do not exists. But then boundary lines are flagged as disputed if they are created from ways * that have the tag disputed=yes, or * that are in a relation tagged boundary=disputed with no admin_level set or an admin_level smaller or equal to the admin_level that the boundary line has (which is the smallest of any boundary relation the way is a member of) Note that the relation that marks a way as disputed is not the same relation that marks a way as part of some specific admin boundary. The first is tagged boundary=disputed, the second is tagged boundary=administrative. This is based on the work in #10 but rewrites the code to be (hopefully) easier to understand. Note that this only fixes the version for shortbread_v1, the version for shortbread_v1_gen also needs updating which will come later.
The way disputed boundaries are implemented was not correct. Generally boundaries should be processed as if disputed boundaries do not exists. But then boundary lines are flagged as disputed if they are created from ways * that have the tag disputed=yes, or * that are in a relation tagged boundary=disputed with no admin_level set or an admin_level smaller or equal to the admin_level that the boundary line has (which is the smallest of any boundary relation the way is a member of) Note that the relation that marks a way as disputed is not the same relation that marks a way as part of some specific admin boundary. The first is tagged boundary=disputed, the second is tagged boundary=administrative. This is based on the work in #10 but rewrites the code to be (hopefully) easier to understand. Note that this only fixes the version for shortbread_v1, the version for shortbread_v1_gen also needs updating which will come later.
Shortbread requires ways be part of an administrative boundary, but a boundary=disputed can influence the properties of a way that is also part of boundary=administrative.
I tested this with the following OPL
Output from
select way_id, admin_level, maritime, disputed from boundarieswas