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

special speed zone sign layout for many countries #3359

Merged
merged 37 commits into from
Nov 4, 2021

Conversation

matkoniecz
Copy link
Member

@matkoniecz matkoniecz commented Oct 10, 2021

changelog:

Speed limit quest - labels on speed limit zone sign are now country-specific (by @matkoniecz). Note that speed limit quest is disabled by default as it requires more involved survey, you can enable it in settings. This specific quest is near the bottom of the quest list.


fixes #2954

It has quite irritating layout duplication, but maybe it is safe to assume that it will be stable?

And alternative would be having layouts "zone text above sign", "zone text below sign" modified in various countries and it may not be sufficient anyway.

Progress (all are handled, ones with checkbox are also tested):

@westnordost
Copy link
Member

Looking at this https://commons.wikimedia.org/wiki/Category:Square_speed_limit_road_signs_with_circle , please do the following:

  1. Create one layout "zone text above sign", one layout "zone text below sign" and one layout "zone without text"
  2. create a string resource "Zone" to put as text there
  3. use the mcc codes to allocate each to each country, the default being "zone without text" (by including in the layout)
  4. nordic countries (yellow background) should already be taken care of

@westnordost westnordost marked this pull request as draft October 10, 2021 15:31
@matkoniecz
Copy link
Member Author

matkoniecz commented Oct 11, 2021

nordic countries (yellow background) should already be taken care of

I have bad news here! It is not working anymore. I spotted it after my mcc localization of untranslatable zone string failed to work.

I will try to locate what broke this, I remember that it used to work.

If my memory and understanding of code is right - traffic signs in Sweden should have yellow background.

1429ab2 is broken, v32.0 is broken, v31.0 compiled with google/dagger#1339 (comment) bugfix applied[0] and in Sweden traffic light are still white, v20.0 has not yet this implemented, v25.1 has white signs despite

cat "/home/mateusz/Documents/install_moje/OSM software/StreetComplete/app/src/main/res/values-mcc240/colors.xml"

giving

<?xml version="1.0" encoding="utf-8"?>
<resources>
    <color name="traffic_sign_background">@color/traffic_yellow</color>
</resources>

and https://en.wikipedia.org/wiki/Mobile_country_code listing Sweden as 240. I even set GPS position to Sweden just in case.

Just in case: https://user-images.githubusercontent.com/899988/136814332-38c87dca-0002-4c0f-a425-48c337c5cb86.png
screen02

(I will continue bisection).

[0]
to fix

error: package javax.annotation.processing does not exist

replace

    val daggerVersion = "2.14.1"

with

    val daggerVersion = "2.14.1"
    compileOnly("com.github.pengrad:jdk9-deps:1.0")

If this does not exist, look for implementation fileTree(dir: 'libs', include: ['*.jar']) and add below compileOnly 'com.github.pengrad:jdk9-deps:1.0'

Earlier it seemed that databinding would be suitable for parametrized layouts but...

