Fixed displacing points specified from context menu #4904

nenad-vujicic · 2024-06-17T12:29:24Z

This PR addresses "Directions from here often places the point hundreds of meters elsewhere" issue mentioned in #2982

Fixes #2982 by updating directions.js so it doesn't perform unnecessary geocoding anymore for previously selected point (either source or destination), when entering second point. Also, improved setting markers by positioning it to selected position (not to result of geocoding).

tomhughes · 2024-06-17T12:40:45Z

How does it "address" that issue? What is the change you are making to address it?

nenad-vujicic · 2024-06-18T21:41:15Z

Dear @tomhughes ,

Thank you for your review and comment. I updated PR and commit messages. Please, let me know if I can improve it further.

Thanks,
Nenad.

AntonKhorev · 2024-06-24T13:39:16Z

so it doesn't perform unnecessary geocoding anymore for the previously selected point (either source or destination), when entering second point

Doesn't it still do unnecessary geocoding for the first point? And then its marker may get shifted as a result of this geocoding.

What an endpoint should do is to detect that it has coordinates as a value and don't try to query nominatim if it does. Search has server-side regexps to detect coordinates:

openstreetmap-website/app/controllers/geocoder_controller.rb

Line 196 in 7cd900e

    
           if latlon = query.match(/^([NS])\s*(\d{1,3}(\.\d*)?)\W*([EW])\s*(\d{1,3}(\.\d*)?)$/).try(:captures) || # [NSEW] decimal degrees

Here something simpler might suffice like number, number format without all those degree signs, N/S/E/W letters etc.

AntonKhorev · 2024-06-25T01:35:32Z

related #1754

nenad-vujicic · 2024-07-02T13:26:39Z

Dear @AntonKhorev ,

I've just updated PR with the latest, simplified and corrected sources. I made short video displaying new functionality (I added debugging alert("GEOCODING") call to directions.js just before $.getJSON(OSM.NOMINATIM_URL .. line to catch geocoding requests).

Screen.Recording.2024-07-02.144007.mp4

Now, geocoding is called only once when user selects "Directions from here" or "Directions to there" or when new address is selected. Another way is to call geocoding only when address is typed, but, then, address will be lost when context menu options are selected (like when dragging markers, i.e. in From / To fields will be drawn Lat / Lng only).

Please, let me know which one option is ideal or if it could be further improved?

Thanks,
Nenad.

AntonKhorev · 2024-07-04T03:41:23Z

app/assets/javascripts/index/directions.js

-      var precision = OSM.zoomPrecision(map.getZoom());
-      input.val(ll.lat.toFixed(precision) + ", " + ll.lng.toFixed(precision));


This code was used by $("#map").on("drop" ... to populate an input when the marker next to the input is dragged and dropped on the map.

What happens now:
Screencast from 04.07.2024 06:35:02.webm

What used to happen:
Screencast from 04.07.2024 06:38:26.webm

nenad-vujicic · 2024-07-04T08:16:51Z

Dear @AntonKhorev ,

Good catch, thank you very much! I've just fixed it and now it works fine! Please, let me know if I could further improve it.

Thanks,
Nenad.

AntonKhorev · 2024-07-06T10:45:17Z

Going to Nominatim with coordinates still happens. Try entering "0,0" into from/to inputs.

Before the changes the code always went to Nominatim and searched for these numbers, found them somewhere in Taiwan (or elsewhere depending on the current map view) and placed the marker there.

After the changes the same happens except the marker is at 0, 0.

We should

either decide that we're not searching for input values when they match the coordinate format,
or never populate inputs with coordinates (and maybe populate them with reverse geocoding results).

nenad-vujicic · 2024-07-08T21:23:37Z

Dear @AntonKhorev ,

Thanks for reviewing and comments. Now, we are contacting nominatim only if: point is defined using context menu (right click + Directions from/to here) or something is entered that doesn't look like coordinates (Number "," or "/" Number). Also, we improved focusing of map view, so it focuses on union of markers and path (if exists), previously it focused only on path.

Please, let me know if we could further improve it.

Thanks,
Nenad.

AntonKhorev · 2024-07-18T14:47:05Z

Another bug:

open the directions panel
select directions from here from the context menu

simplescreenrecorder-2024-07-18_17.44.10.mp4

nenad-vujicic · 2024-07-19T10:41:58Z

Dear @AntonKhorev ,

Thank you for catching this! I've just fixed it. Please, let me know if I could further improve it.

Thanks,
Nenad.

AntonKhorev · 2024-07-20T14:50:41Z

app/assets/javascripts/index/directions.js

    });

