Skip to content

Conversation

@pnorman
Copy link
Contributor

@pnorman pnorman commented Sep 30, 2023

Nearly all users will need a sensible definition for if an object is an area or not. This adapts the OpenStreetMap Carto definitions, as updated in the Flex PR. If a user desires their own definitions, they can always override the defaults.

Nearly all users will need a sensible definition for if an object
is an area or not. This adapts the OpenStreetMap Carto definitions,
as updated in the Flex PR. If a user desires their own definitions,
they can always override the defaults.
@pnorman pnorman requested a review from joto September 30, 2023 07:12
@pnorman
Copy link
Contributor Author

pnorman commented Sep 30, 2023

Thinking about, an alternative way to do this would be to have a new type of function with add_proc, 'line', which only handles ways which don't have area tags, and leave 'way' for raw processing of ways. Most themes would use 'line' for everything, while if you're doing something unusual, you could use 'way'.

pnorman added a commit to pnorman/spirit that referenced this pull request Sep 30, 2023
In conjunction with osm2pgsql-dev/osm2pgsql-themepark#1
this makes area code common, reducing what we need to maintain
specific to this style.
@pnorman
Copy link
Contributor Author

pnorman commented Sep 30, 2023

See pnorman/spirit@f80ec01 for an example of the simplification this allows for users, and ensuring that most users will get it right instead of trying to incorrectly implement their own area rules.

@joto
Copy link
Contributor

joto commented Oct 10, 2023

I always thought having such a list of all "area tags" was important but I am questioning myself now. The shortbread implementation for Themepark doesn't need that list. It simply does the checks for the different keys/tags in the way and area callbacks. The advantage is that we are not checking for lots of tags twice, once for the is_area check, once for the actual topic. But I am not sure. Maybe it makes sense to offer this and then users can decide to use it or not?

There is also the problem of handling conflicting tags. What happens if one tag says this is an area and another says this is a line? If we force one result, we might break something. Maybe somebody built an allotment on a dam or so. :-)

In any case I think we need a faster implementation of the area check. It will be called a lot! We should be able to do, at most, two hash lookups for each tag, one for the key, one for the value. If we have nested hashes, the first for looking up the key can result in true (area) or false (line) or a lua table in which case we make another lookup on the tag value in that table. We'll also need the right default if the second lookup fails. Maybe a structure like this:

local is_area_lookup = {
    aeroway = { true, {taxiway = false, runway = false} },
    allotment = { true },
    aerialway = { false, {station = true} },
    ...
}

function is_area_tag(key, value)
    local kr = is_area_lookup(key)
    if kr[2] ~= nil then
        local vr = kr[2][value]
        if vr ~= nil then
            return vr
        end
    end
    return kr[1]
end

We can still have a nicer config to define the keys/tags in any we like and build the fast lookup table from there on startup.

@joto
Copy link
Contributor

joto commented Dec 12, 2023

I have thought about this some more and I don't think this should be part of the Themepark core. You only really need that list if you want to write some generic code, that creates line and polygon geometries for a bunch of data without looking at the tags in detail.

For instance this code here from the commit you mentioned:

if object.tags.building and object.tags.building ~= 'no' and common.isarea(object.tags) then

What is the common.isarea() call actually contributing here? If something is tagged as building and not building=no, then it is already a building, it doesn't matter what other tags it might have, does it?

So I think we should put that code instead into some generic place, where it can be accessed from if needed, maybe the basic theme.

@pnorman
Copy link
Contributor Author

pnorman commented Dec 12, 2023

What is the common.isarea() call actually contributing here?

Because we know the object has an area tag, it's effectively checking for area=no. A building=yes area=no object is unusual, but area=no does get applied to tags that would otherwise be areas, and area tags get applied to objects that would otherwise be linear.

@joto
Copy link
Contributor

joto commented Dec 13, 2023

But that's a pretty expensive call to just check for area=no. Especially when it never happens or is definitely an error as in the combination with building=yes.

@joto
Copy link
Contributor

joto commented Dec 21, 2023

I have created an alternative PR #3. We already had a function to check polygon tags in the "basic" topic. It extends this function. I am not sure whether this is the best place for the function, but if not we can move it somewhere else after we have figured out whether the functionality is the right one.

@pnorman pnorman closed this Feb 6, 2024
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