Add Cesium ion geocoder to CesiumIonClient::Connection #1019
Conversation
| std::string& displayName_, | ||
| CesiumGeospatial::GlobeRectangle& destination_) |
There was a problem hiding this comment.
| 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.
| std::string& displayName_, | |
| CesiumGeospatial::Cartographic& destination_) | |
| const std::string& displayName_, | |
| const CesiumGeospatial::Cartographic& destination_) |
There was a problem hiding this comment.
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.
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.
| 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.
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.
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.
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.
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.
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.
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.
CesiumJS seems to use lowercase names, and leaves the parameter out entirely for Default:
https://github.com/CesiumGS/cesium/pull/12299/files#diff-1a5903d86aa48fe1b27e5f0d8e5da57e1afc40ee0e1ee6029fded82c23f5b1aaR28
|
Request to get this finished from the forums: https://community.cesium.com/t/geocoder-for-unity/37435 |
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
geocodemethod toCesiumIonClient::Connectionmodeled 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.