android.databinding.tool.processing.ScopedException: [databinding] {"msg":"Found \u003clayout\u003e but data binding is not enabled.\n\nAdd buildFeatures.dataBinding \u003d true to your build.gradle to enable it."

@matkoniecz
Copy link
Member Author

matkoniecz commented Oct 11, 2021

@color/traffic_yellow is also not working in v24.0. Special height layout for foots/inches works fine.

@westnordost
Copy link
Member

so in maxspeed quest it is also not working?

@matkoniecz
Copy link
Member Author

@westnordost
Copy link
Member

Hmm, the inflator has a modified context (that sets the MCC to the current country) to inflate country-specific layouts, see AbstractQuestAnserFragment line 277ff.

However, it seems that that (modified) context is not used to resolve any resources within the country-specific layouts but another context - probably the activity context(?). In early 2019, I added this comment

    /** Note: Due to Android architecture limitations, a layout inflater based on this ContextWrapper
     * will not resolve any resources specified in the XML according to MCC  */

that mentioned this.

So maybe it would be possible to instead modify the activity context rather than the fragment context. This is probably the same hackery that needs to be done for the language-selection to work smoothly without a lot of code.

The other thing that should work is to use the fragment's modified context (getContext) to set the correct background drawables programmatically after inflating.

@matkoniecz
Copy link
Member Author

Or maybe get back to custom layouts without mcc-dependented resources?

I admit that when I looked at commit that introduced @color/traffic_yellow and replaced per-area layouts I was unsure is it a significant improvements given that it relies on bunch of magic to work. I assumed that I probably miss something and that my aversion exists because I avoid learning about new solution.

But given that magic is actually not working and requires even more magic to work...

I would consider reverting ec2529a as one more solution (I am not saying that it is preferable, but I would at least consider it as an option).

@westnordost
Copy link
Member

Just look at all the drawable-mcc*** resources, that would be a lot more work than to set the drawable manually in code after inflating, wouldn't you agree?

I mean, that the layouts are per mcc is also magic we rely on to work. If we were to abandon that, we'd need to get rif of all resources referring to MCCs but instead put this into code. A possibility of course, but again, this is a lot of work. And not sure what we gain with this.

@westnordost
Copy link
Member

Let's grant this bug an own issue and be this ticket only about the speed zone sign which should not be touched by this issue directly.

covers Italy (label on top)
covers North Macedonia (label on bottom)
covers Poland (no label)
label was not actually on top
@matkoniecz
Copy link
Member Author

matkoniecz commented Oct 18, 2021

@westnordost Thanks for handling this bug!


@matkoniecz matkoniecz changed the title special speed zone sign layout for Poland special speed zone sign layout for many countries Oct 18, 2021
@matkoniecz
Copy link
Member Author

If you see anything wrong in #3359 (comment) list - please comment.

If you know how zone speed sign look for one of missing countries (or you know that country is without it) - also please comment.

@Dimitar5555
Copy link
Contributor

Dimitar5555 commented Oct 18, 2021

Bulgaria doesn't have such signs since there is no definition in the traffic law.
The Greek sign says ΠΕΡΙΟΧΗ.
The Armenian sign should say ԹՈՏՒ.

Feel free to ping me if you get any Cyrillic signs that you can't decipher.

@matkoniecz
Copy link
Member Author

Thanks @Dimitar5555 - every part of that was very useful. Now just China is left :)

Feel free to ping me if you get any Cyrillic signs that you can't decipher.

Fortunately it seems that all are using the same text that I managed to obtain.

Bulgaria doesn't have such signs since there is no definition in the traffic law.

I will likely open a new issue for that...

@Dimitar5555
Copy link
Contributor

The Chinese sign is 区域 according to my OCR program, but I can't say if it's right.

@mnalis
Copy link
Member

mnalis commented Oct 19, 2021

If you see anything wrong in #3359 (comment) list - please comment.

I don't see Croatia in that list; we have text ZONA below sign:
https://hr.wikipedia.org/wiki/Prometni_znakovi#/media/Datoteka:Croatia_road_sign_C21.svg

@Kovoschiz
Copy link

Kovoschiz commented Oct 19, 2021

If you see anything wrong in #3359 (comment) list - please comment.

If you know how zone speed sign look for one of missing countries (or you know that country is without it) - also please comment.

Japan: https://commons.wikimedia.org/wiki/File:ZONE30-Sign.jpg bottom "区域 ここあら" (zone, begin)
This is the regulatory sign only. There are other informatory signs used by police of local governments which are not codified into written law schedules (as I understand), as well as road markings if you need it.

Ireland ???????????? https://en.wikipedia.org/wiki/Comparison_of_European_road_signs#Speed_limit

https://commons.wikimedia.org/wiki/File:Ireland_road_sign_F_403.svg is only used together with a wrong living_street-style designation (no pedestrian priority), pictogram at top, "crios mall slow zone" at bottom. But apparently there are some non-compliant examples with the order swapped, one at https://learnirishfromroadsigns.art.blog/2020/03/20/crios-mall-slow-zone/ .

