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.

Sign up for GitHub

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

Jump to bottom

Simplify routing endpoint interface, always do reverse geocoding #5064

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Simplify routing endpoint interface, always do reverse geocoding #5064

wants to merge 7 commits into from

Conversation

AntonKhorev
Copy link
Collaborator

@AntonKhorev AntonKhorev commented Aug 11, 2024 •
edited
Loading

Now you only have to call endpoint.setValue with a single parameter. The parameter is checked if it's coordinates. If not, geocoding is performed. If yes, reverse geocoding is done. Reverse geocoding is also done on marker drag end.

I did what I described in #5062 (comment). The coordinates are saved in data attributes on successful geocode, where "Directions from/to here" context menu can read them. This should fix #1874 and #2982.

Also addresses #1910 to some extent. But if we want to get back "Paris" from the coordinates and not some address inside Paris, we'd need to pass along a zoom level. Nominatim supposedly can use it. Or pass search queries along. Presumably that wasn't done to keep urls short, but we can pass only short queries for example.

Also should fix #1911 if the problem is markers moving from clicked locations to geocoding locations. In this case it's the step 3 of #2982 (comment).

May close #1754 because everything was rewritten.

@AntonKhorev AntonKhorev marked this pull request as ready for review August 12, 2024 04:48
@AntonKhorev AntonKhorev changed the title Simplify routing endpoint interface Simplify routing endpoint interface, always do reverse geocoding Aug 12, 2024
@AntonKhorev AntonKhorev mentioned this pull request Aug 12, 2024
Open
@nenad-vujicic
Copy link
Contributor

Thanks for great PR! The sources are now much more readable and it seems a lot of bugs are fixed.

Just a few remarks:

  1. Perhaps it would be good to fit map's bounds (directions.js Ln 159) on union of markers and route polyline (instead of route polyline only). 2 cases when this is useful:
    a) at least one (or both) of markers are too far from retrieved route and they are not visible in final display and
    b) routing engine was unable to return the route (e.g. place markers at (0,0) and (1,1)) and only markers are displayed (but, if you type them, map didn't focus on them).

  2. It seems we still have duplicated calls to nominatim when we select from context menu (other cases are OK). Please, take a look at below video:

Screen.Recording.2024-08-13.125247.mp4

Anyway, even with above remarks, I think this is great PR. Thanks for making it!

@nenad-vujicic
Copy link
Contributor

One more thing, excuse me for missing it in above post:

  1. It seems is-invalid is not removed from inputs after successful contacting nominatim:
Recording.2024-08-13.132359.mp4

Thanks!

@AntonKhorev
Copy link
Collaborator Author

Perhaps it would be good to fit map's bounds (directions.js Ln 159) on union of markers and route polyline

Yes it would but not in this pull request. This is something that the code before this PR also doesn't do.

@AntonKhorev
Copy link
Collaborator Author

It seems is-invalid is not removed from inputs after successful contacting nominatim

They aren't removed in the original code either. The difference is it doesn't call nominatim, but it should have removed is-invalid as soon as the coordinates updated.

@AntonKhorev AntonKhorev mentioned this pull request Aug 14, 2024
Merged
@AntonKhorev AntonKhorev mentioned this pull request Aug 15, 2024
Merged
@AntonKhorev AntonKhorev mentioned this pull request Aug 16, 2024
Merged
This was referenced Aug 16, 2024
Merged
Draft
@AntonKhorev AntonKhorev mentioned this pull request Aug 19, 2024
Closed
@AntonKhorev AntonKhorev force-pushed the endpoint-module-2 branch 3 times, most recently from 69664f7 to 9528487 Compare August 20, 2024 09:44
@AntonKhorev AntonKhorev mentioned this pull request Aug 20, 2024
Merged
@AntonKhorev AntonKhorev force-pushed the endpoint-module-2 branch 2 times, most recently from c2c4952 to b08845c Compare August 20, 2024 12:01
@AntonKhorev AntonKhorev mentioned this pull request Aug 20, 2024
Closed
@AntonKhorev AntonKhorev force-pushed the endpoint-module-2 branch 2 times, most recently from 6d97492 to 1ae52b8 Compare August 21, 2024 08:49
@AntonKhorev AntonKhorev mentioned this pull request Aug 21, 2024
Merged
@AntonKhorev
Copy link
Collaborator Author

AntonKhorev commented Aug 21, 2024 •
edited
Loading

"2." form #5064 (comment) - I only see one request to Nominatim happening when "Directions from/to here" is selected. I fixed something like this in 2d1cf4c but wasn't it written before the comment?

@nenad-vujicic
Copy link
Contributor

"2." form #5064 (comment) - I only see one request to Nominatim happening when "Directions from/to here" is selected. I fixed something like this in 2d1cf4c but wasn't it written before the comment?

Recording.2024-08-21.120546.mp4

If one point is valid and you change the second one, yes, there is only one request from nominatim. But, if one point is invalid and you change the second one, it will request from nominatim 2 times (once for each point).

I used the latest sources at the moment on which I applied this PR.

Thanks!

@AntonKhorev
Copy link
Collaborator Author

@nenad-vujicic how about now? I added caching of not found results from Nominatim.

@nenad-vujicic
Copy link
Contributor

@nenad-vujicic how about now? I added caching of not found results from Nominatim.

It's perfect now. Thank you very much!

There is one another issue, but probably not part of this PR. In some situations (take a look at attached video), on invalid search scene map fits to some other boundary box and route is displayed on wrong position. After pushing OK, it retrieves good route, displays it on good position and fits to good boundary box.

Recording.2024-08-21.143742.mp4

@AntonKhorev
Copy link
Collaborator Author

In some situations (take a look at attached video), on invalid search scene map fits to some other boundary box and route is displayed on wrong position

Could be caused by alert(). I'll later remove them.

@AntonKhorev
Copy link
Collaborator Author

I guess this can be merged now. I don't think there's anything else worth taking out and merging separately.

tomhughes
tomhughes reviewed Sep 6, 2024
View reviewed changes
Copy link
Member

@tomhughes tomhughes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I agree with either part of the title of this PR as I don't really see any simplification - quite the opposite if anything. The "always do reverse geocoding" is also misleading because it implies extending an existing use when in fact this introduces reverse geocoding for the first time?

app/assets/javascripts/index/directions-endpoint.js Outdated Show resolved Hide resolved
app/assets/javascripts/index/directions-endpoint.js
input.val(json[0].display_name);

changeCallback();
});
}

