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

Add new quest to determine the maxheight #960

Merged
merged 41 commits into from
Aug 15, 2018

Conversation

ENT8R
Copy link
Contributor

@ENT8R ENT8R commented Mar 8, 2018

This PR is a little bit more extensive... It adds a quest for determining the maximum height of e.g. tunnels or covered ways, parking entrances for underground and multi-storey parkings and of course height restrictors. It fixes #399, #421, #447.
Sorry for creating so many PRs in the last time, but I hope this helps to get the issue amount below 100 😄

Here are some things I have questions about:

  • Can somebody maybe proofread the list of height units per country?
    Result: the measurement system is now stored in a new file. All countries and their units were checked while adding them.
  • If there is no sign, should we tag it as maxheight=default (used about 10K times) or maxheight=no_sign as proposed by the wiki but only used 14 times according to taginfo...
    see the source code for more information
    Result: There is another menu for selecting the best fitting value
  • Should the query be improved to add the same functionality as the maxheight map by @mmd-osm does?
    Result: don't add additional functionality because this might need some processing
  • If the unit is metric but the input value is an integer (without values after the comma 😄), should automatically a trailing .0 be added by SC? This is also the current behaviour...
    Result: Should not be added and removed

The quest looks like this in countries with the metric system known:

And this is how the quest will look like in countries with the imperial system

This is the current Overpass query: https://overpass-turbo.eu/s/wSP

@matkoniecz
Copy link
Member

I was just reading code, but

This height looks implausible

(if necessary) should be rather something like

This height looks implausible, are you sure that it is correct?

@westnordost
Copy link
Member

Wow, this is a complex one. You even took care of those imperial countries, nice.

Sorry for creating so many PRs in the last time, but I hope this helps to get the issue amount below 100 😄

It would be better if you do one thing after another and see it to the end, rather than creating one construction site after another, in my opinion. These days, it feels like I am spending 90% of the time I have for SC on commenting on tickets or reviewing PRs.
On that note, I feel sorry for fritruc that I will not help him nor review his PR (in this state), seeing the amount of code he fabricated, but I just don't have the time for that. Perhaps you could give him a hand and point out the blunders and possible improvements in his code since you know quite a bit of Android development, the SC source code by now and the quality I demand?

Anyway, will review your code now...

"(" +
" node['barrier'='height_restrictor']" + QUERY_RESTRICTIONS + ";" +
" node['amenity'='parking_entrance']['parking'~'^(underground|multi-storey)$']" + QUERY_RESTRICTIONS + ";" +
" way['highway'~'^(" + ROADS + ")$']['tunnel'~'^(yes|building_passage)$']" + QUERY_RESTRICTIONS + ";" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add avalanche_protector to tunnel types.

{
double height = Double.parseDouble(getHeightFromInput());
double heightInMeter = isMetric ? height : feetToMeter(height);
return heightInMeter > 6;
Copy link
Contributor

@goldfndr goldfndr Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low heights might also be implausible, maybe < 2 m?

" way['highway'~'^(" + ROADS + ")$']['tunnel'~'^(yes|building_passage)$']" + QUERY_RESTRICTIONS + ";" +
" way['highway'~'^(" + ROADS + ")$']['covered'='yes']" + QUERY_RESTRICTIONS + ";" +
");" +
"out meta geom;";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, Overpass QL does not require ' for simple values like i.e. barrier=height_restrictor. Would improve readability a bit.

/*TODO: maxheight=default or maxheight=no_sign?
according to https://taginfo.openstreetmap.org/keys/maxheight#values maxheight=no_sign is only used 14 times,
while maxheight=default is used about 10K times*/
changes.add("maxheight", "default");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what is maxheight=default (or no_sign) supposed to mean? What is the information gain here?

I think it would be better to in that case ask the user about an estimate of the height and tag the estimate then. (Choose a tagging where it is clear that this is an estimate)

Copy link

@mmd-osm mmd-osm Mar 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They’re documented on the Osm wiki ---> https://wiki.openstreetmap.org/wiki/Key:maxheight

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the information gain here?

I guess that in typical country it is mandatory to sign maxheight if it is below some value (I would expect it to be chosen so normal-sized vehicles can pass). So lack of sign indicates that normal vehicles can pass trough. Also, with this it is always possible to tag something, making this quest possible.

about an estimate of the height and tag the estimate then

At least I am not really able to estimate height with any kind of accuracy that would make worth tagging it.

}
}