@matkoniecz
Copy link
Member Author

@mnalis @Kovoschiz Thanks!

https://commons.wikimedia.org/wiki/File:Ireland_road_sign_F_403.svg is only used together with a wrong living_street-style designation (no pedestrian priority), pictogram at top, "crios mall slow zone" at bottom. But apparently there are some non-compliant examples with the order swapped, one at https://learnirishfromroadsigns.art.blog/2020/03/20/crios-mall-slow-zone/ .

As slow zone is handled separately, than means that Ireland de facto has no zone speed limits.

Also, it will be worth checking is there already support for yellow background on living zone traffic sign.


Note: I will likely keep this list for some time when I have time but are for some reason unable to handle something more complex as it is mostly drudgery with translating country names to https://en.wikipedia.org/wiki/Mobile_country_code and creating bunch of layouts and texts, like in https://github.com/streetcomplete/StreetComplete/blob/caad4bfbc1378ee813f626a467a12d189bf79cc5/app/src/main/res/values-mcc294/strings.xml https://github.com/streetcomplete/StreetComplete/blob/caad4bfbc1378ee813f626a467a12d189bf79cc5/app/src/main/res/layout-mcc294/quest_maxspeed_zone_sign.xml

@riiga
Copy link

riiga commented Oct 19, 2021

@matkoniecz

Sweden (no label, yellow background - so just testing required) https://commons.wikimedia.org/wiki/File:1_2_77_3_(Swedish_road_sign).svg
Sweden - has no zone signs https://en.wikipedia.org/wiki/Comparison_of_European_road_signs#Speed_limit

Sweden has a sign for speed zones, but doesn't use it. I've never seen any other zones apart from no parking/no stopping zones and the occasional parking zone. We don't have any signs for default speed limits either, all speed limits are signed with a regular speed limit sign. The exception are recommended lower speed limits which use an end sign.

Denmark Zone at bottom https://commons.wikimedia.org/wiki/File:Denmark_road_sign_E68.4.svg
Denmark - ????????????????????????? https://commons.wikimedia.org/wiki/File:Denmark_road_sign_E53.svg very suspicious! https://en.wikipedia.org/wiki/Road_signs_in_Denmark - argh.

Denmark has two different signs for speed zones. The blue one is for areas where there is traffic calming and the the speed is generally 30 or 40 km/h (20-45 allowed). The regular one is for what you'd expect.

@westnordost
Copy link
Member

westnordost commented Oct 19, 2021

Alternatively you can put the strings in the code as well and set the text programmatically which is one line of code. Maybe that would be both less effort and is much easier to read. E.g.

private fun getZoneText(countryCode: String) = when(countryCode) {
    "DE" -> "ZONE"
    "HR" -> "ZÓNA"
    ...etc
}

Even doing that for the layout itself may be less effort than creating XML resources for dozens of countries.

@FloEdelmann
Copy link
Member

Or maybe a file maxSpeedZone.yml in res/country_metadata/?

default: false # no speed limit zones known by default
DE: "ZONE"
HT: "ZÓNA"

@westnordost
Copy link
Member

westnordost commented Oct 19, 2021

Right, also possible, this is what the country metadata is for after all. Then e.g.

slowZoneLabelLocation.yml

DE: below
HR: below
PL: none

could also be done

@matkoniecz
Copy link
Member Author

matkoniecz commented Oct 19, 2021

What about:

speedZoneLabel.yml

default: none
DE: {text: ZONE, position: below}

@matkoniecz
Copy link
Member Author

Proposed changelog entry (mirrored in top post):

Speed limit quest - labels on speed limit zone sign are now country-specific (by @matkoniecz). Note that speed limit quest is disabled by default as it requires more involved survey, you can enable it in settings. This specific quest is near the bottom of the quest list.

