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.

Sign up for GitHub

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

Jump to bottom

Ways with different surface tag variants counted twice in track statistics #494

Closed
stesie opened this issue May 6, 2022 · 6 comments
Closed

Ways with different surface tag variants counted twice in track statistics #494

stesie opened this issue May 6, 2022 · 6 comments
Labels
bug

Comments

@stesie
Copy link
Contributor

stesie commented May 6, 2022

With #460 brouter-web now supports ways with a cycleway:surface tag. However, if a way has both a surface and a cycleway:surface tag (and brouter is invoked with processUnusedTags so both are returned), ... then brouter-web counts this way twice in the track statistics:

Screenshot of track statistics w/ ~150m way type and ~310m surface type

Example: https://bikerouter.de/#map=19/49.79318/9.94119/osm-mapnik-german_style&lonlats=9.940186,49.793385;9.942198,49.792959
@mjaschen
Copy link
Contributor

mjaschen commented May 6, 2022 •
edited
Loading

That's on me I guess ;-)

I'll try to fix it ASAP (if that's possible at all: it's unclear which of the both tags should be counted).

@stesie
Copy link
Contributor Author

stesie commented May 6, 2022

If you've got a brouter backend with footway:surface in lookups.dat it could even be three to choose from :-)

Prefering any specialized variant should likely be fine unless processUnusedTags is set. Then it's up to the profile if it's considering it (since it's a cycling profile) or not. With processUnusedTags set however it'll yield wrong statistics with non-bike profiles.

Personally I'd lean towards prefering any specialized variant (and falling back to plain e.g. surface) in favour of little complexity and accept wrongs stats for car profiles... (yet, as a cyclist, I might be biased 😂)

@nrenner nrenner added the bug label May 6, 2022
@mjaschen
Copy link
Contributor

mjaschen commented May 6, 2022

I think we can safely assume that the majority of BRouter-web users utilize it to plan cycling routes ;-)

So I'm with you and think it's ok to prefer the specialized *:surface variants over surface when both are present.

@mjaschen
Copy link
Contributor

mjaschen commented May 7, 2022 •
edited
Loading

FYI, I've implemented a fix.

I'll wait with the pull request until the branches for #68 are merged.

The commit which fixes this bug can be found here: mjaschen@5d75823

mjaschen added a commit to mjaschen/brouter-web that referenced this issue May 7, 2022
@mjaschen
fix for nrenner#494
5d75823 
If a way segment contains more than one `surface` tags (e.g.
`surface=*` and `cycleway:surface=*`), the tag best suited for the
current routing type (currently only *cycling* is supported) is
selected.

i.e. if a `cycleway:surface=` tag belongs to the current way segment
it will be used for the analysis exclusively (all other `surface`
tags are skipped from now on).
@nrenner
Copy link
Owner

nrenner commented May 29, 2022

Could you please make a PR now, planning a release in the coming week.

mjaschen added a commit to mjaschen/brouter-web that referenced this issue May 30, 2022
@mjaschen
fix for nrenner#494
40e3e90 
If a way segment contains more than one `surface` tags (e.g.
`surface=*` and `cycleway:surface=*`), the tag best suited for the
current routing type (currently only *cycling* is supported) is
selected.

i.e. if a `cycleway:surface=` tag belongs to the current way segment
it will be used for the analysis exclusively (all other `surface`
tags are skipped from now on).
nrenner pushed a commit that referenced this issue Jun 1, 2022
@mjaschen
fix for #494 (#550)
4b5d08e 
If a way segment contains more than one `surface` tags (e.g.
`surface=*` and `cycleway:surface=*`), the tag best suited for the
current routing type (currently only *cycling* is supported) is
selected.

i.e. if a `cycleway:surface=` tag belongs to the current way segment
it will be used for the analysis exclusively (all other `surface`
tags are skipped from now on).
mjaschen added a commit to mjaschen/brouter-web that referenced this issue Jun 6, 2022
@mjaschen
Fix for incomplete/buggy solution for nrenner#494
79a1204 
This commit fixes the incomplete/buggy implementation of the fix for nrenner#494.
Values are now converted to the expected format before returned from
`wayTagsNormalize()`.
@mjaschen mjaschen mentioned this issue Jun 6, 2022
Merged
nrenner pushed a commit that referenced this issue Jun 7, 2022
@mjaschen
Fix for incomplete/buggy solution for #494 (#553)
8750c37 
This commit fixes the incomplete/buggy implementation of the fix for #494.
Values are now converted to the expected format before returned from
`wayTagsNormalize()`.
stefankeidel added a commit to cxberlin/brouter-web that referenced this issue Jun 9, 2022
@stefankeidel
Merge remote-tracking branch 'upstream/master'
e4ed990 
* upstream/master: (112 commits)
  release: 0.17.0
  Update changelog for 0.17.0
  Update translations
  Fix RoutingOptions test for CI
  Fix default profile not selected with hash
  Add profile selection tests for RoutingOptions
  Update dependency eslint to v8.17.0 (nrenner#551)
  Fix for incomplete/buggy solution for nrenner#494 (nrenner#553)
  Adapt standalone build to new brouter docs path
  Disable preloading states to reduce traffic on start
  Update dependency del to v6.1.1 (nrenner#549)
  Update dependency eslint to v8.16.0 (nrenner#547)
  Update babel monorepo to v7.18.2 (nrenner#545)
  fix for nrenner#494 (nrenner#550)
  Work around iOS 3rd party browser download (nrenner#418)
  Refactor download
  Allow empty name with client-side formatting (nrenner#454)
  Rename hiking-beta profile to hiking-mountain
  Adopt standalone task to brouter gradle build
  Update overpass-layer
  ...
@nrenner
Copy link
Owner

nrenner commented Jul 12, 2022

Fixed in #550

@nrenner nrenner closed this as completed Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@stesie @mjaschen @nrenner