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

[mkoniecz]

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.

[skyper]

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

[reichg]

within what like 25-50m?

[reichg]

@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.

[skyper]

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

[reichg]

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.

[reichg]

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.

[reichg]

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

[skyper]

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

[reichg]

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.

[skyper]

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.

[reichg]

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

[reichg]

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

[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.

[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)

[reichg]

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

[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.

[reichg]

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.

[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)

[reichg]

addressing checkstyle and translations

[reichg]

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.

[Don-vip]

Milestone renamed

[Don-vip]

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"

[GerdP]

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.

[mkoniecz]

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.

[skyper]

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.

[GerdP]

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.

[skyper]

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.

[GerdP]

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

[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

[Don-vip]

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.

[GerdP]

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.

[skyper]

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?

[GerdP]

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.