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

Add SacScale to trace_attributes output #2818

Merged
merged 18 commits into from
Mar 15, 2021
Merged

Add SacScale to trace_attributes output #2818

merged 18 commits into from
Mar 15, 2021

Conversation

dnesbitt61
Copy link
Member

@dnesbitt61 dnesbitt61 commented Jan 28, 2021

Issue

trace_attributes does not output SacScale (hiking difficulty). This PR adds SacScale to the edge attributes output from trace_attributes. In addition, the sac_scale value of a directed edge is output in the JSON for locate actions.

The following simple request illustrates the output:

http://localhost:8002/trace_attributes?json={"shape":[{"lat": 44.184851, "lon": 7.170174},{"lat": 44.184435, "lon": 7.173414}],"shape_match":"map_snap", "costing":"bicycle"}

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

gknisely
gknisely previously approved these changes Jan 28, 2021
@gknisely
Copy link
Member

@dnesbitt61 MinGW64 build is failing

@dnesbitt61
Copy link
Member Author

MinGW64 issue:
Problem: conflicting requests

  • nothing provides mingw64(api-ms-win-core-synch-l1-2-0.dll) needed by mingw64-boost-1.75.0-1.fc34.noarch

I'm not sure what (if anything I would have done to cause this!)

@nilsnolde
Copy link
Member

@dnesbitt61 i think that’s a pretty random thing right now.. smth in rawhide got messed up is my guess.. my win ninja PR kept failing randomly while I was only updating the azure config. I think it’s safe to ignore it right now.. I’ll see if I can move that CI to a stable fedora version

@kevinkreiser
Copy link
Member

@dnesbitt61 this looks good to me. can we hack in a quick gurka test to verify it, should be pretty quick. gurka supports trace_attributes, i added that pretty recently i believe

@dnesbitt61
Copy link
Member Author

I do not see any gurka examples using trace_attributes action or any expect_* methods to evaluate results. Am I missing something?

@kevinkreiser
Copy link
Member

@dnesbitt61 we now have a DoAction call in gurka that should allow you to test trace_attributes

@kevinkreiser
Copy link
Member

@dnesbitt61 dont forget to run the format script

@dnesbitt61
Copy link
Member Author

the new test fails with:
C++ exception with description "Unknown route serialization action" thrown in the test body.

@dnesbitt61
Copy link
Member Author

this test now works. Good thing, I was missing one of the sac scales in the proto conversion which led to some incorrect values. One problem - I use auto costing. If I change to pedestrian there is only 1 edge in the result.

@kevinkreiser
Copy link
Member

@dnesbitt61 i suspect this could be because of costing that pedestrian only gets one edge. i reckon that our walking costing, by default, avoids hardcore hiking which i think in your test is in the middle of the route. you can modify the costing options to allow for an alpine sac scale in the test if you want to expand the test to do pedestiran as well. let me know i can take a crack at it and then we can merge it

Copy link
Member

@kevinkreiser kevinkreiser left a comment

Choose a reason for hiding this comment

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

looks good to me @gknisely are you good too?

@dnesbitt61 dnesbitt61 merged commit 14bbbe0 into master Mar 15, 2021
@dnesbitt61 dnesbitt61 deleted the sac_scale branch March 15, 2021 13:24
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.

None yet

4 participants