function getReverseGeocode() {
var latlng = endpoint.latlng.clone();
var reverseGeocodeUrl = OSM.NOMINATIM_URL + "reverse?lat=" + latlng.lat + "&lon=" + latlng.lng + "&format=json";
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to control the match precision here? Might this cause a location to "snap" to something a long way away?

Copy link
Collaborator Author

@AntonKhorev AntonKhorev Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The location wouldn't snap in the same way is does on the currently deployed version. The pointer is not going to move, just the address would be too imprecise. Or too precise in an unhelpful way, for example when you zoom out to see a country and click on a city and you're told that you clicked on a house with this particular address.

Ultimately we should control the precision but I didn't want to do it in this pull request, see #4904 (comment)

@AntonKhorev
Copy link
Collaborator Author

in fact this introduces reverse geocoding for the first time?

Reverse geocoding happens in the deployed version when Nominatim decides to do it. I don't know if it's intentional or accidental.

  • context menu, directions from/to here - reverse geocoding
  • drag/drop a marker - no reverse geocoding
  • type in coordinates - reverse geocoding if you typed two numbers with decimal points (or what's the logic for detecting coordinates in Nominatim), no reverse geocoding otherwise

@AntonKhorev
Copy link
Collaborator Author

I'm not sure I agree with either part of the title of this PR as I don't really see any simplification - quite the opposite if anything.

A large part of this pull request is already merged with other pull request. The simplification here is that you don't have to pass latlng to endpoint.setValue.

@tomhughes
Copy link
Member

A large part of this pull request is already merged with other pull request

Can we rebase it then so we can see what's left?

@AntonKhorev
Copy link
Collaborator Author

It is rebased. I mean I haven't updated the title since before any of those other pull requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomhughes tomhughes tomhughes left review comments

@nenad-vujicic nenad-vujicic nenad-vujicic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@AntonKhorev @nenad-vujicic @tomhughes