Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shortbread boundaries #22

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Shortbread boundaries #22

wants to merge 2 commits into from

Conversation

joto
Copy link
Contributor

@joto joto commented Nov 28, 2024

First commit: Boundary lines in shortbread have no names

Do not add the names from boundary ways to the output. Some ways have names but use is inconsistent and generally not useful in this context.

Second commit: Fix shortbread boundary processing in regards to disputed boundaries

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.

Do not add the names from boundary ways to the output. Some ways have
names but use is inconsistent and generally not useful in this context.
@lonvia
Copy link

lonvia commented Nov 28, 2024

The disputed relation handling is still not completely correct. If a disputed relation has an admin_level tag, then you would only want to mark the boundary way as disputed if its own level is equal or higher. That means you need to save the level to which the dispute applies.

@joto joto force-pushed the shortbread-boundaries branch from 49d3282 to f021d2a Compare November 28, 2024 15:26
@joto
Copy link
Contributor Author

joto commented Nov 28, 2024

I have changed to commit to take admin_levels of relations tagged boundary=disputed into account.

@joto joto force-pushed the shortbread-boundaries branch from f021d2a to 47da67c Compare November 28, 2024 15:52
-- if either the relation doesn't have an admin_level tag or the
-- admin_level tag is <= the admin level the way got from the
-- boundary=administrative relation(s).
local admin_level = tonumber(object.tags.admin_level or 1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not right, because admin_level will now be nil when object.tags.admin_level is set but is not a number. Must be: local admin_level = tonumber(object.tags.admin_level or 1) or 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added an explicit function instead which makes it more explicit what's happening here.

-- Get numerical admin level from string, default to 1 if invalid
local function get_admin_level(value)
    if not value or not string.match(value, '^[1-9][0-9]?$') then
        return 1
    end

    return tonumber(value)
end

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.
@joto joto force-pushed the shortbread-boundaries branch from 47da67c to 51c341d Compare November 29, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants