fix graphbuilder's elevation initialization #3571

nilsnolde · 2022-03-11T15:44:19Z

finally getting some time to fix some of my old issues..

mostly fixes returning "mean_elevation": null when there is none, instead of 1234. also some others:

EdgeInfo::json was casting elevation to an unsigned integer, leading to bogus values for the (admittedly few) roads which are sub sea level, e.g. next to dead sea:

curl --request POST \
  --url https://valhalla1.openstreetmap.de/locate \
  --header 'Content-Type: application/json' \
  --data '{
	"costing": "auto",
	"verbose": true,
	"id": 1,
	"locations": [
		{
			"lon": 35.408936,
			"lat": 31.591404
		}
	]
}'

leads to "mean_elevation": 18446744073709551254. with this PR it's "mean_elevation": -362.

…ed mean_elevation to unsigned integer for /locate response leading to bogus values

src/tyr/trace_serializer.cc

valhalla/baldr/graphconstants.h

src/baldr/edgeinfo.cc

src/mjolnir/graphbuilder.cc

…a little

nilsnolde · 2022-03-16T13:32:31Z

src/mjolnir/edgeinfobuilder.cc

@@ -24,10 +24,10 @@ void EdgeInfoBuilder::set_wayid(const uint64_t wayid) {

 // Set the mean elevation.
 void EdgeInfoBuilder::set_mean_elevation(const float mean_elev) {
-  if (mean_elev < kMinElevation) {
+  if (mean_elev <= kNoElevationData) {


I got rid of the kMinElevation as it's now the same value as kNoElevationData. let me know if you want to keep it for semantics

isnt it the case that we need to differentiate between no data and minimum value though? like when its no data then we serialize null and when its minimum value we serialize the minimum value, right?

Minimum value (-500) will not be serialized, same as no data. Think that’s fine, there’s really no place on terrestrial earth at -500. Dead Sea depression is at -450 or so, roads near that have the lowest values in the world. What do you think @kevinkreiser ? Since it's only for enriching the graph where we only look at roads, the /height endpoint won't be affected.

yeah you are saying in practice -500 wont happen so no_data can be the same as min_value but like you mentioned at the beginning it does seem a bit semantically strange in the computation later where you do an offset by the no data value. i'd be happy just duplicating the constant even if they were the same value so that when you read the code you offset by the min-value rather than the no-data-value

yeah true, wasn't very happy with that either but with the comment above the constant in graphconstants I found it ok-ish.. then let me duplicate that constant

reverted the change to edgeinfo.h where kMinElevation was set before, which reverted all changes to edgeinfo.h and edgeinfobuilder.h. kMinElevation is now used where appropriate

is the internet broken today?! github is being ridiculously slow or doesn't respond at all after pushing, geofabrik same, curl actually times out downloading some pbf.. maybe it's telling me to go home instead..

case in point:

To github.com:gis-ops/docker-valhalla.git ! [remote rejected] master -> master (Internal Server Error) error: failed to push some refs to 'github.com:gis-ops/docker-valhalla.git'

first time ever for me

nilsnolde · 2022-03-16T13:34:38Z

src/mjolnir/graphbuilder.cc

-                                      (*nodes[target]).graph_id, w.way_id(), 1234, bike_network,
-                                      speed_limit, shape, names, tagged_values, pronunciations, types,
-                                      added, dual_refs);
+                                      (*nodes[target]).graph_id, w.way_id(), kNoElevationData,


so I changed it from 0 to the no data value, which will return null in the geojson response. I traced all usages of mean_elevation & co and can't see how this change would affect anything negatively

nilsnolde · 2022-03-16T13:35:28Z

src/thor/triplegbuilder.cc

-    } else {
-      trip_edge->set_mean_elevation(kNoElevationData);
-    }
+    trip_edge->set_mean_elevation(edgeinfo.mean_elevation());


this was redundant for elevation. the set_mean_elevation will handle the case when there's no elevation set

its redundant now that you fixed it! it wasnt redundant in the past, previously this protected us from getting 1234 in the response

valhalla/baldr/edgeinfo.h

src/skadi/util.cc

test/util_skadi.cc

…lue from skadi and convert to -500 in mjolnir

src/skadi/util.cc

…i to return the no_data value

src/skadi/util.cc

nilsnolde · 2022-03-17T17:05:41Z

the osx build finished, it was triggered when github had their outages. probably the reason why win never started?

…1234

nilsnolde · 2022-03-22T14:18:47Z

fixed up the last minor things @kevinkreiser . thanks for the review

christian-nils · 2022-03-29T13:32:55Z

src/tyr/trace_serializer.cc

+          edge_map->emplace("mean_elevation", static_cast<int64_t>(options.units() == Options::miles
+                                                                       ? mean
+                                                                       : mean * kFeetPerMeter));


@nilsnolde, it seems that the "inline if" is not correct here.

Suggested change

edge_map->emplace("mean_elevation", static_cast<int64_t>(options.units() == Options::miles

? mean

: mean * kFeetPerMeter));

edge_map->emplace("mean_elevation", static_cast<int64_t>(options.units() == Options::miles

? mean * kFeetPerMeter

: mean ));

wow oops, i missed that. thanks for the catch and ill take care of that!

Co-authored-by: Christian-Nils Åkerberg Boda <[email protected]>

Co-authored-by: Christian-Nils Åkerberg Boda <[email protected]> Co-authored-by: Christian-Nils Åkerberg Boda <[email protected]>

* Fix in addition to #3571 * Update changelog

graphbuilder initialized elevation on edges with 1234; edge_info cast…

d808560

…ed mean_elevation to unsigned integer for /locate response leading to bogus values

nilsnolde requested review from kevinkreiser and dnesbitt61 March 11, 2022 15:44

nilsnolde commented Mar 11, 2022

View reviewed changes

src/tyr/trace_serializer.cc Show resolved Hide resolved

nilsnolde commented Mar 11, 2022

View reviewed changes

valhalla/baldr/graphconstants.h Outdated Show resolved Hide resolved

nilsnolde commented Mar 11, 2022

View reviewed changes

src/baldr/edgeinfo.cc Show resolved Hide resolved

kevinkreiser reviewed Mar 11, 2022

View reviewed changes

src/mjolnir/graphbuilder.cc Outdated Show resolved Hide resolved

nilsnolde added 2 commits March 14, 2022 12:46

Merge branch 'master' into nn-fix-elevation-1234

9572f72

initialize mean elevation to kNoElevationData; consolidate constants …

c07efc2

…a little

nilsnolde commented Mar 16, 2022

View reviewed changes

valhalla/baldr/edgeinfo.h Outdated Show resolved Hide resolved

nilsnolde requested a review from kevinkreiser March 16, 2022 13:40

handle no_data values in elevation tiles

da0b97b

nilsnolde commented Mar 16, 2022

View reviewed changes

src/skadi/util.cc Outdated Show resolved Hide resolved

kevinkreiser reviewed Mar 16, 2022

View reviewed changes

test/util_skadi.cc Show resolved Hide resolved

move kNoElevationDataRaw to graphconstants; return the raw no_data va…

a5f2574

…lue from skadi and convert to -500 in mjolnir

nilsnolde commented Mar 17, 2022

View reviewed changes

src/skadi/util.cc Outdated Show resolved Hide resolved

nilsnolde commented Mar 17, 2022

View reviewed changes

src/skadi/util.cc Outdated Show resolved Hide resolved

nilsnolde requested a review from kevinkreiser March 17, 2022 09:50

nilsnolde mentioned this pull request Mar 17, 2022

/locate does not convert to miles/feet #3572

Closed

nilsnolde added 2 commits March 17, 2022 12:45

revert most of the previous changes and use the free function in skad…

292d8cf

…i to return the no_data value

remove extra header

5bf428b

kevinkreiser reviewed Mar 17, 2022

View reviewed changes

src/skadi/util.cc Outdated Show resolved Hide resolved

nilsnolde added 3 commits March 17, 2022 14:30

use n to derive if there was elevation values or not

dfe2cdc

revert removing kMinElevation

e87cf7a

forgot one spot to use kMinElevation

63bc334

nilsnolde requested a review from kevinkreiser March 17, 2022 14:23

nilsnolde added 2 commits March 22, 2022 15:14

Merge remote-tracking branch 'upstream/master' into nn-fix-elevation-…

2497357

…1234

fix minor stuff

89827bb

kevinkreiser approved these changes Mar 22, 2022

View reviewed changes

nilsnolde merged commit 6e28861 into master Mar 22, 2022

nilsnolde deleted the nn-fix-elevation-1234 branch March 22, 2022 17:39

christian-nils reviewed Mar 29, 2022

View reviewed changes

kevinkreiser added a commit that referenced this pull request Mar 29, 2022

https://github.com/valhalla/valhalla/pull/3571#discussion_r837481014

37cecda

Co-authored-by: Christian-Nils Åkerberg Boda <[email protected]>

kevinkreiser mentioned this pull request Mar 29, 2022

fix trace mean elevation serialization #3586

Merged

kevinkreiser added a commit that referenced this pull request Mar 29, 2022

#3571 (comment) (#3586)

6bdc1e6

Co-authored-by: Christian-Nils Åkerberg Boda <[email protected]> Co-authored-by: Christian-Nils Åkerberg Boda <[email protected]>

DmitryAk added a commit to DmitryAk/valhalla that referenced this pull request Sep 7, 2022

Fix in addition to valhalla#3571

6f1877c

DmitryAk mentioned this pull request Sep 7, 2022

Fix elevation serializing #3735

Merged

1 task

kevinkreiser pushed a commit that referenced this pull request Sep 7, 2022

Fix elevation serializing (#3735)

618a656

* Fix in addition to #3571 * Update changelog

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix graphbuilder's elevation initialization #3571

fix graphbuilder's elevation initialization #3571

nilsnolde commented Mar 11, 2022

nilsnolde Mar 16, 2022

kevinkreiser Mar 16, 2022

nilsnolde Mar 17, 2022 •

edited

Loading

kevinkreiser Mar 17, 2022

nilsnolde Mar 17, 2022

nilsnolde Mar 17, 2022

nilsnolde Mar 17, 2022

nilsnolde Mar 16, 2022

nilsnolde Mar 16, 2022

kevinkreiser Mar 17, 2022

nilsnolde commented Mar 17, 2022

nilsnolde commented Mar 22, 2022

christian-nils Mar 29, 2022 •

edited

Loading

christian-nils Mar 29, 2022

kevinkreiser Mar 29, 2022 •

edited

Loading

fix graphbuilder's elevation initialization #3571

fix graphbuilder's elevation initialization #3571

Conversation

nilsnolde commented Mar 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsnolde Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsnolde commented Mar 17, 2022

nilsnolde commented Mar 22, 2022

christian-nils Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

nilsnolde Mar 17, 2022 •

edited

Loading

christian-nils Mar 29, 2022 •

edited

Loading

kevinkreiser Mar 29, 2022 •

edited

Loading