Cache ion endpoints #442
Cache ion endpoints #442
Conversation
| @@ -60,6 +60,9 @@ struct Tileset::LoadIonAssetEndpoint::Private { | |||
| assert(false); | |||
| }); | |||
| } | |||
|
|
|||
| static std::unordered_map<std::string, std::shared_ptr<IAssetRequest>> | |||
| endpointCache; | |||
There was a problem hiding this comment.
We shouldn't hold onto the IAssetRequest instance like this. The problem is that IAssetRequest is implemented by cesium-native's client (e.g. Cesium for Unreal or O3DE or whatever) and may (probably does) hold some of the client's resources hostage. For example, maybe there's a limit on the number of requests that can exist at once, and these requests will count against that limit. Or maybe it's really important that request resources be released before, say, the HTTP module shuts down, but we can't do that if a static field is keeping them alive.
So it may be possible to just cache the parts of the request/response that are needed. Or, if necessary, you can even create another class that implements IAssetRequest and store instances of that class in the cache instead. We do something similar in CachingAssetAccessor.cpp.
|
Getting errors with the last commit. I will investigate. |
There was a problem hiding this comment.
Looks good @joseph-kaile! Very minor comments below.
We should also extend this mechanism IonRasterOverlay, which has a nearly identical situation. But it can be a separate PR.
| tileset.getExternals().pCreditSystem->createCredit( | ||
| html->value.GetString())); | ||
| endpointAttribution.html = html->value.GetString(); | ||
| endpoint.attributions.push_back(endpointAttribution); |
There was a problem hiding this comment.
Slightly cleaner / more performant to do:
AssetEndpointAttribution& endpointAttribution = endpoint.attributions.emplace_back();
endpointAttribution.html = html->value.GetString();
There was a problem hiding this comment.
Thanks for the tip.
| */ | ||
| bool updateContextWithNewToken( | ||
| TileContext* pContext, | ||
| std::optional<std::string> GetNewAccessToken( |
There was a problem hiding this comment.
Use camelCase for function names.
| std::optional<std::string> GetNewAccessToken( | |
| std::optional<std::string> getNewAccessToken( |
|
Thanks @joseph-kaile! |
Fixes #382