-    endpoint.setValue = function (value, latlng) {
+    endpoint.setValue = function (value, force_reverse_geocoding, latlng) {


What is force_reverse_geocoding? Is there any code here that actually does reverse geocoding?

Yes, there is. Reverse geocoding is done when user right-clicks on map and selects "Directions from here" or "Directions to here".

Variable force_reverse_geocoding is introduced to pass information that user did this (right clicked and selected from context menu) and to perform reverse geocoding.

There's no forcing of reverse geocoding. There's no code calling reverse like here. Nominatim will usually do reverse geocoding when "Directions from/to here" are selected, but that's because it decides on its own that the input query looks like coordinates. It won't make this decision correctly every time.

simplescreenrecorder-2024-07-22_01.46.00.mp4

Thanks for catching this! I've just updated with adding code for calling "reverse" like in above link.

Thanks for catching this! I've just updated with adding code for calling "reverse" like in above link.

We have just gotten another idea how to improve this part. I'll make another push soon.

@AntonKhorev I've just pushed the latest fixes.

We improved logic of what is displayed in input boxes after right-clicking and selecting option from context menu. Now, if nominatim doesn't return anything or it returns something that is more than 200m away from clicked position, we will display coordinates (Number, Number), otherwise we will display what nominatim returned. The logic behind is "in input boxes should be displayed user-friendly text iff user entered it manually or user clicked on / or near the object recognized by nominatim, otherwise, coordinates will be displayed".

For calculating distance between position where user clicked and position of what nominatim returned, we used distanceTo method provided by Leaflet's LatLng which reduces it to calculating distance on great circle. After testing on Longyearbyen and Longyearbyen, it seems this approximation works fine.

Please, let me know if we can further improve it.

Thanks,
Nenad.

AntonKhorev · 2024-07-20T14:50:57Z

app/assets/javascripts/index/directions.js

@@ -402,8 +440,10 @@ OSM.Directions = function (map) {
      }
    }

-    endpoints[0].setValue(params.from || "", from);
-    endpoints[1].setValue(params.to || "", to);
+    var ll_from_search_form = decodeURIComponent(location.search).match(/^\?from=([+-]?\d+(\.\d*)?)(?:\s+|\s*[/,]\s*)([+-]?\d+(\.\d*)?)$/);


Does this regexp ever match anything?

"Directions from here" generate location.search like ?from=52.54124,13.56290&to= which doesn't match. Do you want it to match?

Yes, it does. It captures the situation when user inputs some coordinates into search field and then pushes "Find directions between two points" button. Earlier, this performed reverse geocoding, now it doesn't do it anymore.

Our idea behind the current solution is "if user enters the coordinates or drags markers, leave the coordinates. If user right-clicks and selects from context menu, perform reverse geocoding".

nertc · 2024-08-06T09:48:37Z

When user tries to add a marker out of the borders of countries, alert is thrown "Sorry - couldn't locate", but marker is still added and if user drags it to the valid position, route is not updated (see Video 1).

Video 1:

OpenStreetMap.and.45.more.pages.-.Work.-.Microsoft.Edge.2024-08-06.13-37-54.mp4

Contrary to this, if user adds marker in the valid location and then drags it out of the borders, route is still calculated and shown without any alerts (see Video 2).

Video 2:

OpenStreetMap.and.45.more.pages.-.Work.-.Microsoft.Edge.2024-08-06.13-38-13.mp4

Maybe, in the first case, we should still show coordinates and calculate route even though Nominatim location was not found (as it is done in the second case).

nertc · 2024-08-06T09:55:59Z

The same logic is with "Couldn't find a route between those two places" error. If it is thrown, dragging location pin doesn't update route (see Video).

Video:

OpenStreetMap.and.45.more.pages.-.Work.-.Microsoft.Edge.2024-08-06.13-53-27.mp4

nertc · 2024-08-06T11:08:32Z

I know that we are using var everywhere, but I think, it would be great if we stick to let and const, where using var is not absolute necessity. let and const are less vague about scopes and much bug-proof. There are some examples Stackoverflow link. These are the lines you used "var" on:

Line 129: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR129

Line 179: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR179

Line 301: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR301

Line 316: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR316

Line 456: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR456

Line 469: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR469

nertc · 2024-08-06T11:10:57Z

app/assets/javascripts/index/directions.js

-          alert(I18n.t("javascripts.directions.errors.no_place", { place: endpoint.value }));
-          return;
-        }
+      if (endpoint.value.match(/^([+-]?\d+(\.\d*)?)(?:\s+|\s*[/,]\s*)([+-]?\d+(\.\d*)?)$/)) {


As we reuse /^([+-]?\d+(\.\d*)?)(?:\s+|\s*[/,]\s*)([+-]?\d+(\.\d*)?)$/ regex, it would be more understandable if we create separate constant variable for it and then use it with the variable name.

nertc · 2024-08-06T11:19:32Z

app/assets/javascripts/index/directions.js

+          geocodeCallback();
+        });
+      } else {
+        $.getJSON(OSM.NOMINATIM_URL + "search?q=" + encodeURIComponent(endpoint.value) + "&format=json&viewbox=" + viewbox, function (json) {


When concatenating many strings, I think, using Template literals are more suitable and easier to read way.

nertc · 2024-08-06T11:20:50Z

app/assets/javascripts/index/directions.js

+        $.getJSON(OSM.NOMINATIM_URL + "search?q=" + encodeURIComponent(endpoint.value) + "&format=json&viewbox=" + viewbox, function (json) {
+          endpoint.awaitingGeocode = false;
+          endpoint.hasGeocode = true;
+          if (!json || json.length === 0) {


This can be shortened as !json?.length

let/const, template literals, ?. etc - we should decide on these in a separate issue, not in this PR.

If it needs a separate issue, we can create it, however I didn't mean refactoring of the old code (which definitely would be a separate issue), but to change them in the new code. Also, if somewhere we have coding style guidelines, please, link it and I'll create an issue linked to it.

Looked at what is already in use by included libraries:

let/const - used by Bootstrap and Turbo

?. - used by Turbo

template literals - used by Bootstrap and Turbo

nertc

Everything I tested, worked perfectly. This is just recommendation for less code to be written.

nertc · 2024-08-08T06:33:57Z

app/assets/javascripts/index/directions.js

@@ -5,6 +5,7 @@
 OSM.Directions = function (map) {
  var awaitingRoute; // true if we've asked the engine for a route and are waiting to hear back
  var chosenEngine;
+  var coordinatesRegularExpression = "([+-]?\\d+(\\.\\d*)?)(?:\\s+|\\s*[/,]\\s*)([+-]?\\d+(\\.\\d*)?)";


To avoid new RegExp("^" + coordinatesRegularExpression + "$") every time we use this variable, you can define it as var coordinatesRegularExpression = /^([+-]?\d+(\.\d*)?)(?:\s+|\s*[\/,]\s*)([+-]?\d+(\.\d*)?)$/; and it will automatically be regexp.

Why does this expression have capture groups if they are not used? Instead there's endpoint.value.split(/[/,]/).

Yes, I agree, I could have used better solutions on many places. My intention was to fix the bug and to make it simple, short and readable as much as I can. The source code is already a little bit complex and requires refactoring. We can optimize it in some other PR.

nenad-vujicic · 2024-08-08T10:27:33Z

All above is fixed and PR is again ready to be reviewed. Thanks!

AntonKhorev · 2024-08-08T14:50:36Z

app/assets/javascripts/index/directions.js

@@ -51,7 +43,7 @@ OSM.Directions = function (map) {
    select.append("<option value='" + i + "'>" + I18n.t("javascripts.directions.engines." + engine.id) + "</option>");
  });

-  function Endpoint(input, iconUrl, dragCallback, geocodeCallback) {
+  function Endpoint(input, iconUrl) {


You're undoing #4902. I was going to move endpoint code into a separate module because it's difficult to test this one.

I apologize for the undoing, I thought it's just simplifying .. I returned using callback for calling getRoute, so, Endpoint can be pulled out now to separate module.

Thanks,
Nenad.

Fixes openstreetmap#2982 by updating directions.js so it doesn't perform unnecessary geocoding anymore for previously selected point (either source or destination) when entering second point. Allows reverse geocoding only when coordinates are specified from context menu. Also, improved setting markers by positioning it to selected position (not to result of geocoding) and improved focusing map view to include markers.

AntonKhorev · 2024-08-09T16:08:31Z

app/assets/javascripts/index/directions.js

    });

-    endpoint.setValue = function (value, latlng) {
+    endpoint.setValue = function (value, force_reverse_geocoding, latlng, fitMap) {


Why would a single endpoint control fitting?

Then below you have:

endpoints[0].setValue(params.from || "", !ll_from_search_form, from, false); endpoints[1].setValue(params.to || "", !ll_from_search_form, to, true);

And not it's not clear why the first endpoint gets fitMap = false but the second one gets fitMap = true.

Why would a single endpoint control fitting?

Because the route can change on single endpoint update, so, it can go out of viewing area and we need to fit bounds.

Then below you have:

endpoints[0].setValue(params.from || "", !ll_from_search_form, from, false); endpoints[1].setValue(params.to || "", !ll_from_search_form, to, true);

And not it's not clear why the first endpoint gets fitMap = false but the second one gets fitMap = true.

Because in some situations there is a flickering if we are doing fitting both times.

Why would a single endpoint control fitting?

Because the route can change on single endpoint update

It's for the code outside of endpoints to determine what to do with the route.

AntonKhorev · 2024-08-20T11:38:54Z

app/assets/javascripts/index/directions.js

+            return;
+          }
+
+          if (endpoint.latlng.distanceTo(L.latLng(json.lat, json.lon)) > 200.0) {


What was this distance check about? Was it to filter out useless reverse geocoding results that are too far away from the endpoint?

Yes, exactly! If it is found something near (e.g. 200m) the clicked coordinates, accept it, otherwise, display coordinates.

I don't do this in #5064 because it's a different problem, but it probably should be solved somewhere.

What's "near" probably depends on the current zoom level if the coordinates came from a click on the map.

nenad-vujicic · 2024-09-04T21:36:37Z

Closed in favor of #5064

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch 2 times, most recently from 77a8796 to 7afb4ce Compare June 17, 2024 14:55

nenad-vujicic marked this pull request as draft June 25, 2024 16:42

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch from 7afb4ce to b0b3ecd Compare July 2, 2024 12:58

nenad-vujicic marked this pull request as ready for review July 2, 2024 13:29

AntonKhorev reviewed Jul 4, 2024

View reviewed changes

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch from b0b3ecd to 8aa3ff8 Compare July 4, 2024 07:43

nenad-vujicic marked this pull request as draft July 8, 2024 09:28

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch from 8aa3ff8 to afc0f21 Compare July 8, 2024 21:01

nenad-vujicic marked this pull request as ready for review July 8, 2024 21:26

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch 2 times, most recently from 98acb86 to eb504ae Compare July 19, 2024 10:38

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch 2 times, most recently from 5b2e432 to 6489d0e Compare July 20, 2024 14:03

AntonKhorev reviewed Jul 20, 2024

View reviewed changes

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch from 6489d0e to 139d6c4 Compare July 24, 2024 10:07

nenad-vujicic marked this pull request as draft July 25, 2024 09:57

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch from 139d6c4 to 4394f80 Compare July 25, 2024 12:07

nenad-vujicic marked this pull request as ready for review July 26, 2024 10:38

nertc reviewed Aug 6, 2024

View reviewed changes

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch from 4394f80 to 2e20a63 Compare August 7, 2024 14:51

nenad-vujicic requested a review from nertc August 7, 2024 15:03

nertc reviewed Aug 8, 2024

View reviewed changes

nertc approved these changes Aug 8, 2024

View reviewed changes

nenad-vujicic requested a review from AntonKhorev August 8, 2024 10:33

AntonKhorev reviewed Aug 8, 2024

View reviewed changes

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch from 2e20a63 to 5b6e81e Compare August 8, 2024 17:52

nenad-vujicic requested a review from AntonKhorev August 8, 2024 18:06

nenad-vujicic force-pushed the issue_2982_displacing_point_away branch from 5b6e81e to f9d39a5 Compare August 8, 2024 19:34

AntonKhorev reviewed Aug 9, 2024

View reviewed changes

nenad-vujicic requested a review from AntonKhorev August 9, 2024 19:02

AntonKhorev mentioned this pull request Aug 10, 2024

Move directions endpoints code to its own module #5062

Merged

AntonKhorev reviewed Aug 20, 2024

View reviewed changes

nenad-vujicic closed this Sep 4, 2024

AntonKhorev mentioned this pull request Sep 8, 2024

Simplify routing endpoint interface, always do reverse geocoding #5064

Open

		var precision = OSM.zoomPrecision(map.getZoom());
		input.val(ll.lat.toFixed(precision) + ", " + ll.lng.toFixed(precision));

Fixed displacing points specified from context menu #4904

Fixed displacing points specified from context menu #4904

Conversation

nenad-vujicic commented Jun 17, 2024 • edited Loading

tomhughes commented Jun 17, 2024

nenad-vujicic commented Jun 18, 2024

AntonKhorev commented Jun 24, 2024

AntonKhorev commented Jun 25, 2024

nenad-vujicic commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

nenad-vujicic commented Jul 4, 2024

AntonKhorev commented Jul 6, 2024

nenad-vujicic commented Jul 8, 2024 • edited Loading

AntonKhorev commented Jul 18, 2024 • edited Loading

nenad-vujicic commented Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nenad-vujicic Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

nertc commented Aug 6, 2024

nertc commented Aug 6, 2024

nertc commented Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nertc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nenad-vujicic commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nenad-vujicic commented Sep 4, 2024

nenad-vujicic commented Jun 17, 2024 •

edited

Loading

nenad-vujicic commented Jul 2, 2024 •

edited

Loading

nenad-vujicic commented Jul 8, 2024 •

edited

Loading

AntonKhorev commented Jul 18, 2024 •

edited

Loading

nenad-vujicic commented Jul 19, 2024 •

edited

Loading

nenad-vujicic Jul 20, 2024 •

edited

Loading

nertc commented Aug 6, 2024 •

edited

Loading