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 Cesium ion geocoder to CesiumIonClient::Connection #1019

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@

### ? - ?

##### Additions :tada:

- Added `CesiumIonClient::Connection::geocode` method for making geocoding queries against the Cesium ion geocoder API.

##### Fixes :wrench:

- Fixed a bug in `thenPassThrough` that caused a compiler error when given a value by r-value refrence.

### v0.42.0 - 2024-12-02
Expand Down
1 change: 1 addition & 0 deletions CesiumIonClient/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ target_link_libraries(CesiumIonClient
PUBLIC
CesiumAsync
CesiumUtility
CesiumGeospatial
PRIVATE
picosha2::picosha2
modp_b64::modp_b64
Expand Down
16 changes: 16 additions & 0 deletions CesiumIonClient/include/CesiumIonClient/Connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "ApplicationData.h"
#include "Assets.h"
#include "Defaults.h"
#include "Geocoder.h"
#include "Profile.h"
#include "Response.h"
#include "Token.h"
Expand Down Expand Up @@ -291,6 +292,21 @@ class CESIUMASYNC_API Connection {
const std::vector<std::string>& newScopes,
const std::optional<std::vector<std::string>>& newAllowedUrls) const;

/**
* @brief Makes a request to the ion geocoding service.
*
* A geocoding service is used to make a plain text query (like an address,
* city name, or landmark) and obtain information about where it's located.
*
* @param provider The ion geocoding provider to use.
* @param type The type of request to make. See {@link GeocoderRequestType} for more information.
* @param query The query to make.
*/
CesiumAsync::Future<Response<GeocoderResult>> geocode(
const GeocoderProviderType& provider,
const GeocoderRequestType& type,
Comment on lines +306 to +307
Copy link
Member

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.

const std::string& query);

/**
* @brief Decodes a token ID from a token.
*
Expand Down
131 changes: 131 additions & 0 deletions CesiumIonClient/include/CesiumIonClient/Geocoder.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
#pragma once

#include "CesiumGeospatial/Cartographic.h"
#include "CesiumGeospatial/GlobeRectangle.h"

#include <CesiumUtility/Math.h>

#include <string>
#include <variant>
#include <vector>

namespace CesiumIonClient {

/**
* @brief The supported types of requests to geocoding API.
*/
enum GeocoderRequestType {
/**
* @brief Perform a full search from a complete query.
*/
Search,

/**
* @brief Perform a quick search based on partial input, such as while a user
* is typing.
* The search results may be less accurate or exhaustive than using {@link GeocoderRequestType::Search}.
*/
Autocomplete
};

/**
* @brief The supported providers that can be accessed through ion's geocoder
* API.
*/
enum GeocoderProviderType {
/**
* @brief Google geocoder, for use with Google data.
*/
Google,

/**
* @brief Bing geocoder, for use with Bing data.
*/
Bing,

/**
* @brief Use the default geocoder as set on the server. Used when neither
* Bing or Google data is used.
*/
Default
};

/**
* @brief A single feature (a location or region) obtained from a geocoder
* service.
*/
struct GeocoderFeature {
/**
* @brief The user-friendly display name of this feature.
*/
std::string displayName;

/**
* @brief The region on the globe for this feature.
*/
std::variant<CesiumGeospatial::GlobeRectangle, CesiumGeospatial::Cartographic>
destination;

GeocoderFeature(
std::string& displayName_,
CesiumGeospatial::GlobeRectangle& destination_)
Comment on lines +70 to +71
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::string& displayName_,
CesiumGeospatial::GlobeRectangle& destination_)
const std::string& displayName_,
const CesiumGeospatial::GlobeRectangle& destination_)

: displayName(displayName_), destination(destination_) {}
GeocoderFeature(
std::string& displayName_,
CesiumGeospatial::Cartographic& destination_)
Comment on lines +74 to +75
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::string& displayName_,
CesiumGeospatial::Cartographic& destination_)
const std::string& displayName_,
const CesiumGeospatial::Cartographic& destination_)

Copy link
Member

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_) {}

Copy link
Member

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.

: displayName(displayName_), destination(destination_) {}

/**
* @brief Returns a {@link CesiumGeospatial::GlobeRectangle} representing this feature.
*
* If the geocoder service returned a bounding box for this result, this will
* return the bounding box. If the geocoder service returned a coordinate for
* this result, this will return a zero-width rectangle at that coordinate.
*/
CesiumGeospatial::GlobeRectangle getGlobeRectangle() const;

/**
* @brief Returns a {@link CesiumGeospatial::Cartographic} representing this feature.
*
* If the geocoder service returned a bounding box for this result, this will
* return the center of the bounding box. If the geocoder service returned a
* coordinate for this result, this will return the coordinate.
*/
CesiumGeospatial::Cartographic getCartographic() const;
};

/**
* @brief Attribution information for a query to a geocoder service.
*/
struct GeocoderAttribution {
/**
* @brief An HTML string containing the necessary attribution information.
*/
std::string html;

/**
* @brief If true, the credit should be visible in the main credit container.
* Otherwise, it can appear in a popover.
*/
bool showOnScreen;

GeocoderAttribution(std::string& html_, bool showOnScreen_)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GeocoderAttribution(std::string& html_, bool showOnScreen_)
GeocoderAttribution(const std::string& html_, bool showOnScreen_)

: html(html_), showOnScreen(showOnScreen_) {}
};

/**
* @brief The result of making a request to a geocoder service.
*/
struct GeocoderResult {
/**
* @brief Any necessary attributions for this geocoder result.
*/
std::vector<GeocoderAttribution> attributions;

/**
* @brief The features obtained from this geocoder service, if any.
*/
std::vector<GeocoderFeature> features;
};

}; // namespace CesiumIonClient
146 changes: 146 additions & 0 deletions CesiumIonClient/src/Connection.cpp
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
#include "CesiumIonClient/Connection.h"

#include "CesiumGeospatial/BoundingRegion.h"
#include "CesiumGeospatial/Cartographic.h"
#include "CesiumGeospatial/GlobeRectangle.h"
#include "CesiumIonClient/Geocoder.h"
Comment on lines +3 to +6
Copy link
Member

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".

#include "fillWithRandomBytes.h"
#include "parseLinkHeader.h"

#include <CesiumAsync/IAssetResponse.h>
#include <CesiumUtility/JsonHelpers.h>
#include <CesiumUtility/Result.h>
#include <CesiumUtility/SpanHelper.h>
#include <CesiumUtility/Uri.h>
#include <CesiumUtility/joinToString.h>
Expand All @@ -13,6 +18,7 @@
#include <modp_b64.h>
#include <rapidjson/document.h>
#include <rapidjson/error/en.h>
#include <rapidjson/pointer.h>
#include <rapidjson/stringbuffer.h>
#include <rapidjson/writer.h>
#include <uriparser/Uri.h>
Expand Down Expand Up @@ -582,6 +588,85 @@ Defaults defaultsFromJson(const rapidjson::Document& json) {
return defaults;
}

