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

Enable a dynamic range search for matched way points #729

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

Conversation

afischerdev
Copy link
Collaborator

This should enable the #387 issue.

It could be tested on the BRouter test server together with the #728 and #726
Sample

The logic is only valid for the first and the last point. So please test 2 points routes only at the moment.

For a dynamic range test you will need a new profile constant

assign use_dynamic_range   = true  # %use_dynamic_range% | Enable distant start/end points | boolean

Please remember to test #728 with this entry

assign check_start_way      = true   # %check_start_way% | Activate a test for the starting way | boolean | noStartWay=route,ferry;highway,motorway;highway,motorway_link

@devemux86
Copy link
Contributor

@afischerdev

The logic is only valid for the first and the last point. So please test 2 points routes only at the moment.

Testing it I see that besides the first and last point, it also works with the via points?

If we place a via point away from the road, it produces 2 beelines going to and returning from the via point.

I am not sure what users want, a global setting for all points (start + via + end) or more detailed control?


@abdullahO2 can you test it on BRouter test website and tell us your opinion?

(instructions are in the 1st post)

@afischerdev
Copy link
Collaborator Author

@devemux86

Testing it I see that besides the first and last point, it also works with the via points?

When you test this in the web client this will work because web client sends line by line to BRouter.
The Android client will break at the via point. I'm working on it - it was my own restriction.

@abdullahO2
Copy link

@afischerdev and @devemux86

I'm incredibly excited to report that I've tested the new feature on the BRouter test website, and it works flawlessly! I tested a route with two off-road waypoints in a remote desert area, and BRouter seamlessly generated the optimal path, including the necessary beeline segments. This feature will significantly enhance my ability to explore remote areas and plan adventures that were previously difficult to navigate.

I appreciate your responsiveness and hard work in developing this feature so quickly. It's truly remarkable to see it come to life, especially considering the complexities involved. I want to extend my special thanks to @afischerdev for implementing this complex feature so effectively, and to @devemux86 for his insightful feedback and testing.

To answer @devemux86's question about via points, I find the current implementation, which generates beelines to and from off-road via points, to be ideal for my needs. It strikes a great balance between flexibility and practicality, allowing me to plan routes that include off-road sections without sacrificing ease of use.

I'm so impressed with BRouter and its ongoing development that I'd love to make a donation to support the project. Is there a way to contribute financially? If so, could you please provide me with the relevant information?

Best regards,

Abdullah Abdulrahman

@afischerdev
Copy link
Collaborator Author

Now the via points are enabled for the library.
I tested something like this with 'handmade' and dynamic via points at start, end and in the middle

Don't forget to assign the new constant

assign use_dynamic_range = true # %use_dynamic_range% | Enable distant points | boolean

@abdullahO2
Copy link

@afischerdev

I've noticed that the dynamic_range feature works within a range of approximately 20km from the nearest road. Attempting to exceed this range results in a message like "Start marker is too far from a route."

Would it be possible to make this distance configurable through the profile settings? This would provide users with greater flexibility in situations where they need to navigate further away from established roads.

Thanks again for your excellent work on this feature!

@devemux86
Copy link
Contributor

@abdullahO2 @afischerdev

Isn't this the waypointCatchingRange profile numeric parameter (default 250 m)?

@abdullahO2
Copy link

@devemux86

I've tried setting assign waypointCatchingRange = 3000 in the profile, but it didn't seem to change anything. I tested this with the destination point, and the "Destination marker is too far from a route" message still appears when exceeding approximately 20km from the nearest road.

@afischerdev
Copy link
Collaborator Author

@abdullahO2
Do you have some coordinates from your test that we could check against the new lib?

@abdullahO2
Copy link

@abdullahO2 Do you have some coordinates from your test that we could check against the new lib?

https://brouter.de/brouter-test/#map=10/17.8696/46.8004/standard&lonlats=46.895592,17.601443;46.720824,17.530452;46.587524,17.680655

@afischerdev
Copy link
Collaborator Author

@abdullahO2
Thanks for your sample.
When I add this it works:

assign use_dynamic_range = true

The profiles are not prepared for this test, you are using the public ones. It will be the next step when this work is done to enable a switch inside the current profiles.

@abdullahO2
Copy link

abdullahO2 commented Oct 2, 2024

@afischerdev
Copy link
Collaborator Author

