Add ImproveOSM Q/A Layer

[kymckay]

Ref #5683

This PR is still WIP, but opened for progress tracking and collaborative purposes.

My current priorities:

• Query all error types
• Render errors in viewport
  • Add an appropriate icon
• Display error details to user
• Implement error resolving (mark as false/fixed)
• Implement error commenting (appears to be non-functional)
• Cleanup and feedback incorporation

There's a lot of similar and duplicate code to the KeepRight implementation that could probably be made more general and shared between the two (and future services). I could probably do this too, but would rather get this working first then worry about that in a later PR.

[kymckay]

Doing some more work on this today and it actually seems like the ImproveOSM comment system doesn't work. Can anyone else try it on their site and confirm this?

[bhousel]

Thanks @SilentSpike ! This looks pretty great.. FYI we are preparing an iD release for this week, so it won't be in January, but I'd like to try to get it into the February release.

[kymckay]

All critical functionality is in, really just needs implementation cleanup and decisions (i.e. do we want all future QA layers to have consistent UX or each have their own icons/behaviour).

For the time being (after trying various new icons) I've made these errors render the same as points with icons (but coloured). From my latest commit message:

This allows more diverse representation of the error subject at a glance than relying on colour alone.

However, it would be good to have a generic error icon that can contain icons which is differentiated from the point icon for clarity.

Sidebar header currently still uses the bolt icon until I figure out how best to deal with that. Also the font awesome icons don't seem to work, perhaps there's additional code needed for those that I've missed.

Here's a preview of what that looks like.

[kymckay]

Went ahead and introduced a new containing icon as per my previous commit. Here it is next to a point icon for reference. Also got it working in the sidebar header and learned quite a bit about working with svg elements.

[bhousel]

All critical functionality is in, really just needs implementation cleanup and decisions (i.e. do we want all future QA layers to have consistent UX or each have their own icons/behaviour).

Awesome @SilentSpike , looking forward to trying this out. I think it's ok for the different Q/A layers to have their own icons for now. We probably won't add more than a handful. We might be able to generalize what the sidebar does.

[kymckay]

@bhousel I think different icons are actually useful to see which service is being interacted with.

My main line of thinking with the more generic icon + detail icon implemented here is geared towards future Osmose support. It's similar to this service in that there's no distinguishable icon to use, but unlike this there are many more error types that could potentially be supported in future to differentiate between.

In terms of the current status of this, the current desirable list in my head ordered by ratio of significance/ease:

• I should probably bump turn restriction error positions slightly so they're not right on the junction node. An easy QOL improvement.
• Review UI strings
  • Could add identified start/end nodes to one-way errors
  • Decide if happy with wording and information given
  • Need to update the help string that explains QA layers to reflect this implementation