GeocoderResult geocoderResultFromJson(const rapidjson::Document& json) {
GeocoderResult result;

auto featuresMemberIt = json.FindMember("features");
if (featuresMemberIt != json.MemberEnd() &&
featuresMemberIt->value.IsArray()) {
auto featuresIt = featuresMemberIt->value.GetArray();
Copy link
Member

Choose a reason for hiding this comment

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

This isn't an iterator, is it?

for (auto& feature : featuresIt) {
const rapidjson::Value* pLabel =
rapidjson::Pointer("/properties/label").Get(feature);
Comment on lines +599 to +600
Copy link
Member

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.

if (!pLabel) {
SPDLOG_WARN("Missing label for geocoder feature");
Copy link
Member

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.

continue;
}

std::string label(pLabel->GetString());
Copy link
Member

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).

auto bboxMemberIt = feature.FindMember("bbox");
if (bboxMemberIt == feature.MemberEnd() ||
!bboxMemberIt->value.IsArray()) {
// Could be a point value.
const rapidjson::Value* pCoordinates =
rapidjson::Pointer("/geometry/coordinates").Get(feature);
if (!pCoordinates) {
SPDLOG_WARN(
"Missing bbox and geometry.coordinates for geocoder feature");
continue;
}

if (!pCoordinates->IsArray() || pCoordinates->Size() != 2) {
SPDLOG_WARN("geometry.coordinates must be an array of size 2");
continue;
}

auto coordinatesArray = pCoordinates->GetArray();

CesiumGeospatial::Cartographic point =
CesiumGeospatial::Cartographic::fromDegrees(
JsonHelpers::getDoubleOrDefault(coordinatesArray[0], 0),
JsonHelpers::getDoubleOrDefault(coordinatesArray[1], 0));

result.features.emplace_back(label, point);
} else {
auto bboxIt = bboxMemberIt->value.GetArray();
if (bboxIt.Size() != 4) {
SPDLOG_WARN("bbox property should have exactly four values");
continue;
}

CesiumGeospatial::GlobeRectangle rect =
CesiumGeospatial::GlobeRectangle::fromDegrees(
JsonHelpers::getDoubleOrDefault(bboxIt[0], 0),
JsonHelpers::getDoubleOrDefault(bboxIt[1], 0),
JsonHelpers::getDoubleOrDefault(bboxIt[2], 0),
JsonHelpers::getDoubleOrDefault(bboxIt[3], 0));

result.features.emplace_back(label, rect);
}
}
}

auto attributionMemberIt = json.FindMember("attributions");
if (attributionMemberIt != json.MemberEnd() &&
attributionMemberIt->value.IsArray()) {
const auto& valueJson = attributionMemberIt->value;

result.attributions.reserve(valueJson.Size());

for (rapidjson::SizeType i = 0; i < valueJson.Size(); ++i) {
const auto& element = valueJson[i];
Copy link
Member

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:

Suggested change
const auto& element = valueJson[i];
const rapidjson::Value& element = valueJson[i];

std::string html = JsonHelpers::getStringOrDefault(element, "html", "");
bool showOnScreen =
!JsonHelpers::getBoolOrDefault(element, "collapsible", false);
result.attributions.emplace_back(html, showOnScreen);
}
}

return result;
}

} // namespace

Future<Response<Defaults>> Connection::defaults() const {
Expand Down Expand Up @@ -1179,3 +1264,64 @@ Connection::tokens(const std::string& url) const {
return Response<TokenList>(pRequest, tokenListFromJson(d));
});
}

CesiumAsync::Future<Response<GeocoderResult>> Connection::geocode(
const GeocoderProviderType& provider,
const GeocoderRequestType& type,
const std::string& query) {
const std::string endpointUrl = type == GeocoderRequestType::Autocomplete
? "v1/geocode/autocomplete"
: "v1/geocode/search";
std::string requestUrl =
CesiumUtility::Uri::resolve(this->_apiUrl, endpointUrl);
requestUrl = CesiumUtility::Uri::addQuery(requestUrl, "text", query);

// Add provider type to url
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;
}
Comment on lines +1280 to +1291
Copy link
Member

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


return this->_pAssetAccessor
->get(
this->_asyncSystem,
requestUrl,
{{"Accept", "application/json"},
{"Authorization", "Bearer " + this->_accessToken}})
.thenInMainThread(
[](std::shared_ptr<CesiumAsync::IAssetRequest>&& pRequest) {
const IAssetResponse* pResponse = pRequest->response();
if (!pResponse) {
return createEmptyResponse<GeocoderResult>();
}

if (pResponse->statusCode() < 200 ||
pResponse->statusCode() >= 300) {
return createErrorResponse<GeocoderResult>(pResponse);
}

rapidjson::Document d;
if (!parseJsonObject(pResponse, d)) {
return createJsonErrorResponse<GeocoderResult>(pResponse, d);
}
if (!d.IsObject()) {
return createJsonTypeResponse<GeocoderResult>(
pResponse,
"object");
}

return Response<GeocoderResult>(
geocoderResultFromJson(d),
pResponse->statusCode(),
std::string(),
std::string());
});
}
Loading