@Override public String getCommitMessage() { return "Add maximum height"; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is usually plural

{
boolean isParkingEntrance = "parking_entrance".equals(tags.get("amenity"));
boolean isHeightRestrictor = "height_restrictor".equals(tags.get("barrier"));
boolean isTunnel = "yes".equals(tags.get("tunnel"));

This comment was marked as resolved.


@Override public int getTitle() { return R.string.quest_maxheight_title; }
@Override public int getDefaultDisabledMessage() { return 0; }
@Nullable @Override public Boolean isApplicableTo(Element element) { return null; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is. But it would be a long chain of if-checks, I figure. You could use several tagfilters. I.e.... (pseudo code)

TagFilterExpression nodeFilter = parser.parse("nodes with (barrier=height_restrictor or amenity=parking_entrance and parking ~ underground|multi-storey) and !maxheight and !maxheight:physical");
TagFilterExpression wayFilter = parser.parse("ways with highway~(primary|secondary|tertiary|trunk)(_link)?|motorway|service|residential|unclassified|living_street and (covered=yes or tunnel~yes|building_passage) !maxheight and !maxheight:physical");
return nodeFilter.matches(element) || wayFilter.matches(element);

To be honest, with a little extension on the TagFilterExpression class (adding a method that outputs the TagFilterExpression without the full thing (bbox and out geom meta;) as Overpass QL, perhaps these filters could be used as well in the getOverpassQuery method.

if (!heightInput.getText().toString().isEmpty())
{

return heightInput.getText().toString().replace(",", ".");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the "," come from?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. If the user enters it with , instead of . (in Germany, e.g.) However, I think, even the German keyboard layout for numbers limits the input to . as a decimal separator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is why I added it...

I think, even the German keyboard layout for numbers limits the input to . as a decimal separator.

I don't think so... I even specified the possible input values (the same is done for the housenumber quest too) here:
https://github.com/ENT8R/StreetComplete/blob/59b437425c51bacc9c0fd8c8b9866c86e4f474e2/app/src/main/res/layout/quest_max_height.xml#L22

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ENT8R, the line you cited is not there to allow German decimal points but to allow an input like "3,5" (multiple housenumbers).

I do not believe that Android multilingual support is supposed to be that crappy that it leaves it to the app to "understand" the number of the user input.

You should set android:inputType=numberDecimal and probably remove the android:digits="0123456789.,", then everything should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. Will do that!

{
if (!feetInput.getText().toString().isEmpty() && !inchInput.getText().toString().isEmpty())
{
return feetInput.getText().toString() + "." + inchInput.getText().toString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you do this, but I find this dangerous. Ten inch are not one foot, twelve inch are one foot. By introducing the dot, you seem to imply that the inch are the decimal, which they are not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, true... I will have to find a better solution...

<path
android:fillColor="#fff"
android:pathData="M17.967,22.201c3.207,-1.6 5.235,-4.876 5.236,-8.461 -0.002,-3.585 -2.029,-6.86 -5.236,-8.461l-4.224,3.698 -4.217,-3.694C6.317,6.881 4.287,10.156 4.283,13.74c0.005,3.585 2.035,6.859 5.244,8.456l4.217,-3.694z"/>
</vector>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it would also be possible to use the simple layer-list drawable (see background_maxspeed_sign.xml). The advantage would be that it is also displayed as vector on older Android versions.

Copy link
Contributor Author

@ENT8R ENT8R Mar 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some chance to convert it? I don't think so, but I'm not familiar at all with those layer-list drawables...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. But hmm, don't mind about that. Turns out that those two triangles are not possible with shape drawables alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright.

<string name="quest_maxheight_tunnel_title">What is the height limit of this tunnel?</string>
<string name="quest_maxheight_answer_noSign">There is no sign</string>
<string name="quest_maxheight_answer_noSign_confirmation_title">Are you sure there is no sign?</string>
<string name="quest_maxheight_answer_noSign_confirmation">Did you check at both ends? If there is no sign, default limits apply.</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense. What are the "default limits"? Also, it doesn't make sense (IMO) to ask the user to look at "both ends". I think it doesn't matter from which side you enter a tunnel for the height restriction to apply. :-D

# https://en.wikipedia.org/wiki/Metrication#Overview
# https://en.wikipedia.org/wiki/Imperial_units#Current_use
default: [m]
CA: [ft, m]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Canada should be meter only, see https://en.wikipedia.org/wiki/Comparison_of_MUTCD-influenced_traffic_signs
Or, at least, m first, then ft.

{
if (!feetInput.getText().toString().isEmpty() && !inchInput.getText().toString().isEmpty())
{
return feetInput.getText().toString() + "." + inchInput.getText().toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although simplifying inches to a range from 12 to 10 doesn't seem so bad, an 11 inch measurement will give an odd result. (I admittedly didn't check against boundary condition.)

@westnordost
Copy link
Member

Hmm, not sure why it is not shown (to me), but I also wrote:

Unfortunately, the US is not the only country that uses imperial units. You need to separate the selection of the layout from the selection of which unit to select. I.e. this is how the signs look like in UK apparently.

  1. Traffic sign layout: most countries follow the International Standard, see also the comparison of European road signs. This is the layout I saw in your screenshot. However, I think you should also include the "m" in the form, even if it is not a dropdown menu, because it is an element in all the signs.
    Some countries follow the US standard (MUTCD). In the linked page, search for "Height restriction ahead" to see how the sign look there. So, the layout should look like that. If you feel extra-cool, you could make extra forms even on variations, i.e. for Australia.
  2. Meters vs Feet+Inch: So, both layouts need to have both input fields. These input fields can simply be made visible/invisible depending on the country or user's selection where both systems may be used.

@westnordost
Copy link
Member

Should the query be improved to add the same functionality as the maxheight map by @mmd-osm does?

No, because that would potentially show very long stretches of road which would need to be split up. So, this would introduce possibly wrong data because the height restriction surely is not valid for the whole road/stretch but only for the area below the bridge.

@mmd-osm
Copy link

mmd-osm commented Mar 9, 2018

westnordost commented 7 hours ago
Should the query be improved to add the same functionality as the maxheight map by @mmd-osm does?
No, because that would potentially show very long stretches of road which would need to be split up. So, this would introduce possibly wrong data because the height restriction surely is not valid for the whole road/stretch but only for the area below the bridge.

BTW: Some queries in maxheight map depend on extensive post processing. Using the query results as-is will not suffice in all cases. This is especially true for queries with intersection checks.

Instead of adding data to the way, also existing nodes could be used for maxheight tags. This could avoid expensive splitting of roads.

@westnordost
Copy link
Member

westnordost commented Mar 9, 2018 via email

@mmd-osm
Copy link

mmd-osm commented Mar 9, 2018

ping //cc: @slhh


/*the query does not exclude elements which are not accessible by the public,
because a maxheight sign will very probably be visible at the entrance or beginning*/
private static final String QUERY_RESTRICTIONS = "['maxheight'!~'.']['maxheight:physical'!~'.']";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use !tag instead of "tag"!~'.' here : e.g. [!maxheight], or [!"maxheight:physical"]

@slhh
Copy link

slhh commented Mar 9, 2018

If there is no sign, should we tag it as maxheight=default (used about 10K times) or maxheight=no_sign as proposed by the wiki but only used 14 times according to taginfo...

If there is no sign, the user should be asked whether the physical height restricts any normal trafic including hgv. According to the answer we can tag:

  1. No -> maxheight = default
  2. Yes -> maxheight = below_default
  3. Maybe (I can't estimate) -> maxheight = no_indications
  4. I don't remember -> maxheight = no_sign

@rugk
Copy link
Contributor

rugk commented Mar 10, 2018

HGV?

@mmd-osm
Copy link

mmd-osm commented Mar 10, 2018

Hgv : Heavy goods vehicle (LKW in deutsch)

@rugk
Copy link
Contributor

rugk commented Mar 10, 2018

Ah, "truck".😌

@ENT8R
Copy link
Contributor Author

ENT8R commented Mar 10, 2018

I don't remember -> maxheight = no_sign

Why should a surveyor who sees the bridge/tunnel in "real life" should not remember if he is standing right in front of it...?

@matkoniecz
Copy link
Member

"This height looks implausible" message hides value entered by user, maybe it should be somehow displayed in a message?

@matkoniecz
Copy link
Member

matkoniecz commented Mar 10, 2018

I would recommend excluding vehicle=private|no ways.

I would recommend excluding also access=private|no ways.

@matkoniecz
Copy link
Member

If there is no sign, should we tag it as maxheight=default (used about 10K times) or maxheight=no_sign as proposed by the wiki but only used 14 times according to taginfo...

I would use no_sign as it is more clear what is really mapped.

@matkoniecz
Copy link
Member

should automatically a trailing .0 be added by SC?

I see no justification for that - why it should be done?

@ENT8R
Copy link
Contributor Author

ENT8R commented Mar 10, 2018

I would recommend excluding vehicle=private|no ways.
I would recommend excluding also access=private|no ways.

I would not, because this decreases the amount of solveable quest significantly. And very often the maxheight sign will be even displayed at the entrance of e.g. a parking

@matkoniecz
Copy link
Member

I would not, because this decreases the amount of solveable quest significantly

Is it removing unwanted/pointless quests? If yes then I think that significant decrease makes it an even better idea. See cases like https://www.openstreetmap.org/way/518140477#map=19/50.07558/19.93155 or https://www.openstreetmap.org/way/295422615#map=19/50.05658/19.94075

What is the point of mapping maxheight restrictions for places inaccessible to general public? Owners of yard are not going to check maps/routing engines to check whatever they can access their own property.

@ENT8R
Copy link
Contributor Author

ENT8R commented Mar 10, 2018

Owners of yard are not going to check maps/routing engines to check whatever they can access their own property.

Well, that's true, but maybe there are some underground parkings which are not accessible by the general public but if you rent a parking space you can go in there... So there is not a single user but some more people who might be interested in using that data...

@matkoniecz
Copy link
Member

matkoniecz commented Mar 10, 2018

So maybe keep

node['amenity'='parking_entrance']['parking'~'^(underground|multi-storey)$']" + QUERY_RESTRICTIONS + ";" +

unchanged and add access filters for

way['highway'~'^(" + ROADS + ")$']['tunnel'~'^(yes|building_passage)$']" + QUERY_RESTRICTIONS + ";" +

?

This should handle both cases.

@ENT8R
Copy link
Contributor Author

ENT8R commented Mar 10, 2018

Good idea! I will do this.

@ENT8R
Copy link
Contributor Author

ENT8R commented Mar 10, 2018

"This height looks implausible" message hides value entered by user, maybe it should be somehow displayed in a message?

Don't think that this is necessary... The user will either remember the value he entered some moments ago or can simply press Cancel/I will check...

@westnordost
Copy link
Member

whoops, did a rebase-merge instead of a normal merge. In case there will be problems when merging back to master, I will fix them.

@westnordost
Copy link
Member

I tested this a bit, found this issue:

  • If I change the language of my phone to German, it does not let me input 5,5 m.

@westnordost
Copy link
Member

Hmm, regarding the Iceland-thing, it is weird. The LayoutInflater has the correct mcc-infused resources and selects the correct layout to inflate. Furthermore, it passes the same context to views it creates. For testing, I created a new view and included it in the layout.

public class TestView extends View
{
	public TestView(Context context)
	{
		super(context);
		setBackgroundResource(R.drawable.background_maxheight_sign);
	}

	public TestView(Context context,
					@Nullable AttributeSet attrs)
	{
		super(context, attrs);
		setBackgroundResource(R.drawable.background_maxheight_sign);
	}

	public TestView(Context context, @Nullable AttributeSet attrs, int defStyleAttr)
	{
		super(context, attrs, defStyleAttr);
		setBackgroundResource(R.drawable.background_maxheight_sign);
	}

	@TargetApi(Build.VERSION_CODES.LOLLIPOP) public TestView(Context context,
															 @Nullable AttributeSet attrs, int defStyleAttr, int defStyleRes)
	{
		super(context, attrs, defStyleAttr, defStyleRes);
		setBackgroundResource(R.drawable.background_maxheight_sign);
	}
}

Result:
device-2018-08-12-162145

So I am still in the dark as to why it does not work for that other view.

@westnordost
Copy link
Member

Okay I found it. It is not easily solvable, as Google did not anticipate I guess that one would like to work with modified Resources:
For reading the attributes during layout inflation, not directly the context's Resources are used, but the Context's Theme (getTheme). My ContextWrapper implementation in AbstractQuestAnswerFragment only overrides getResources(). While getTheme can also be overridden, the method's return type Resources.Theme is final, so I cannot create something like a ThemeWrapper that instead uses the overridden resources.
This means that resources specified in XML (backgrounds, strings, images, ...) will never be resolved to the current-country resources.

The workaround then is to manually set the resources again programmatically after inflation.

@westnordost
Copy link
Member

westnordost commented Aug 12, 2018

Also, in regards to the earlier problem I noticed: When you write a dot instead of a comma on a non-English locale, the app crashes

Caused by: java.text.ParseException: Unparseable number: "14.5" (at offset 4)
                      at java.text.NumberFormat.parse(NumberFormat.java:557)
                      at de.westnordost.streetcomplete.quests.max_height.AddMaxHeightForm.getHeightFromInput(AddMaxHeightForm.java:192)

Also, a feet has 12 inch. That means, that you should not be allowed to enter 12 inch, but at most 11.

* make signs of height quest look-alike with speed quest and the other way round
* adapt wording a little
* add maxheight sign source svg
@goldfndr
Copy link
Contributor

Although the measure_height_help image is gone from this PR, I have some feedback on it for when it next appears.

  • The object should have a flat top and bottom, to encourage accuracy.
  • The person and object should be at the same level.
  • A flat elevation line should extend from the object to the person, to emphasize that a slope gives less accuracy.

@westnordost
Copy link
Member

Looks like this was an Android bug until Android O. Since this was only fixed then, we still need to workaround this.

…o always be the "." for EditTexts with inputType "numberDecimal", independent of Locale. See https://issuetracker.google.com/issues/36907764
…t be confused whether he should use the one as displayed on the sign or in his phone's locale
@westnordost
Copy link
Member

Completed final tests

@westnordost westnordost merged commit 551f015 into streetcomplete:master Aug 15, 2018
@ENT8R
Copy link
Contributor Author

ENT8R commented Aug 15, 2018

Thank you very much for finishing this! 👍 🎉 I was really busy in the last days...

@ENT8R ENT8R deleted the maxheight branch August 15, 2018 19:35
@westnordost
Copy link
Member

You're welcome! But also, thank you of course for preparing this PR :-)

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.

Quest for maxheight
8 participants