• Believe I may have unsafely used the userDetails function to get OSM username (it's also possible this isn't actually needed to post updates to the errors and because commenting doesn't work might be worth checking that).
• Decide whether we want to track the errors closed using their external identifiers (they're based on unique features of each error, not very pretty readable numbers like KeepRight).
• More appropriate error positioning for oneway errors, may be beyond my current ability. Currently just takes the average of the start and end of the way segment in question as the API only serves these lat/lon. This works fine for relatively straight segments, but if the segment is curved it can become far off the way.
• Decide if we want to add links to the error on ImproveOSM (no way that I see to link directly to it, but we could link to the location or something).

Beyond that:
In future I could see all services defining their errors as children of a base error class which has the properties common to all (position, name, description, category, subcategory, icon, external ID).

In this way a lot of the code could be made general to work with these common values, then there could be some cross-service consistency in things like colouring (based on category) to aid workflow (e.g. if someone wanted to investigate all navigation related errors they'd just look for a certain colour).

[bhousel]

I played around with this a bunch today.. I think it's very nearly ready to merge 🎉

My main line of thinking with the more generic icon + detail icon implemented here is geared towards future Osmose support.

I agree, this is a good approach..

• Review UI strings - Decide if happy with wording and information given

They look pretty good to me.. What do you think about calling it an ImproveOSM "Issue" or "Detection" instead of "Error".. Possibly also include some way to show the user the confidence score. There are a lot of false positives in this dataset and I don't want to give users the impression that these are all real errors that need fixing.

• Believe I may have unsafely used the userDetails function to get OSM username (it's also possible this isn't actually needed to post updates to the errors and because commenting doesn't work might be worth checking that).

Yes, I tried this on the main ImproveOSM site and I couldn't get it to work there either. It's ok to leave the commenting feature out for now.

• Decide whether we want to track the errors closed using their external identifiers (they're based on unique features of each error, not very pretty readable numbers like KeepRight).

Ah yeah I noticed this too. It might be best to just invent an id string out of the loc of the error, like "-74.00765,40.96369" (truncate to 5 decimals?). Though if users close a lot of issues we'll overrun the 255 char limit in the changeset tag. Maybe this is a good use for open location codes? They'd be a slightly shorter than coordinate pairs.

• More appropriate error positioning for oneway errors, may be beyond my current ability. Currently just takes the average of the start and end of the way segment in question as the API only serves these lat/lon. This works fine for relatively straight segments, but if the segment is curved it can become far off the way.

What you did looks pretty good for now! I was thinking of drawing some shape or arrows over the whole way (for missing oneways) or over the from/via/to (for missing turn restrictions) like on the ImproveOSM site, but we don't have to.

• Decide if we want to add links to the error on ImproveOSM (no way that I see to link directly to it, but we could link to the location or something).

Yeah without true stable ids, I don't see a good way to do it. We can skip this part.

Beyond that:
In future I could see all services defining their errors as children of a base error class which has the properties common to all (position, name, description, category, subcategory, icon, external ID).

yes!

In this way a lot of the code could be made general to work with these common values, then there could be some cross-service consistency in things like colouring (based on category) to aid workflow (e.g. if someone wanted to investigate all navigation related errors they'd just look for a certain colour).

Yes, this would be great to have some consistency. FWIW, I did put a bunch of thought into the KeepRight colors. I probably spent a few hours adjusting them so that they

• have enough contrast with each other and with map features
• reduce the overall number of unique colors by grouping similar kinds of errors
• chose colors based on how frequently the issues/markers occur on the map

Anyway, great work! I'm happy to merge it whenever you feel that it's finished.

[kymckay]

They look pretty good to me.. What do you think about calling it an ImproveOSM "Issue" or "Detection" instead of "Error".

Yes this is a good idea, something I've had in the back of my mind for a while now.

Possibly also include some way to show the user the confidence score. There are a lot of false positives in this dataset and I don't want to give users the impression that these are all real errors that need fixing.

The potential for false positives is also something I've been considering how best to convey. Unfortunately the detections shown here are only the most confident cases (precisely to reduce the amount of false positives we're exposing to potentially new mappers). This is why I've included the data on number of trips used to make the detection.

However, another thought I had was that we could have a note in the sidebar explaining that false positives are likely and the detection should be confirmed via imagery or on the ground. Beyond that (and beyond ImproveOSM) we could provide information on how to solve different QA "error" types, which for these could include mentioning first determining if it's a false positive due to their likelihood.

It might be best to just invent an id string out of the loc of the error, like "-74.00765,40.96369" (truncate to 5 decimals?). Though if users close a lot of issues we'll overrun the 255 char limit in the changeset tag. Maybe this is a good use for open location codes? They'd be a slightly shorter than coordinate pairs.

Will investigate this!

What you did looks pretty good for now! I was thinking of drawing some shape or arrows over the whole way (for missing oneways) or over the from/via/to (for missing turn restrictions) like on the ImproveOSM site, but we don't have to.

Yeah I realised the one-way data served was more useful than I thought so it should no longer have the curved segments issue. Considered using a more visual overlay, but decided to keep it simple for now as I worry that it'd clutter things too much (already moving these point icons not to block OSM features). Something to investigate once this is merged.

[kymckay]

Rebased to master and squashed a few trivial commits in the process.

In terms of functionality I'm happy with what's here. The code itself can probably be improved/cleaner in places, but I figure let's get this into the master branch since it's working and improve from there.

I haven't used open location codes here as I didn't want to introduce a new dependency just for this, but perhaps worth using if they'll also be used elsewhere.

You'll probably want to review e9397aa in particular before merging as it's the first time I've had to properly consider how scope and callbacks work in javascript.

[bhousel]

In terms of functionality I'm happy with what's here. The code itself can probably be improved/cleaner in places, but I figure let's get this into the master branch since it's working and improve from there.

Yes, I agree.. I think it looks great (and PRs get harder to merge the longer they sit).

I haven't used open location codes here as I didn't want to introduce a new dependency just for this, but perhaps worth using if they'll also be used elsewhere.

Sounds good. I might make an attempt at it this weekend...

You'll probably want to review e9397aa in particular before merging as it's the first time I've had to properly consider how scope and callbacks work in javascript.

You did great 👍 iD is a fantastic project to learn on. Also, ping me on Slack anytime if you ever have any questions, or are looking for more to do! 😄

[quincylvania]

Looks awesome, @SilentSpike, thanks so much for contributing in a big way like this!

[mvexel]

@SilentSpike Do you still need someone to investigate the comments on the ImproveOSM side?

[kymckay]

@mvexel Do you mean to fix the functionality on the ImproveOSM end?

We've confirmed that they don't seem to be functional, but if that is updated I'll happily add in the support on this end.

[tBeata-grab]

The comment functionality should work for the ImproveOSM layers, at least in the ImproveOSM plugin works as expected.
You can add a comment with an optional status change to a list of ImproveOSM entities by using the "comment" operation from the ImproveOSM API's.

Example of comment operation request body:
Missing Roads:
{
"username": "test",
"text": "There is no road there",
"status": "INVALID",
"targetIds":
[
{
"x": 100,
"y": 100
},
{
"x": 100,
"y": 101
}
]
}

One ways (Direction of flow)
{
"username": "zero",
"text": "There is no road there",
"status": "INVALID",
"targetIds":
[
{
"wayId": 100,
"fromNodeId": 100100,
"toNodeId": 100200
},
{
"wayId": 100,
"fromNodeId": 100200,
"toNodeId": 100300
}
]
}

Turn restrictions
{
"username": "zero",
"text": "There is no road there",
"status": "INVALID",
"targetIds":
[
"abc", "def"
]
}

The comments of an ImproveOSM element can be accessed by using the "retrieveComments" operation.
Example of retrieveComments requests:
Missing Roads :
http://missingroads.skobbler.net/missingGeoService/retrieveComments?tileX=66243&tileY=119729

**Turn restrictions ** :
http://turnrestrictionservice.skobbler.net/turnRestrictionService/retrieveComments?targetId=40879459%2C2497705373%2C62660208%3A8755171%2C62660208%2C2497705372%2B162%23193%230

Let me know if you need further assistance with the ImproveOSM API's.

[kymckay]

@beataj-telenav Awesome thank you! I was only going by the requests I observed being sent by the improveosm.org editor so was totally unaware of the retrieveComments operation (and posting new comments weren't showing up there).

With this information I'll see if I can get commenting working here in iD 😃

[tBeata-grab]

@SilentSpike : I forgot to add a retrieveComments example for the One-way (Direction of flow) component. In this case the request would be: https://directionofflow.skobbler.net/directionOfFlowService/retrieveComments?wayId=229550180&fromNodeId=151993197&toNodeId=2381277816.
Let me know if you have any further questions regarding the ImproveOSM API's or need any specification (parameters, status codes, available filters etc). We don't have any public documentation published for these components, but I can provide the necessary documentation for integration if needed.

Btw: great work with the integration, I'm glad that our components will be integrated also to the main iD version.

[kymckay]

@beataj-telenav Managed to suss those out myself 😉

I have working comments now, but one question for you: the timestamp served, what format is it in (as you can see it came out as the wrong date in the above screenshot)?

[tBeata-grab]

@SilentSpike The timestamp is the number of seconds elapsed since 1970-01-01 00:00:00 UTC, not counting leap seconds. In order to display the correct value, you need to format it based on the current timezone. (In the JOSM plugin we first transform the value into milliseconds and then format based on the user timezone).

[tBeata-grab]

@SilentSpike For the missing roads data in some cases, you might get -1 for the numberOfTrips field. This happens for data that came from a 3rd party. In the JOSM plugin for these cases, we display "not available".