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

feat: adding independent geometry for each step #873

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

Conversation

DhiegoHenriqueA
Copy link

Issue

adding independent geometry for each step

Tasks

  • ...
  • Update docs/API.md (remove if irrelevant)
  • Update CHANGELOG.md (remove if irrelevant)
  • review

@jcoupey
Copy link
Collaborator

jcoupey commented Jan 20, 2023

Thanks for the PR, I totally get it is useful to be able to split the final geometry into route legs between steps. On the other hand I have a couple concerns on how it is done here:

  1. This requires getting more data from OSRM (steps=true)
  2. The implementation for the libosrm wrapper would still remain to be implemented
  3. If I get it correctly, we have to first decode the various route steps for the geometry between two of our own steps, then re-encode that back as one piece. This feels weird as after all the decoded result on the client-side will simply be a subarray of the full decoded geometry.
  4. We'd be sending back chunks of geometry per step. That will probably take a lot of bandwidth for stuff that is in fact redundant with parts of the full geometry.

I wonder if we could have a simple way to simply find the rank of each step in the fully decoded geometry. This way we could add a simple step.geometry_index integer and users could easily cut the full decoded geometry into route legs client-side?

@DhiegoHenriqueA
Copy link
Author

DhiegoHenriqueA commented Jan 20, 2023

I understood your offered option, it makes total sense thinking about performance and data reduction, but would you be able to find out the geometry divider index between one leg and another without passing the steps=true?

@jcoupey
Copy link
Collaborator

jcoupey commented Jan 23, 2023

No, I did not actually check whether this is doable with OSRM without adding steps=true so maybe the extra OSRM parameter is required anyway if we want this feature.

@DhiegoHenriqueA
Copy link
Author

Do you think it's worth adding this option to the project? We could add a new parameter in the vroom to return the geometry and just in this case look for the steps resource in OSRM.

@jcoupey
Copy link
Collaborator

jcoupey commented Jan 27, 2023

I mentioned adding steps=true as an overhead because it is (probably more in term of response size rather than computing time), but it is probably not such a big concern. At least less than duplicating geometry parts in our own response.

Do you think it's worth adding this option to the project?

Yes, I think providing the ability to chunk the whole geometry into route legs between steps is a useful addition to the description of the solution we provide.

@DhiegoHenriqueA
Copy link
Author

DhiegoHenriqueA commented Jan 27, 2023

To improve the application, do you think it's necessary to add a new boolean parameter in the VROOM to return the geometry of each step? or do we leave it for all cases? And in the return we will only add a step.geometry_index?

@jcoupey
Copy link
Collaborator

jcoupey commented Jan 28, 2023

Yes, only adding a step.geometry_index would be nice as it provides all the required info in output while keeping the changes low and not bloating the response with additional geometry. Do you think it would be straightforward to add?

If the overhead of getting the index values is marginal, it would be simpler to add that by default whenever the geometry is requested (choice already possible using the -g flag).

@DhiegoHenriqueA
Copy link
Author

Yes, it seems to be easy to do that, as soon as I have time I'll do it and update the pull request.

@VROOM-Project VROOM-Project deleted a comment from sonarcloud bot Sep 8, 2023
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