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

Segregated #1135

Merged
merged 4 commits into from Aug 15, 2018
Merged

Segregated #1135

merged 4 commits into from Aug 15, 2018

Conversation

matkoniecz
Copy link
Member

If desirable I may split ImageList modification to a separate PR.

Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

I will draw the pictograms and an icon

@@ -85,7 +85,7 @@

protected int getItemsPerRow()
{
return 4;
return Math.min(4, getItems().length);
Copy link
Member

Choose a reason for hiding this comment

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

This and the changes to the other quests is not a good idea. The quest types below have 3 per row, not because they are exactly three choices, but the size of photo-selection-items should be not still smaller than that. Pictograms and icons are usually clear enough that they can be smaller.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will drop this part.

Pictograms and icons are usually clear enough that they can be smaller.

Is it still desirable to show two small icons with blank space on the right side (that is what happens where there is not enough entries to fill even the first row?

Generally, I see not even single quest where getItemsPerRow was set to value higher than count of possible answers.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed from this PR

@@ -121,6 +122,7 @@
new AddWheelChairAccessToilets(o),
new AddReligionToWaysideShrine(o),
new AddBikeParkingType(o),
new AddSegregated(o),
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 what is segregated could be put in the class name here

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously it was baroque AddSegregatedStatusOfCombinedFootwayAndCycleway but it was ridiculous.

AddIsCyclewaySegregated?

Copy link
Member

Choose a reason for hiding this comment

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

AddCyclewaySegregated?

Copy link
Member Author

Choose a reason for hiding this comment

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

AddCyclewaySegregated sounds like it is about mapping cycleways. AddCyclewaySegregatedStatus?

Copy link
Member

Choose a reason for hiding this comment

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

AddCyclewaySegregation?

Copy link
Member Author

Choose a reason for hiding this comment

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

that is good, thanks

EDIT: applied

"(" +
"(highway = path and bicycle = designated and foot = designated)" +
" or (highway = footway and bicycle = designated)" +
" or (highway = cycleway and foot = designated)" +
Copy link
Member

Choose a reason for hiding this comment

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

Why only designated?

Copy link
Member Author

Choose a reason for hiding this comment

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

because (at least in my experience) highway=footway + bicycle=yes is always segregated=no

Is it possible that bicycle has dedicated part of path and the way is not dedicated to usage by cyclists?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can change that but I am curious how correctly tagged "highway=footway + bicycle=yes + segregated=yes" or "highway=cycleway + foot=yes + segregated=yes" looks like.

Copy link
Contributor

@rugk rugk Jul 15, 2018

Choose a reason for hiding this comment

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

Did not we discuss this in the issue already?

highway=footway + bicycle=yes + segregated=yes

Okay, yeah, must always be not segregated. Don't know what this has to do with the quest selection though.


But I'd also include the yes values. Because the wiki says:

This [segregated] key is used for combined cycle- and footways. If both have their own lane, tag segregated=yes. If they share one lane, tag segregated=no.

So actually segregated is never implied and as such the tagging of bike and footway is incorrect without a segregated tag. And it is best, if a surveyor can verify or deny whether the way is segregated or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is rather a good reason to change wiki.

If someone can provide a photo of correctly tagged "highway=footway + bicycle=yes + segregated=yes" or "highway=cycleway + foot=yes + segregated=yes" location I may make change that will include =yes values.

Copy link
Member Author

Choose a reason for hiding this comment

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

The sign, @ENT8R linked or included there – which link is apparently gone – is this one:

Yes, I know about such places. But every single one that I encountered with this sign of equivalent is not segregated. That is why I asked for a photo of such location.

Copy link
Member

Choose a reason for hiding this comment

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

To add my two cents:
At least in Germany, and apparently documented that way in the German wiki (didn't check), there exists the opinion that only cycleways which are compulsory to use should be tagged with cycleway=designated. And I think this has been documented like this already a long time because I remember when I started with OSM, I decided to rather not tag any cycleway as designated (from satellite imagery) but rather as just yes because I was not sure if it was compulsory to use. See also this forum topic to see that this opinion is still around.
Long story short, and wrong as this "only in Germany, designated has a special additional meaning" may be, it has been practised like this for a long time, so chances are that a lot of these situations are around.

If the solution here should be to include all yes as well then, ... well, this is something we should discuss.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it also never does any harm to include yes. These are only a few extra roads then, ….

Copy link
Member Author

Choose a reason for hiding this comment

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

"only a few extra roads" - my city has 970+ highway=footway + bicycle=yes without segregated, all of them are segregated=no ( http://overpass-turbo.eu/s/Ajo )

Copy link
Member

Choose a reason for hiding this comment

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

Well, then perhaps not include footways with cycleway=yes after all. The Germans just need to get their act together and fix their tagging practice.

" or (highway = footway and bicycle = designated)" +
" or (highway = cycleway and foot = designated)" +
")" +
" and surface !~" + TextUtils.join("|", OsmTaggings.ANYTHING_UNPAVED) +
Copy link
Member

Choose a reason for hiding this comment

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

or rather surface ~ ...ANYTHING_PAVED?

Copy link
Member Author

Choose a reason for hiding this comment

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

This quest is after surface quest so either

So the difference exists when

So in the end I planned to convince you that surface !~ ANYTHING_UNPAVED is preferable but rather convinced myself.

Copy link
Member Author

Choose a reason for hiding this comment

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


@Override
public void applyAnswerTo(Bundle answer, StringMapChangesBuilder changes) {
List<String> values = answer.getStringArrayList(AddPowerPolesMaterialForm.OSM_VALUES);
Copy link
Member

Choose a reason for hiding this comment

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

PowerPolesMaterialForm?

Copy link
Member Author

Choose a reason for hiding this comment

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

ops (it still worked)

EDIT: Fixed

@@ -609,5 +609,8 @@ Otherwise, you can download another keyboard in the app store. Popular keyboards
<string name="quest_address_answer_no_housenumber_message2">Is this correct and is it not just part of a building?</string>

<string name="notification_channel_download">"Download"</string>
<string name="quest_segregated_title">How footway and cycleway are combined here?</string>
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, my first suggestion is:

How are the foot- and cycleway laid out here?

  1. Segregated from one another
  2. Cyclists and pedestrians share same space

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I really like this version.

edit: changed

@matkoniecz
Copy link
Member Author

I will draw the pictograms and an icon

Thanks!

return
"ways with " +
"(" +
"(highway = path and bicycle = designated and foot = designated)" +
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, did you consider what happens if this quest get's tagging invalid values? Such as highway = footway and foot = no (and e.g. bicycle=designated to trigger this quest)?
Don't know whether/how we should handle it, but it may be an edge-case that is happening and could result in bad taggings or so.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, did you consider what happens if this quest get's tagging invalid values? Such as highway = footway and foot = no (and e.g. bicycle=designated to trigger this quest)?

No, but I will do it now. A good point, thanks for catching.


Such combination appears 25 times worldwide ( http://overpass-turbo.eu/s/Aj7 ) so I will happily ignore it.

I opened notes for this problems and commented on changesets that introduced this. I will consider asking for JOSM validator complaints on highway=footway and foot=no.

Copy link
Contributor

@rugk rugk Jul 15, 2018

Choose a reason for hiding this comment

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

Of course it is rare – everything else would be embarrassing. 😉
But I don't think ignoring is the best way here, because we are not talking about including tags/quests, because they are rare, but about a mistagging that may affect this quest.


Thinking about it, however, I guess it really is not bad to show this quest for such situations. Because it does not worsen the mistagged information (i.e. now the footway/foot=no is tagged with segregated=no or so, well… who cares?) and one also does not expect that SC would fix these issues.


Generally maybe it would be a good idea to add these two other answers though:

"This is a cycleway only" | "This is a footway only"

Just leaving a note for these small changes, which are very easy to identify for a surveyor looking at the path, would be a bit too much, IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding your last suggestion, @rugk , if you look outside "signposted" Germany, you will find that many sidewalks and paths will have no signs at all. How would one distinguish the case that pedestrians and cyclists are mixed from that no cyclists are allowed (i.e. because surveyor doesn't see one right now or subjectively thinks cyclists should not use this path)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yeah, indeed difficult. So rather leave them out.

@matkoniecz matkoniecz changed the title Segregated [drawables are missing] Segregated [drawables are missing + waits for fixes by PR author] Jul 15, 2018
@matkoniecz matkoniecz changed the title Segregated [drawables are missing + waits for fixes by PR author] Segregated [drawables are missing + retest after adding graphics] Jul 18, 2018
@westnordost
Copy link
Member

The drawables are there now, btw

@matkoniecz matkoniecz changed the title Segregated [drawables are missing + retest after adding graphics] Segregated [add drawables + retest after adding graphics] Jul 22, 2018
@rugk
Copy link
Contributor

rugk commented Jul 22, 2018

So another thing: What about including side lanes? There, those features are likely most relevant.

@matkoniecz
Copy link
Member Author

What about including side lanes?

Can you clarify what you mean? It is unclear what and how is supposed to be included.

@rugk
Copy link
Contributor

rugk commented Jul 22, 2018

Sorry, meant sidewalks. They may also be queried and the same tags (with prefix) can be added. Example: #527 (comment)

@matkoniecz
Copy link
Member Author

It is about "#527 (comment) but not mapped as a separate path."? I see no good reason to handle it in the same quest - it requires a completely different query and applied tags are also different.

@rugk
Copy link
Contributor

rugk commented Jul 23, 2018

Yeah it's about the comment I've linked (it's the beginning).

E.g.:

cycleway:right=track
cycleway:right:bicycle=designated
cycleway:right:segregated=yes

Or with sidewalk:

sidewalk=right sidewalk:right:foot=designated sidewalk:right:bicycle=yes sidewalk:right:oneway=*

I see no good reason to handle it in the same quest

Excect taht it is totally the ame question, UI and everything. Okay, maybe slight changes to indicate the user what way the quest asks about, but it's the same topic with the same possible quest.

applied tags are also different.

Not really, you just have to prefix them. AFAIK this was also done in other quests like this.

@matkoniecz
Copy link
Member Author

matkoniecz commented Jul 23, 2018

It requires different UI (to handle separately left/right info - both form and question will be different), a completely different query and requires handling of prefixes for added tags.

Only quest icon will be the same.

@rugk
Copy link
Contributor

rugk commented Jul 23, 2018

I thought it could just ask for the left and right sidewalk separately, then the UI is the same. In a minimal way, the question alone could mention, which sidewalk it is asked about.

@matkoniecz matkoniecz changed the title Segregated [add drawables + retest after adding graphics] Segregated [retest after adding graphics] Jul 23, 2018
@matkoniecz
Copy link
Member Author

You need to communicate to user which sidewalk is right and left (it would probably reuse cycleway/oneway quest UI)

@matkoniecz
Copy link
Member Author

I am unsure is it important enough to block merging or is it a pointless bikeshedding.

At least on my phone, the initial quest UI view shows almost entire selection area, but a small part is obscured and needs to be pulled up. I think that it would be preferable to show all of it (similar things happens in some other quests).

The second issue is that the text on the "shared space" symbol is on part of pedestrian symbol what is not looking ideal.

screenshot_2018-07-23-13-18-22-906_de westnordost streetcomplete debug

@matkoniecz matkoniecz changed the title Segregated [retest after adding graphics] Segregated [ready for review] Jul 24, 2018
@westnordost
Copy link
Member

Hmm right, this does not look nice. You can easily change the size of the xml vector icons by modifying the android:width/height property.

Quest forms that show icons usually use another item layout which shows the text below the icon, not above it. See for example the sports quests. Also possible would be to show the text right of the icon, like in the building quest (but in two columns, if there is space).

@matkoniecz matkoniecz changed the title Segregated [ready for review] Segregated [change layout] Jul 24, 2018
# Conflicts:
#	app/src/main/java/de/westnordost/streetcomplete/quests/QuestModule.java
#	app/src/main/res/values/strings.xml
@westnordost
Copy link
Member

done and tested.

device-2018-08-15-215158

@westnordost westnordost merged commit 616e93d into streetcomplete:master Aug 15, 2018
@westnordost westnordost changed the title Segregated [change layout] Segregated Aug 15, 2018
@matkoniecz
Copy link
Member Author

Sorry, I had no time recently - and thanks for finishing this!

@rugk
Copy link
Contributor

rugk commented Aug 16, 2018

BTW while adapting the overpass-query for overpass-turbo for the Quests wiki, I noticed that this quest seems to be extremely rare. In whole Rom (in Den Haag: only 8 and Netherlands are said to be bicycle-friendly) only one element was returned. When removing the "paved street" limitation it were a few more, at least.
I guess, what limits this quest, is not only this "paved street", but even more the strong "designated" requirements. Maybe they are not always tagged correctly or so.

So just FYI, maybe the data is also already so good that most places have segregated already. 😉

@westnordost
Copy link
Member

I rather think this is because most of these ways are actually tagged something with ...=yes rather than designated. But well, I think we already discussed that the quest should only show on designated.

Though, perhaps it could also show on cycleway +foot=yes.

@westnordost
Copy link
Member

Also, another reason why few of it shows is that it is only shown for paved - so if the surface hasn't been tagged yet, the quest also doesn't appear, which is okay

@rugk
Copy link
Contributor

rugk commented Aug 16, 2018

Though, perhaps it could also show on cycleway +foot=yes.

I'd also say so, but OP said:

because (at least in my experience) highway=footway + bicycle=yes is always segregated=no

Which may be true, but I guess many things are not correctly tagged like this and pay attention to this difference. Maybe that's why the wiki says, "segregated" should always be tagged.

In any case, given the low results of the quest, I feel it would not hurt to decrease the limits anymore. An explicitly tagged "segregated" very certainly does not hurt anyone, but it may help, as it may not always be clear that "highway=footway + bicycle=yes" (or reverse) is this "Footway, but 🚲 are allowed".

@westnordost
Copy link
Member

westnordost commented Aug 16, 2018

I meant highway=cycleway + foot=yes, not highway=footway + bicycle=yes.

Edit: ...because the former doesn't really make sense, so the app could assume that what the person tagging this meant was actually foot=designated.

@matkoniecz matkoniecz deleted the segregated branch August 16, 2018 09:35
@rugk
Copy link
Contributor

rugk commented Aug 16, 2018

Ah, I get your point. Yes, it really makes no sense, or, at least, I have not seen it anywhere, yet.

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.

None yet

3 participants