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

Elevation feature #1095

Closed
wants to merge 2 commits into from
Closed

Conversation

gberaudo
Copy link
Contributor

Since last pull request, lots of code was removed and tests reworked:

  • The Phantom node elevation is computed earlier (patch from @radupopescu;
  • Test code is reworked to use valuable suggestions by @emiltin.

The pull request was split in two parts:

  • small fixes unrelated to elevation feature (Fixes #1094);
  • elevation feature, with tests.

Regarding #1089 (comment),
an open discussion would certainly ease mutual understanding and lead to solutions.

The code looks good now and would probably be of use for others.
I found out these project which try to enhance OSM data using SRTM elevations:

Regards,

@DennisOSRM
Copy link
Collaborator

Tests are failing:

cucumber -p verify features/elevation/elevation.feature:8 # Scenario: Route and retrieve elevation - match on elevations - short
cucumber -p verify features/elevation/elevation.feature:33 # Scenario: Route and retrieve elevation - match on elevations
cucumber -p verify features/elevation/elevation.feature:64 # Scenario: Route and retrieve elevation - match on decoded geometry
cucumber -p verify features/elevation/elevation.feature:95 # Scenario: Route and retrieve elevation - match encoded geometry
cucumber -p verify features/elevation/elevation.feature:132 # Scenario: Route with elevation without ele tags in osm data

@gberaudo
Copy link
Contributor Author

Is it possible to have conditional cucumber tests based on compilation
flags?

Some elevation tests may only be run if the program has been compiled
with elevation support (cmake -DOSRM_HAS_ELEVATION:BOOL=ON).

@DennisOSRM
Copy link
Collaborator

Not that I know, but lets hold any work here for now. At this point it is not clear if this can or will be merged. See #1090

@gberaudo
Copy link
Contributor Author

Does anybody knows why Windows compilation is failing?
Are there some shortcomings with constexpr on this compiler?

@DennisOSRM
Copy link
Collaborator

shortcomings is very flattering to MSVC.

@@ -39,7 +39,7 @@ def test_routability_row i

Then /^routability should be$/ do |table|
build_ways_from_table table
reprocess
reprocess elevation_data_enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

i suggest just reading the option in reprocess() instead of passing it everywhere

@emiltin
Copy link
Contributor

emiltin commented Jun 24, 2014

i added a few comments to details of the implementation, but like dennis i suggest to wait a bit until we figure out if the current approach to elevation is atually the way forward (i have some doubts myself).

gberaudo added 2 commits June 25, 2014 11:55
- Import elevation from osm node tag 'ele';
- Optional elevation in JSON and GPX formats;
- Phantom nodes elevation set early;
- Elevation tests with cucumber -p elevation.

The elevation is stored as an int.
Using a smaller structure would lead to misalignement or no gain.

The routing algorithm is unchanged.

The elevation feature must be enabled at compile time with:
cmake option -DOSRM_HAS_ELEVATION:BOOL=ON and each tool must be passed
the '-e 1' parameter to activate it. Finally, the viaroute request may
either include the 'elevation=true' parameter to get elevation, set it
to false or skip it to get default behaviour.

The elevation is read as meters from the osm data.
Since we must store it as an int, we get a 1cm precision.
It is then exported as 10^4 meters in JSON.
@gberaudo
Copy link
Contributor Author

gberaudo commented Jun 1, 2015

Closing this PR as it doesn't look to be merged any time soon.

@gberaudo gberaudo closed this Jun 1, 2015
@TheMarex
Copy link
Member

TheMarex commented Jun 1, 2015

@gberaudo @lbud is working on something similar in #1102

@gberaudo
Copy link
Contributor Author

gberaudo commented Jun 1, 2015

Thank you @TheMarex for pointing to this discussion.

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.

4 participants