@matkoniecz
Copy link
Member Author

matkoniecz commented Oct 28, 2021

Is there anyone from Ireland here or familiar with signs there?

I am confused a bit - are there valid highway=living_street there? Right now SC is not offering living street as a selection there and I found https://en.wikipedia.org/wiki/File:Ireland_road_sign_F_403.svg

This will likely not be done as part of this PR but I added it on my TODO pile.

@matkoniecz matkoniecz marked this pull request as ready for review October 28, 2021 08:57
Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

The code looks fine, but I haven't tested this in action. EDIT: Works and looks fine in Germany.

@matkoniecz
Copy link
Member Author

matkoniecz commented Oct 28, 2021

If anyone would test in specific country: please mention it here so I can mark proper checkbox in the initial post (or mark them directly if you can do this)

@westnordost
Copy link
Member

What exactly is missing here? Is it ready for review&merge? In the list it shows up as 16/46 tasks finished.

@matkoniecz
Copy link
Member Author

It is finished and ready. Checkboxes mark where it was tested, what - I think - covered various variations.

Though if you think that it would be useful I can also test in remaining 30 locations.

@westnordost
Copy link
Member

Though if you think that it would be useful I can also test in remaining 30 locations.

Nah that's fine

@westnordost
Copy link
Member

Since no new strings were added and you tested this well, this might as well go into a minor release (in case there'll be one), so we can merge it now

@westnordost westnordost merged commit afa205a into streetcomplete:master Nov 4, 2021
@matkoniecz matkoniecz deleted the poland_zone branch November 4, 2021 21:32
@matkoniecz
Copy link
Member Author

@westnordost if you want you can use following as a changelog entry

Speed limit quest - labels on speed limit zone sign are now country-specific (by @matkoniecz). Note that speed limit quest is disabled by default as it requires more involved survey, you can enable it in settings. This specific quest is near the bottom of the quest list.

(I can also add it directly if you think that it is fine)

@smichel17
Copy link
Member

smichel17 commented Nov 5, 2021

Cleaned up:

Speed limit quest - labels on speed limit zone sign are now country-specific (by @matkoniecz). Note: the speed limit quest is disabled by default as it requires more involved survey. You can enable it in settings, near the bottom of the quest list.

I think technically "it requires more involved survey" should be either "…a more involved survey" or "…more involved surveying", but for changelog purposes it's fine.

@westnordost
Copy link
Member

Regarding updates on the changelog:

  • I'd rather completely do it myself
  • OR if someone else updates the changelog, that person should add all the things (relevant for users) that have been added since the last time the changelog was updated.

To keep the overview till which commit the changelog was maintained, you know.

In general, the changelog entries should be very short / simplified. For more information and notes, there is always the link to the actual ticket. So for this PR, I'd rather add someting like "display speed limit zone signs as they appear in your country in the form" or similar.

@westnordost
Copy link
Member

Regarding updates on the changelog:

Any anyone with write acces can do this, without a PR, if he follows these rules. I wanted to put this into the "working together efficiently" post in the discussion forum but then again I thought this is not going to be a big effort for me as I need to go through the commit history anyway before each release again to see if hte changelog is up-to-date.

@Dimitar5555
Copy link
Contributor

Bulgaria doesn't have such signs since there is no definition in the traffic law.

A few months ago this sign was introduced. It has "ЗОНА" on the top side.

image

matkoniecz added a commit to matkoniecz/countrymetadata that referenced this pull request Jul 13, 2022
@matkoniecz
Copy link
Member Author

matkoniecz commented Jul 13, 2022

Thanks!

done in streetcomplete/countrymetadata#8

in general, for such thing it is better to open new issue - closed PRs and issues can be no longer watched, and if not processed immediately it may be forgotten


Now once SC on new release gets new countrymetadata data, then Bulgaria should get slow zone tagging option.

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.

Maxspeed zone sign has no "zone" label in Poland, mismatches with actual signs in other places
8 participants