a-b-street / osm2lanes

A common library and set of test cases for transforming OSM tags to lane specifications
https://a-b-street.github.io/osm2lanes/
Apache License 2.0
34 stars 2 forks source link

Rust: Sidewalk #51

Closed droogmic closed 2 years ago

droogmic commented 2 years ago

Add sidewalk support, fixes issue #49.

tordans commented 2 years ago

Just for me to learn (and maybe as a suggestion):

Are you normalizing tags before processing? Like renaming sidewalk=yes to sidewalk:both=yes (or the other way around)?

I image with this project, a first set of tests would be about normalization and the the second set to test on the normalized data?

droogmic commented 2 years ago

Just for me to learn (and maybe as a suggestion):

Are you normalizing tags before processing? Like renaming sidewalk=yes to sidewalk:both=yes (or the other way around)?

I image with this project, a first set of tests would be about normalization and the the second set to test on the normalized data?

Depending on your definition of normalization and categorization, yes and no. This is actually a categorizing step (a type of normalization?), the enum at https://github.com/a-b-street/osm2lanes/blob/main/rust/osm2lanes/src/transform/tags_to_lanes/foot_shoulder.rs#L18 represents all the variants of sidewalk for each side as implied by the tags. We then parse the tag strings to find which variant is valid for each side. A later step can then additionally guess if a shoulder or sidewalk is needed based on other input.

The match statement is the core of the business logic:

    match (
        tags.get(SIDEWALK),
        tags.get(SIDEWALK + "both"),
        tags.get(SIDEWALK + locale.driving_side.tag()),
        tags.get(SIDEWALK + locale.driving_side.opposite().tag()),
    )

We are constructing a switch statement that needs to cover all values of sidewalk, sidewalk:both, sidewalk:left and sidewalk:right. The reason to do this instead of normalization where sidewalk is blindly combined with sidewalk:both is it would allow us to catch erroneous cases.

We can now handle unusual situations like this:

sidewalk=yes
sidewalk:both=yes

(we currently throw an error, but we could change it to just throw a warning)

so that ugly cases like the following are caught as an error:

sidewalk:both=yes
sidewalk:left=no

Normalization could imply mutability which is a bit of a code smell (especially in rust), so I think I will try to go as far as possible without mutating the input tags by categorizing where needed.

As an aside: Your work in Berlin inspired me to work on this, so I hope I can bring this to a level where it is useful for your work.

droogmic commented 2 years ago

test on the normalized data

We maybe do something like what you are expecting, by testing that we can roundtrip. so if we convert tags to lanes, we check that those lanes -> tags -> lanes is a perfect roundtrip.

droogmic commented 2 years ago

I tried https://github.com/a-b-street/osm2lanes/commit/d0ca4b6e64c21c16d0a196c42fb569e6039cac5f

but I don't like the (None, None) => unreachable!(), and I am also not sure it adds much.

will try something else