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

Country specific highway access - Project-OSRM / osrm-backend #7038 #7022

Closed
wants to merge 41 commits into from

Conversation

tombay
Copy link

@tombay tombay commented Aug 29, 2024

An issue #6710 has been logged (reopening #6701).
Address the issue of highway=trunk (and trunk_link) being blocked for foot and bicycle profiles. Discussed in https://wiki.openstreetmap.org/wiki/Key:motorroad and https://wiki.openstreetmap.org/wiki/OSM_tags_for_routing/Access_restrictions (esp #Alternative_ideas) proposed to address issue via
osrm-backend ... --location-dependent-data

Tasklist

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@tombay
Copy link
Author

tombay commented Aug 29, 2024

I had inadvertently created pr #7013 on not the master branch. It deleted when I removed that branch. #6710 and #6701 have described the issue.

@frodrigo
Copy link
Member

frodrigo commented Sep 24, 2024

  • Please, can you include only one simplified geojson n the PR.
  • Can you redact the list of commit to simplify the history ? And make it more easy to review.
  • Can we merge it into the default foot en bicyle profile ?

@tombay
Copy link
Author

tombay commented Sep 26, 2024

  • I have updated foot.lua and bicycle.lua to support country data. I will remove countryfoot.lua and countrybicycle.lua.

  • The geojson file test/data/countrytest.geojson defines a simplified array of countries:
    Switzerland (CHE), Finland (FIN), France (FRA), Belgium (BEL), Greece (GRC), Ireland (IRL)and Lithuania (LIU).
    This file along with features/foot/countryspeeds.feature outlines the relationships.

  • I am not exactly sure how to clean up the commit stream.
    (In my repository == git branch master ; git rebase --interactive ?)

@frodrigo
Copy link
Member

git rebase --interactive

Yes. It it is the first time you use it. Be careful, make a copy of you local clone ;-).

Switzerland (CHE), Finland (FIN), France (FRA), Belgium (BEL), Greece (GRC), Ireland (IRL)and Lithuania (LIU).

Before I was not understanding you need the countries poly to map to properties. It is not how the other geo-filters work.

It may not be a bad idea. And could be used by other geo-filter. But it probably better to make this in an other PR and keep this one more simple. Just a suggestion.

@tombay tombay changed the title Country specific highway access. fixes Project-OSRM / osrm-backend #6710 Country specific highway access. fixes Project-OSRM / osrm-backend #7038 Sep 27, 2024
@tombay tombay changed the title Country specific highway access. fixes Project-OSRM / osrm-backend #7038 Country specific highway access - Project-OSRM / osrm-backend #7038 Sep 27, 2024
@tombay tombay closed this Sep 27, 2024
@frodrigo
Copy link
Member

@tombay you can push -f your branch to update the same PR. No need to close it.

@tombay
Copy link
Author

tombay commented Sep 28, 2024

@frodrigo - Excuse the mess.
I wanted to revert back to the start of my master - to just do the "trunk only" changes. I force pushed to the start of my fork and that closed the PR. I cannot re-open it as it is on Project-OSRM. A force-push to my latest change restores my fork but does not reopen the PR. When I have sorted out my code I will probably recreate my fork and hence a new PR. I had moved my code from my local branch (tombay-country-profiles) to my master branch (tombay:master) because automated testing did not seem to work on tombay-country-profiles.

@tombay tombay reopened this Sep 28, 2024
@tombay
Copy link
Author

tombay commented Sep 28, 2024

I have reopened it %-) . I have linked this to the Feature Request #7038.
I shall create a PR for issue #6710.

@frodrigo
Copy link
Member

frodrigo commented Oct 3, 2024

Never do a merge from remote on a PR branch, always use rebase to keep you RP branch clean and readable.

@tombay
Copy link
Author

tombay commented Oct 3, 2024

Sorty. I shall look into fixing this %-(.

@tombay
Copy link
Author

tombay commented Oct 4, 2024

In reviewing https://wiki.openstreetmap.org/wiki/OSM_tags_for_routing/Access_restrictions and just considering highway="trunk" and highway="trunk_link" there are only seven countries that do not support foot and bike access.
(France, Belgium, Switzerland, Austria, Slovakia, Hungary and Denmark).
Currently foot.lua and bicycle.lua exclude trunk and trunk_link.
SO
1/ if uselocationtags = trunk is added to the profile then include trunk and trunk_link in the valid speed list.
2/ if osrm-extract argument --location-dependent-data data/notrunk.geojson is used then the countries can be excluded.
The geojson file is only 50k.

The only issue is that if you want to build a dataset that includes a mixture of "trunk" and "notrunk" countries you must remember the location-dependent-data command option.

@tombay
Copy link
Author

tombay commented Oct 4, 2024

@frodrigo : I am removing all the extra data that I was writing to full support the access_restriction.
there are so many commits I will clean it all up.
So git reset --hard (to get to when I forked)
git rebase ( to bring the fork to the latest from Project-OSRM:master)
then create my branch
OR
delete and recreate my fork.

In either case this PR will close

@frodrigo
Copy link
Member

frodrigo commented Oct 4, 2024

delete and recreate my fork.

You just need a branch. But yes, you can reset you master.

In either case this PR will close

Yes better.

@tombay tombay closed this by deleting the head repository Oct 5, 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