removing the code that is merging turn instructions - Issue #177 #179
Conversation
|
I agree that this is definitely an issue in terms of missing out important instructions. I believe that the original logic was to try and cut down the number of instructions that would give the same information (i.e. on a large curved bend, you may get multiple instructions saying "turn slight right"). I think that we should look into this again at some point and try to re-include the merging of instructions where it is suitable, as in the old code the logic seems a little amiss.
The above two images show where merging of instructions is not valid. The two images below shows where the merging of instructions could be seen as better
|
There was a problem hiding this comment.
I have copied the changed code into a local version of ORS on my machine, and two of the API tests are failing (ResultTest.testSteps:571 and ResultTest.testStepsDetails:591).
Can you update both of those tests so that they pass as they are both related to the number of steps (and so instructions) that are returned in the response.
To run the API tests, you need to copy the app.config.test file from the openrouteservice-api-tests/conf folder into the openrouteservice/WebContent/WEB-INF folder and use that for building ORS (change its name to app.config, but make sure that you rename your current app.config to something like app.config.dev so you don't lose your changes in there) and then rebuild and run the ORS service. Then in the openrouteservice-api-tests run "mvn clean install" to run the api tests.
If there are any problems let me know and I will try to help.
|
I am working on the test fixes asap... for the given example where it "might" had been sense full that turn instructions will be merged - from my perspective the answer would be "Yes & No" - IMHO this is an issue of the graph hopper turn instructions itself - I see them at many places - specially when street names are changing (and even if there is no junction in the OSM data)... So WHEN you want to get rid of "unnecessary" turn instructions, then this should/have to be addressed within GraphHopper - since there we have the information about the junction/nodes (and the possible alternative ways)... In the Turn instruction the infos are already lost (what are the alternative options at a certain node) - and with that it's IMHO impossible to make the decision about sense or nonesense. |
|
tests all pass now thanks - I am not sure why you were having a problem with the GeoJSON test as it worked ok on my local version on both the development branch and on the one with the above changes... |
|
@marq24 sorry, one last thing that I missed in the original review - can you add the following to the CHANGELOG.md (under [Unreleased] Fixed:) please so that everything is all kept together in the one pull request: Updated code to remove merging of instructions as this resulted in missing important turn instructions (Issue #177) |
… into revert-turn-instruction-merging
|
I hope now all is complete and ready to merge :-) |
|
All merged into development, thanks |
Pull Request Checklist
[x ] 1. I have merged the latest version of the development branch into my feature branch and all conflicts have been resolved
[ -] 2. I have added information about the change/addition to functionality to the CHANGELOG.md file under the [Unreleased] heading
[ -] 3. I have documented my code using JDocs tags
[ x] 4. I have removed unecessary commented out code, imports and System.out.println statements
[ -] 5. I have written JUnit tests for any new methods/classes and ensured that they pass
[ -] 6. I have created API tests for any new functionality exposed to the API
[ -] 7. If changes/additions are made to the app.config file, I have added these to the app.config.sample file along with a short description of what it is for, and documented this in the Pull Request (below)
[ x] 8. I have built graphs with my code of the Heidelberg.osm.gz file and run the api-tests with all test passing
-> there are 2 TESTS that are failing right now! [ the GeoJSON Tests that are checking for 'features' = yes] - i have no clue what need to be done so that they will pass - but this is already failing with the original development branch
[ x] 9. I have referenced the Issue Number in the Pull Request (if the changes were from an issue)
[ -] 10. For new features involving building of graphs, I have tested on a larger dataset (at least Germany) and the graphs build without problems (i.e. no out-of-memory errors)
[ x] 11. I have written in the Pull Request information about the changes made including their intended usage and why the change was needed.
Fixes #177 .
Information about the changes
Key functionality added:
well actually I have removed the TurnInstruction Merging code - simply cause I have found only cases where REQUIRED turn instructions will be removed.
Reason for change:
Required Turn Instructions will be removed from the Instruction list (merged) that should not be removed - actually might be that the code was required in the past - but honestly I already missed two times my route (on my road bike) cause if this turn instruction "optimization"... And IF there was ever any use - then the negative impact is way to massive.
Required changes to app.config (if applicable)
Kind Regards
Matthias