-
Notifications
You must be signed in to change notification settings - Fork 216
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 Cesium ion geocoder to CesiumIonClient::Connection #1019
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @azrogers! Just some nitpicky comments here.
std::string& displayName_, | ||
CesiumGeospatial::GlobeRectangle& destination_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::string& displayName_, | |
CesiumGeospatial::GlobeRectangle& destination_) | |
const std::string& displayName_, | |
const CesiumGeospatial::GlobeRectangle& destination_) |
std::string& displayName_, | ||
CesiumGeospatial::Cartographic& destination_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::string& displayName_, | |
CesiumGeospatial::Cartographic& destination_) | |
const std::string& displayName_, | |
const CesiumGeospatial::Cartographic& destination_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you could do this:
GeocoderFeature(
std::string displayName_,
const CesiumGeospatial::Cartographic& destination_)
: displayName(std::move(displayName_)), destination(destination_) {}
Which would be slightly more efficient because you can optionally move the string into the constructor (moving a GlobeRectangle isn't any different from copying it).
Slightly more efficient still would be two constructors, but that's almost certainly not worth the noise:
GeocoderFeature(
std::string&& displayName_,
const CesiumGeospatial::Cartographic& destination_)
: displayName(std::move(displayName_)), destination(destination_) {}
GeocoderFeature(
const std::string& displayName_,
const CesiumGeospatial::Cartographic& destination_)
: displayName(displayName_), destination(destination_) {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another possibility here is to just get rid of the constructors entirely for these simple value-holding types. Users could still create instances with almost the exact same syntax, except they'd have to use curly braces.
*/ | ||
bool showOnScreen; | ||
|
||
GeocoderAttribution(std::string& html_, bool showOnScreen_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GeocoderAttribution(std::string& html_, bool showOnScreen_) | |
GeocoderAttribution(const std::string& html_, bool showOnScreen_) |
const GeocoderProviderType& provider, | ||
const GeocoderRequestType& type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we pass enums by value instead of const reference, cause they're just integers.
#include "CesiumGeospatial/BoundingRegion.h" | ||
#include "CesiumGeospatial/Cartographic.h" | ||
#include "CesiumGeospatial/GlobeRectangle.h" | ||
#include "CesiumIonClient/Geocoder.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is inconsistent to begin with, but we should move toward #include <CesiumHeader/Whatever.h>
instead of #include "CesiumHeader/Whatever.h"
.
const rapidjson::Value* pLabel = | ||
rapidjson::Pointer("/properties/label").Get(feature); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal, but it'd be nice to move the rapidjson::Pointer
construction outside the loop. No reason to do it repeatedly.
continue; | ||
} | ||
|
||
std::string label(pLabel->GetString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if properties.label
isn't a string? It'd be safer to either check IsString
, or use JsonHelpers::getStringOrDefault
(which will do this for you).
const rapidjson::Value* pLabel = | ||
rapidjson::Pointer("/properties/label").Get(feature); | ||
if (!pLabel) { | ||
SPDLOG_WARN("Missing label for geocoder feature"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should generally avoid logging this sort of thing directly, because it's not accessible programmatically and in some cases it might be viewed as spam. The pattern used elsewhere in this file (use the OrDefault
methods to silently fill in a default if the JSON doesn't match what we expect) isn't perfect, but better to stick with that for consistency.
result.attributions.reserve(valueJson.Size()); | ||
|
||
for (rapidjson::SizeType i = 0; i < valueJson.Size(); ++i) { | ||
const auto& element = valueJson[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to use auto
for iterators, but this is more clear without:
const auto& element = valueJson[i]; | |
const rapidjson::Value& element = valueJson[i]; |
switch (provider) { | ||
case GeocoderProviderType::Bing: | ||
requestUrl = CesiumUtility::Uri::addQuery(requestUrl, "geocoder", "BING"); | ||
break; | ||
case GeocoderProviderType::Google: | ||
requestUrl = CesiumUtility::Uri::addQuery(requestUrl, "geocoder", "GOOGLE"); | ||
break; | ||
case GeocoderProviderType::Default: | ||
requestUrl = | ||
CesiumUtility::Uri::addQuery(requestUrl, "geocoder", "DEFAULT"); | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CesiumJS seems to use lowercase names, and leaves the parameter out entirely for Default
:
https://github.com/CesiumGS/cesium/pull/12299/files#diff-1a5903d86aa48fe1b27e5f0d8e5da57e1afc40ee0e1ee6029fded82c23f5b1aaR28
As described in #974, we want to make it easier for users to find the coordinates of locations of interest in our integrations, as an alternative to looking up the coordinates through some other service and entering them manually. The first step of this is completing #70, an implementation of the Cesium ion geocoder API, which is what this PR is for. It adds a
geocode
method toCesiumIonClient::Connection
modeled on the CesiumJS geocoder API. This isn't a complete geocoder implementation supporting multiple services, but it will allow us to implement CesiumGS/cesium-unreal#99 to improve Cesium for Unreal's ease of use.