@abdullahO2
Thanks for the sample. This helped to find the error.
Please try the new library.

@abdullahO2
Copy link

@afischerdev

Thank you for the update! I can confirm that the new library resolves the 20km limit issue.

However, I've noticed a new, unexpected behavior. When requesting a route to a distant location, BRouter sometimes generates a direct line even when there are roads relatively nearby. It seems to bypass the step of finding the nearest road and creating a direct line from there.

Unfortunately, I'm unable to provide specific examples at the moment because the server is currently experiencing a 502 Bad Gateway error. I'll share more details as soon as the server is back online.

@afischerdev
Copy link
Collaborator Author

@abdullahO2
Server is restarted.

@abdullahO2
Copy link

abdullahO2 commented Oct 4, 2024

@afischerdev

I can now provide an example of the new issue I mentioned earlier.

With assign use_dynamic_range = true in the profile, please see this route:

https://brouter.de/brouter-test/#map=8/20.225/43.671/standard&lonlats=46.720379,22.287226;46.604144,18.122351

As you can see, BRouter generates a direct line between the two points, even though there are roads relatively nearby. It doesn't seem to be finding the nearest road to the end point and creating a direct line from there, as expected.

I hope this example helps clarify the issue.

image

@afischerdev
Copy link
Collaborator Author

@abdullahO2
Ok, the size for the distance check is now more expanded. And the point is marked as 'not reachable' when not inside.

The picture shows all the called data chunks in this area. In the middle, the standard call with smaller data parts is visible.
distance

All this will have new side effects - I guess. Please give it a new try.

@abdullahO2
Copy link

@afischerdev

Thank you for the update! Expanding the distance check and marking unreachable points is a good step forward.

However, while this solves the 20km limit issue, I've noticed some unexpected behavior in certain cases. Sometimes, BRouter generates a direct line to distant locations even when there are roads relatively close by. It seems like the expanded distance check might be causing it to bypass nearby roads in favor of a direct line, even when that's not the most logical route.

Here's an example that illustrates this behavior:

https://brouter.de/brouter-test/#map=8/22.162/48.238/standard&lonlats=48.103582,23.083552;48.784866,20.802156

Compared to this more logical route:

https://brouter.de/brouter-test/#map=8/22.159/48.235/standard&lonlats=48.103582,23.083552;48.806759,20.956385

It appears that the expanded distance check might need further refinement to ensure that it prioritizes nearby roads when generating direct lines to distant locations.

I appreciate your continued efforts to improve this feature.

@abdullahO2
Copy link

@afischerdev

Thank you again for the updates! I've been reviewing the recent changes to useDynamicRange and have a suggestion that might further improve its behavior, particularly in cases where closer roads are bypassed in favor of a direct line to a distant location.

I've been experimenting with a modified version of the preloadPosition method in NodesCache.java that uses a tiered approach to expand the search area when useDynamicRange is enabled. The idea is to prioritize closer roads by starting with a smaller search radius and gradually expanding it if no suitable roads are found.

Suggestion (generated with the help of an AI assistant):

private void preloadPosition(OsmNode n, int initialCellsize, int initialScale) {
    first_file_access_failed = false;
    first_file_access_name = null;

    int cellsize = initialCellsize;
    int scale = initialScale;

    // Try loading segments with increasing search radius until a match is found or a limit is reached
    while (waypointMatcher != null && ((WaypointMatcherImpl) waypointMatcher).useDynamicRange && !waypointMatcher.isMatched(n) && cellsize <= 1000000 / 32) { 
      for (int idxLat = -scale; idxLat <= scale; idxLat++) {
        for (int idxLon = -scale; idxLon <= scale; idxLon++) {
          if (idxLon != 0 || idxLat != 0) {
            loadSegmentFor(n.ilon + cellsize * idxLon, n.ilat + cellsize * idxLat);
          }
        }
      }

      // Increase search radius and scale for the next iteration
      cellsize *= 2; // Or any other suitable factor
      scale++; 
    }


    if (first_file_access_failed) {
      throw new IllegalArgumentException("datafile " + first_file_access_name + " not found");
    }
  }

  // Add a helper method to WaypointMatcher to check if a waypoint is matched
  interface WaypointMatcher {
    // ... other methods ...

    boolean isMatched(OsmNode waypoint);
  }

  // Implement the isMatched method in WaypointMatcherImpl
  class WaypointMatcherImpl implements WaypointMatcher {
    // ... other methods ...

    @Override
    public boolean isMatched(OsmNode waypoint) {
      for (MatchedWaypoint mwp : waypoints) {
        if (mwp.waypoint == waypoint && mwp.crosspoint != null) {
          return true;
        }
      }
      return false;
    }
  }

