Modify

Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#17188 closed enhancement (wontfix)

[Patch] complain about bus stop position without nearby highway=bus_stop

Reported by: mkoniecz Owned by: team
Priority: normal Milestone:
Component: Core validator Version:
Keywords: template_report bus stop Cc:

Description

node with bus=yes and public_transport=stop_position should have nearby highway=bus_stop object.

Example of edit fixing this type of problem: https://www.openstreetmap.org/changeset/66160382

What steps will reproduce the problem?

  1. load area from https://www.openstreetmap.org/changeset/66160382
  2. delete highway=bus_stop nodes
  3. run validator

What is the expected result?

Validator complains about missing highway=bus_stop

What happens instead?

Validator does not complain about missing highway=bus_stop

Please provide any additional information below. Attach a screenshot if possible.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-12-31 15:09:58 +0100 (Mon, 31 Dec 2018)
Build-Date:2018-12-31 14:24:10
Revision:14620
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14620 en) Linux Ubuntu 16.04.5 LTS
Memory Usage: 369 MB / 869 MB (188 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (34535)
+ buildings_tools (34724)
+ continuosDownload (82)
+ imagery_offset_db (34641)
+ measurement (34529)
+ reverter (34552)
+ todo (30306)

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.

Attachments (5)

17188-v1.patch (3.5 KB ) - added by reichg 3 years ago.
patch looks for nodes alongs ways with bus=yes and public_transport=stop_position that do not have a Node with highway=bus_stop and bus=yes within ~25 meters.
17188-v2.patch (3.6 KB ) - added by reichg 3 years ago.
Adjusting distance to ~15 meters instead of 25 to fix reproducable issue provided with the data in the ticket.
17188_v3.patch (9.1 KB ) - added by reichg 3 years ago.
includes user input and checking if feature is in download area.
17188_v4.patch (8.8 KB ) - added by reichg 3 years ago.
put the imports back how they were (optimizing imports moved them around.)
17188_v5.patch (8.7 KB ) - added by reichg 3 years ago.
addressing checkstyle and translations

Download all attachments as: .zip

Change History (36)

comment:1 by skyper, 4 years ago

Needs to take download area into account. Should be oK for id:0.

comment:2 by reichg, 3 years ago

within what like 25-50m?

Last edited 3 years ago by reichg (previous) (diff)

comment:3 by reichg, 3 years ago

@Skyper

What is the best way to find nearby features in josm? Looks like there isn't much built into the framework for gathering features within a bbox or distance.

Last edited 3 years ago by reichg (previous) (diff)

comment:4 by skyper, 3 years ago

I am not sure if I am the right person to answer your question but I try to.

In mapcss, I only know gpx_distance(), see #17131 (GpxDistance.java). Even this function is using Geometry.java, look at line 1262 and following (#17616).

The plugin "public_transport_layer" calculates the distance between stops: DistanceBetweenStops.java

The validator tests for associatedSteet-relation calculates distances between house (numbers): trunk/src/org/openstreetmap/josm/data/validation/tests/Addresses.java?rev=17787#L358

Altogether, calculating the distance between two specified objects, especially nodes, should not be the problem, as we are talking about short distances and we do not need a high precision but how to find/define the two objects. Using "greatCircleDistance" is using quite some resources/time and maybe a smarter calculation can be done.

What is strange, in my eyes, is the fact that mapcss misses a feature to get the coordinates of a node. In the end, this and the primitive distances could be added, I think. -> new ticket

by reichg, 3 years ago

Attachment: 17188-v1.patch added

patch looks for nodes alongs ways with bus=yes and public_transport=stop_position that do not have a Node with highway=bus_stop and bus=yes within ~25 meters.

comment:5 by reichg, 3 years ago

expanding the bbox by 15 meters on each side is arbitrary but seems reasonable for now. It can be easily changed. I have to convert a non-GCS LatLon degree into a number so it is approximately 15 meters. Nothing else I found has an easy way to expand a bbox really precisely. The message can also be an error if need be. This now fixes the "reproducable" data provided in the ticket.

Last edited 3 years ago by reichg (previous) (diff)

by reichg, 3 years ago

Attachment: 17188-v2.patch added

Adjusting distance to ~15 meters instead of 25 to fix reproducable issue provided with the data in the ticket.

comment:6 by skyper, 3 years ago

How about an advanced preference to make the distance user configurable?

in reply to:  6 comment:7 by reichg, 3 years ago

Replying to skyper:

How about an advanced preference to make the distance user configurable?

Yeah I can add that. I also have a few other edits to make to this patch that I noticed. Will have it up soon.

in reply to:  1 comment:8 by skyper, 3 years ago

Replying to skyper:

Needs to take download area into account. Should be oK for id:0.

@Gabe:
Please, check that the node is inside download area or new (id:0). Otherwise this test will produce a lot of false positives.

by reichg, 3 years ago

Attachment: 17188_v3.patch added

includes user input and checking if feature is in download area.

by reichg, 3 years ago

Attachment: 17188_v4.patch added

put the imports back how they were (optimizing imports moved them around.)

comment:9 by reichg, 3 years ago

Milestone: 21.08

comment:10 by skyper, 3 years ago

Keywords: bus stop added
Summary: complain about bus stop position without nearby highway=bus_stop[Patch] complain about bus stop position without nearby highway=bus_stop

comment:11 by skyper, 3 years ago

Well, I only thought about an advanced preferences to set the distance. If I correctly read your patch it adds a dialog which would be new for validator test.

in reply to:  11 ; comment:12 by anonymous, 3 years ago

Replying to skyper:

Well, I only thought about an advanced preferences to set the distance. If I correctly read your patch it adds a dialog which would be new for validator test.

Yup, in the user preferences under the highway test there is a text field which allows users to put in a custom distance for this test. If they ignore it the default is used. (~15 meters)

in reply to:  12 comment:13 by reichg, 3 years ago

Replying to anonymous:

Replying to skyper:

Well, I only thought about an advanced preferences to set the distance. If I correctly read your patch it adds a dialog which would be new for validator test.

Yup, in the user preferences under the highway test there is a text field which allows users to put in a custom distance for this test. If they ignore it the default is used. (~15 meters)

Response was me

comment:14 by skyper, 3 years ago

I see, nice. I was wrong about the new stuff as the ignore file for tag checker has already an input field. All other validator preferences beside the check boxes are only available under advanced preferences and not within validator preferences. In #20689 I thought about a new tab for the general settings but this has only little to do with settings for specific tests.

in reply to:  14 comment:15 by reichg, 3 years ago

Replying to skyper:

I see, nice. I was wrong about the new stuff as the ignore file for tag checker has already an input field. All other validator preferences beside the check boxes are only available under advanced preferences and not within validator preferences. In #20689 I thought about a new tab for the general settings but this has only little to do with settings for specific tests.

Yes and I submitted a bug #21234 regarding validation on the "ok" click within the preferences window. I could not currently implement validation upon that click since the preference window stil closes if there are errors in the tag checker test.

comment:16 by Don-vip, 3 years ago

Milestone: 21.0821.09

The patch needs rework, please run ant checkstyle pmd and fix the issues that will rise. Also some strings must not be hardcoded in English and with numeric value subject to change in the future like "15 m", please make this translatable and configurable with something like tr("... {0}m", 15)

by reichg, 3 years ago

Attachment: 17188_v5.patch added

addressing checkstyle and translations

in reply to:  16 comment:17 by reichg, 3 years ago

Replying to Don-vip:

The patch needs rework, please run ant checkstyle pmd and fix the issues that will rise. Also some strings must not be hardcoded in English and with numeric value subject to change in the future like "15 m", please make this translatable and configurable with something like tr("... {0}m", 15)

Didn't realize josm had checkstyle! Used it before, that's nice! 17188_v5.patch fixed those checkstyle warnings and the translation addition. Pretty sure the issue was only on the labelForBboxExpansion label.

comment:18 by Don-vip, 2 years ago

Milestone: 21.0921.10

Milestone renamed

comment:19 by Don-vip, 2 years ago

Thanks, sorry for delay. New remarks:

  • Can you please extend org.openstreetmap.josm.gui.widgets.AbstractTextComponentValidator for the visual feedback?
  • Please don't cut tr() strings, keep them on a single line. If it's too long for checkstyle, you can silence the warning by wrapping it as follows:
            // CHECKSTYLE.OFF: LineLength
            return tr("a very long line.");
            // CHECKSTYLE.ON: LineLength
    
  • Please don't use English strings in the UI that are not translatable. new JButton("Set Default") should be new JButton(tr("Set Default")). Please also look for existing translated strings in Launchpad that you could reuse (less work for translators).
  • No hardcoded color like Color.RED, we have NamedColorProperty for this. But you don't have to deal with colors at all if you reuse AbstractTextComponentValidator (first remark)
  • string != null && !string.isEmpty() tests should be replaced by Utils.isNotEmpty(string)
  • string.equals("a constant string") should be tested the other way around "a constant string".equals(string) to ensure null-safeness
  • string.equals("") should be replaced by Utils.isBlank(string)
  • Please catch NumberFormatException when you check for number validity, not Exception
  • Please always log catched exceptions. For ignored exceptions you can simply log them at lowest (trace) level Logging.trace(exception);
  • Instead of writing directly to the preferences with put(key, double.toString()), please use an instance of org.openstreetmap.josm.data.preferences.DoubleProperty
  • The preference key should be "validator.highways.missing_bus_stop-distance"

comment:20 by Don-vip, 2 years ago

Milestone: 21.1021.11

comment:21 by GerdP, 2 years ago

I think the ticket should be closed with wontfix.
The wiki https://wiki.openstreetmap.org/wiki/Tag:public_transport=stop_position doesn't mention that a nearby highway=bus_stop node is mandantory for an object with public_transport=stop_position.
The current wiki even says "However, marking the stop position adds no information whatsoever when it is placed on the road at the point closest to highway=bus_stop or on the tram tracks closest to railway=tram_stop. In that case it can be abandoned."
Doesn't that mean that validator should rather complain about the nearby and therefore obsolete public_transport=stop_position node for the given example?
Reg. the code:

  • Do we really need a gui for this preference? We have lots of other validator preferences without a gui. I think the value in the preference should specify metres, similar to e.g. validator.UnconnectedWays.node_way_distance.
  • I think the code as well as the message Node needs a nearby related Node with tags: highway=bus_stop and bus=yes. is wrong. I see no need for bus=yes on a highway=bus_stop node. Maybe Node needs a nearby node with tag highway=bus_stop. is enough?
  • I think a proper test would have to consider the driving side (left-hand traffic or right-hand traffic) to make sure that the "correct" bus_stop node was found.

comment:22 by mkoniecz, 2 years ago

The current wiki even says "However, marking the stop position adds no information whatsoever when it is placed on the road at the point closest to highway=bus_stop or on the tram tracks closest to railway=tram_stop. In that case it can be abandoned."
Doesn't that mean that validator should rather complain about the nearby and therefore obsolete public_transport=stop_position node for the given example?

Deprecating PTv2 proposal ( https://wiki.openstreetmap.org/w/index.php?oldid=625726 ) by JOSM would be likely very controversial.

There are people trying to deprecate highway=bus_stop railway=station amenity=taxi and so on, there are people who want to replace simple tagging by some elaborate scheme duplicating info.

OSM Wiki has conflicting info depending on who last edited it.

It is not helping that PTv2 proposal was not really clear and some people want tagging that goes further than what was discussed then.


In summary the safest would be, I think, encouraging to use standard tagging scheme if only PTv2 was used.


If I would be King-Emperor of OSM then I would definitely go for "complain about the nearby and therefore obsolete public_transport=stop_position node". But I think that doing this is quite risky.

in reply to:  21 comment:23 by skyper, 2 years ago

Replying to GerdP:

I think the ticket should be closed with wontfix.

Or move it to pt_assistant plugin.

The wiki Tag:public_transport=stop_position doesn't mention that a nearby highway=bus_stop node is mandatory for an object with public_transport=stop_position.
The current wiki even says "However, marking the stop position adds no information whatsoever when it is placed on the road at the point closest to highway=bus_stop or on the tram tracks closest to railway=tram_stop. In that case it can be abandoned."

Now, it gets tricky. The wiki is a mess as it is a mix of PTv1 and PTv2.
highway=bus_stop should be mapped on the side of the road for a long time now. But the data still contains a lot of them with or without `platform on the side.
In PTv1 we need a highway=bus_stop or a railway=* on the way or on the side.
In PTv2 we need a public_transport=platform on the side together with an optional stop_position on the way.
As PTv2, on its own, is often not supported (e.g. OSM-Carto) the highway=bus_stop or a railway=* is used as combination on the side. Another reason is a mix of PTv1 and PTv2 relations stopping at the stop.

In Bavaria, public transport companies decided to use only PTv2 tags as they do not want to touch the route relations which adds to the mix. Several discussions with the developers of OSM-Carto came to the result that, cause of the mix, they only support one tagging and that they ignore public_transport=* completely.

Doesn't that mean that validator should rather complain about the nearby and therefore obsolete public_transport=stop_position node for the given example?

In PTv2 stop_position is not obsolete but optional but it needs a platform at least on one side.

Reg. the code:

  • Do we really need a gui for this preference? We have lots of other validator preferences without a gui. I think the value in the preference should specify metres, similar to e.g. validator.UnconnectedWays.node_way_distance.

+1

  • I think the code as well as the message Node needs a nearby related Node with tags: highway=bus_stop and bus=yes. is wrong. I see no need for bus=yes on a highway=bus_stop node. Maybe Node needs a nearby node with tag highway=bus_stop. is enough?

For platform this changed over the time. The transport vehicle is only used on stop_position as we get problems with access tags on ways, especially, together with highway=footway/path but also with highway=platform.
If we warn about stop_position, a platform is needed and highway=bus_stop is option.

  • I think a proper test would have to consider the driving side (left-hand traffic or right-hand traffic) to make sure that the "correct" bus_stop node was found.

+1, but tricky, especially with bus:backward, bus:lanes:backward and oneway:bus=no.

comment:24 by GerdP, 2 years ago

For platform this changed over the time.

So far this ticket did not mention platform and the patch doesn't search for one. A highway=bus_stop doesn't require bus=yes, does it?

If we implement the test we could change the message to something like Consider to add a node with highway=bus_stop or maybe A node with highway=bus_stop might be missing.

comment:25 by skyper, 2 years ago

public_transport=stop_position is PTv2 only and not valid without a public_transport=platform.
highway=bus_stop is a common tag and PTv1 and does not require any bus=yes.

Will I admit that a highway=bus_stop is use- and helpful, the missing object is at least one public_transport=platform. In fact, pt_assistant has a warning about highway=bus_stop without public_transport=*.
The warning should at least suggest both tags.

I see the combination of highway=bus_stop with public_transport=stop_position on a way node as a problem. This combination is still missing a public_transport=platform. highway=bus_stop should not be used on the way but to fix that all relations need to be checked and changed as PTv1 and PTv2 use different roles for the combination of highway=bus_stop + public_transport=platform.

Yet, again PT-tagging is complex with at least two different tagging systems used at the same time which makes it difficult to get exact warnings and suggestions.

comment:26 by GerdP, 2 years ago

@skyper: Does that mean you agree to close the ticket as wontfix?

comment:27 by skyper, 2 years ago

Do not ask me, I am biased.
The question is what do we want to support in core and what better fits in a plugin or external rules.
Do we want to encourage to use both tagging systems for stops? They work together.

We could warn:

  • if both tags, platform and bus_stop, are missing in the area of stop_position
  • if platform is missing in the area of stop_position
  • about highway=bus_stop on a way node (independent of all other warnings and this ticket)
  • about highway=bus_stop without public_transport=platform (already in pt_assistant plugin)
  • about highway=bus_stop together with public_transport=stop_position

comment:28 by Don-vip, 2 years ago

Milestone: 21.11

I won't apply the patch, don't have time to read lengthy discussions is there is no consensus. @Gerd if you feel convinced it must be applied, feel free to do so.

comment:29 by GerdP, 2 years ago

Resolution: wontfix
Status: newclosed

I am sure the patch doesn't work well and the wanted warning would be wrong since the node with bus=yes and public_transport=stop_position doesn't require a highway=bus_stop node nearby.
I might be wrong but I think we have no other test which suggests to add another object with specific tags because "there should be one nearby".
We could as well suggest to add a node with traffic_sign=stop next to a node with highway=stop (or vice versa), but why?
I think these suggestions should not be marked as warnings in the OSM Validator and the warning should not show the correct node. This requires a different representation. MapRoulette challenges might be better for this.

in reply to:  27 comment:30 by skyper, 2 years ago

Ok, the validation of a stop_position missing nearby platform and bus_stop could be added to pt_assistant plugin, I guess, if the bus=yes is removed and public_transport=platform is added. A highway=bus_stopon both platform and stop_position is incorrect, too, which should be checked.

Replying to skyper:

Do not ask me, I am biased.
The question is what do we want to support in core and what better fits in a plugin or external rules.
Do we want to encourage to use both tagging systems for stops? They work together.

We could warn:

  • if both tags, platform and bus_stop, are missing in the area of stop_position
  • if platform is missing in the area of stop_position
  • about highway=bus_stop on a way node (independent of all other warnings and this ticket)
  • about highway=bus_stop without public_transport=platform (already in pt_assistant plugin)
  • about highway=bus_stop together with public_transport=stop_position

Are any of the last three warnings acceptable for core or should they all go into pt_assistant or/and external rules?

comment:31 by GerdP, 2 years ago

Please open a new ticket.
We can add those last three tests to combinations.mapcss if you think they are important. According to overpass there are 203533 nodes with

node["highway"="bus_stop"]["public_transport"="stop_position"];

so it seems quite usual to combine them although it's obviously wrong.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.