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 encoded elevation to EdgeInfo - serialize in route and trace_attributes JSON #4279

Merged
merged 27 commits into from
Oct 17, 2023

Conversation

dnesbitt61
Copy link
Member

@dnesbitt61 dnesbitt61 commented Aug 29, 2023

Issue

This PR adds elevation along all edges to Valhalla tiles. Elevation is added to each node (NodeInfo) and to each edge (EdgeInfo) using delta encoding in a byte array. Adding elevation in this manner does not increase tile size by more than about 2% (so there are not tile building options - elevation is always added where available. Edges that lie outside regions where elevation is available (as specified by the directory in the config file) have a has_elevation flag set to false.

Elevation along an edge that is identified as a bridge or a tunnel is special: the elevation at the beginning and end of the bridge/tunnel edge is set at the end nodes and elevation along the edge is set using a linear interpolation between the elevation at the end nodes. This "fixes" elevation so it does not drop to the terrain below a bridge or rise to the terrain on top of a tunnel. However, this does not account for the true elevation along the bridge/tunnel if they rise of fall.

Elevation can be requested in route, trace_route, and trace_attribute calls by specifying elevation_interval. An elevation interval of 30 meters is recommended since this is the elevation interval used when encoding elevation in tiles.

Sample route request with elevation returned:
http://localhost:8002/route?json={"locations":[{"lat":40.134140,"lon":-75.515294},{"lat":40.132871,"lon":-75.518312}],"costing":"bicycle","elevation_interval":30}

JSON response includes:

"elevation_interval": 30.0,
        "elevation": [
              36.3,
              36.7,
              37.5,
              38.3,
              38.9,
              40.1,
              40.5,
              40.4,
              39.2,
              38.4,
              38.3,
              38.6,
              38.9,
              39.1
        ],

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
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

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

@kevinkreiser
Copy link
Member

awesome, great to have you back twiddling those bits!

proto/options.proto Outdated Show resolved Hide resolved
@dnesbitt61 dnesbitt61 marked this pull request as ready for review August 30, 2023 14:00
Copy link
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

This is a great addition, thanks for the effort to push this into the public project. I don't have much to add other than a few nits. I didn't look much at stuff past triplegbuilder.cc, but until there it made all sense. Great that you handled bridges & tunnels. That's currently not possible using the /height service with a route result!

One major drawback in this PR is the test coverage IMO. Just a single route request would probably exercise the majority of changed code. We already have test/pixels.h which some elevation tests are using. Couldn't a gurka test be coerced to build a large-ish map in the geography of that elevation tile? Would you be up for that @dnesbitt61? Or maybe you thought about smth else to increase coverage?

src/mjolnir/graphtilebuilder.cc Show resolved Hide resolved
src/mjolnir/elevationbuilder.cc Outdated Show resolved Hide resolved
src/mjolnir/elevationbuilder.cc Outdated Show resolved Hide resolved
valhalla/midgard/elevation_encoding.h Show resolved Hide resolved
valhalla/midgard/elevation_encoding.h Show resolved Hide resolved
valhalla/midgard/elevation_encoding.h Outdated Show resolved Hide resolved
valhalla/midgard/elevation_encoding.h Outdated Show resolved Hide resolved
src/mjolnir/edgeinfobuilder.cc Outdated Show resolved Hide resolved
src/thor/triplegbuilder.cc Outdated Show resolved Hide resolved
docs/docs/api/turn-by-turn/api-reference.md Outdated Show resolved Hide resolved
@dnesbitt61
Copy link
Member Author

I'm not too familiar with gurka tests. Are there any unit tests that build tiles with elevation that could be used to test? I don't have much time this week, but perhaps can look more next week.

@nilsnolde
Copy link
Member

It's a bit complex-ish.. there's a header test/pixels.h which has the binary representation of one elevation tile somewhere in the US. That'd first need to be saved to disk locally in the test code to a path, that path has to get set in the gurka test config as the elevation path, then build tiles with that config and do a route. Or smth like that 😄 I can help out on that early next week if you want.

@dnesbitt61
Copy link
Member Author

pixels.h adds some elevation data to create a sparse set of data within a tile. Does this elevation data run along any graph edges (hopefully 2 connected edges)? The sparse elevation data would have to fully surround the road since the skadi sample method looks at adjacent elevations when forming the elevation. If so, then we could create data with this and do trace route along the road path and check that the encoded elevation is close to matching what is expected along the path.

@kevinkreiser
Copy link
Member

im not sure if its near anything, i made that a long time ago and forget pretty much everything about where i put it. it is probably easier to create fake data on the fly in the test by just making an srtm tile with predictable data (each row of pixels has the same elevation and it maybe increases or decreases with each row in the tile). then north south roads in a gurka test would have increasing/decreasing elevation where as east west ones would have constant elevation. thats what id do for a test.

@dnesbitt61
Copy link
Member Author

Trying to use pixels.h but have some questions/issues:

  1. Why are values for each index included 4 times?
  2. This doesn't seem close to any real values. There are very large jumps in elevation between adjacent posts. Since the elevation along an edge is encoded using delta encoding (max change in any 30 meter increment is 16 meters) I do not believe this will work as a test.

@kevinkreiser
Copy link
Member

@dnesbitt61
Copy link
Member Author

so duplicates (4x per index) are due to the way you created this. This shouldn't impact the test. However, the actual elevation values cannot be real if this is from N40W077.hgt. There should be no negative values in this land area. That and the large delta's between adjacent postings (such as {4521066, 1}, {4521067, 513})

@gknisely
Copy link
Member

@dnesbitt61 you can see how i created the data here: 6fb8b3d#diff-cdf1ee9054e7a0d87f469d85364ff34bc799e7655fbcc4baf3308ae4f6c10912R10

@kevinkreiser what log infos are we to uncomment? re: uncommenting the log infos

@gknisely
Copy link
Member

@dnesbitt61 you can see how i created the data here: 6fb8b3d#diff-cdf1ee9054e7a0d87f469d85364ff34bc799e7655fbcc4baf3308ae4f6c10912R10

@kevinkreiser what log infos are we to uncomment? re: uncommenting the log infos

@kevinkreiser nm. I was looking at test/sample.cc and not skadi/sample.cc

@kevinkreiser
Copy link
Member

There should be no negative values in this land area. That and the large delta's between adjacent postings (such as {4521066, 1}, {4521067, 513})

remember that the values are in srtm format, which is big endian. to make use of the values (as is done in sample.cc) you need to flip the endianess. you cant look at the numbers in the pixels header and make sense of them like you're trying to do. they all go through this function to be read:

int16_t flip(int16_t value) {
return ((value & 0xFF) << 8) | ((value >> 8) & 0xFF);
}

@dnesbitt61
Copy link
Member Author

that is what I was missing. thanks for clarifying.


EXPECT_EQ(elevation_along_edges.size(), elevation_from_skadi.size());
for (size_t i = 0; i < elevation_along_edges.size(); ++i) {
EXPECT_NEAR(elevation_along_edges[i], elevation_from_skadi[i], 0.5f);
Copy link
Member

Choose a reason for hiding this comment

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

@dnesbitt61 is 0.5f tolerance ok here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is fine for now. I suspect we could find cases (esp. long edges) where the accuracy could deviate more, requiring a higher tolerance. Statistically, the delta encoding should do pretty well except the cases where slope exceeds 45 percent.

kevinkreiser
kevinkreiser previously approved these changes Oct 17, 2023
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.

nice collaboration here

@kevinkreiser
Copy link
Member

seems the test doesn't work on osx. lint build might just be a random failure

@gknisely
Copy link
Member

seems the test doesn't work on osx. lint build might just be a random failure

Looks like I just forgot to close the file

@gknisely gknisely merged commit f22344c into master Oct 17, 2023
6 of 8 checks passed
@gknisely gknisely deleted the elevation_in_tiles branch October 17, 2023 14:49
@nilsnolde nilsnolde mentioned this pull request Dec 2, 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.

None yet

4 participants