This tiered approach could potentially help ensure that BRouter always finds the most logical route, regardless of the distance to the nearest road.

Please note that this is just a suggestion generated with the help of an AI assistant. I'm not a software developer, so I might not be fully aware of all the implications of this change. I'd love to hear your thoughts on this and whether you think it's worth exploring further.

Thank you for your continued dedication to improving BRouter!

@afischerdev
Copy link
Collaborator Author

@abdullahO2
thank you very much for the suggestion and the examples. I will try it, but it will take a while.

At first glance I think it will not work because a test for crosspoint = null is no longer true after the first point pass with dynamic distance. And preload node n is not the point the waypoint could compete to.

Anyway we need a solution and cellsize has not been treated in this way so far. We will see.

@abdullahO2
Copy link

@afischerdev

You're very welcome! I appreciate you taking the time to consider the suggestion. I understand that it might not be a straightforward implementation, and I trust your expertise in finding the best solution.

Thank you again for your valuable time and dedication to improving BRouter. Your commitment to ongoing development is truly appreciated.

@afischerdev
Copy link
Collaborator Author

@abdullahO2
Here now the test results:
The long distances beeline is generated when checking the first waypoint.
block_1

The second block doesn't contain the 2nd waypoint. The red line is from an other test to show the way.
block_2

That means to me the routine needs a max distance check to protect from long beelines.

One suggestion was to increase the cell size. This ends up in longer (uncontrolled) routes.
Current sample - red line with a max distance check:
distance

Another suggestion was to check whether a result was already available - sample above.
One problem with this was the running time doubled - in this area. In normal contexts, however, we need this test to avoid working with too much data.

My current library is installed on test server and could be used - while I clean my sources.

@abdullahO2
Copy link

@afischerdev

Thank you so much for taking the time to investigate this issue further and for sharing your detailed test results. I really appreciate your dedication to finding a solution that works effectively.

I agree that implementing a max distance check is crucial to prevent the generation of excessively long beelines. I'm also open to any other ideas that you think might improve the behavior of useDynamicRange without negatively impacting performance.

I understand the trade-offs involved in increasing the cell size, and I agree that it's important to find a solution that balances accuracy with efficiency.

I'm happy to test your current library on the test server and provide feedback as soon as I can.

Thank you again for your hard work and commitment to improving BRouter. It's truly inspiring!

@afischerdev
Copy link
Collaborator Author

@abdullahO2
Did you find the time to test the last published version on test server?
And if so ready for publishing?

@abdullahO2
Copy link

@afischerdev

My apologies for the delayed response. I've finally had a chance to test the latest version on the test server, and I'm thrilled to report that the results are fantastic! The improvement is remarkable, and the number of unreachable locations is significantly reduced compared to previous versions. The difference is truly night and day.

I did notice that the server's response time can be slow occasionally. I'm not sure if this is related to the new feature or due to something else entirely.
firefox_30

Overall, I'm very impressed with the latest changes, and I believe this feature is ready for publishing. Thank you again for your hard work!

@afischerdev
Copy link
Collaborator Author

@abdullahO2
I couldn't find your break. Anyway:
The available memory is very small, but should be alright for a test server.
java -Xmx512m -Xms512m -Xmn64m ...

@abdullahO2
Copy link

@afischerdev

Thanks for looking into the server response time. I understand that the available memory on the test server is limited. I haven't experienced any significant issues affecting my testing, so I wouldn't be overly concerned about it for now. If I encounter any further problems, I'll be sure to let you know.

@devemux86
Copy link
Contributor

@afischerdev Shouldn't the variable be added to other profiles too, like fastbike, etc.?

@afischerdev
Copy link
Collaborator Author

@abdullahO2

If I encounter any further problems, I'll be sure to let you know.

That is a good way. So I will merge it.
First I added the assign use_dynamic_range = true to some profiles.

@devemux86
At the moment I ignored the profiles fastbike, fastbike-verylowtraffic, vm-forum-liegerad-schnell, vm-forum-velomobil-schnell because they are not really off road prepared.

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.

3 participants